feat: add DBMS_OUTPUT package #15
Conversation
📝 WalkthroughWalkthroughAdds an Oracle-compatible DBMS_OUTPUT feature to contrib/ivorysql_ora: a C backend exposing SQL-callable functions, SQL package/types and DDL, regression tests, and build/merge registration for compilation and test runs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as PL/SQL client
participant Package as dbms_output (SQL package)
participant CLayer as C layer (ora_dbms_output_*)
participant Buffer as Per-backend buffer
participant Txn as Transaction manager
rect rgba(135,206,235,0.08)
Note over Client,Package: Enable
Client->>Package: dbms_output.enable(size)
Package->>CLayer: ora_dbms_output_enable(size)
CLayer->>Buffer: init / allocate
CLayer-->>Package: success
Package-->>Client: return
end
rect rgba(144,238,144,0.06)
Note over Client,Package: Emit lines
Client->>Package: put / put_line / new_line
Package->>CLayer: ora_dbms_output_put*(...)
CLayer->>Buffer: append / enforce limits
CLayer-->>Package: success
Package-->>Client: return
end
rect rgba(255,228,196,0.06)
Note over Client,Package: Retrieve
Client->>Package: get_line / get_lines
Package->>CLayer: ora_dbms_output_get_*(...)
CLayer->>Buffer: read / pop lines, set status
CLayer-->>Package: return line(s)/status
Package-->>Client: OUT params populated
end
rect rgba(221,160,221,0.06)
Note over Txn,CLayer: Transaction lifecycle
Client->>Txn: COMMIT / ROLLBACK
Txn->>CLayer: tx callback
CLayer->>Buffer: clear/reset as appropriate
Txn-->>Client: complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
contrib/ivorysql_ora/src/builtin_functions/dbms_output--1.0.sql (1)
73-76: Consider matching or omitting the default in the package body.The package specification declares
DEFAULT 20000(line 62), while the package body declaresDEFAULT NULL. Although the specification's default takes precedence for external callers, this inconsistency can be confusing. Consider either omitting the default in the body or matching the specification.Apply this diff to improve clarity:
- PROCEDURE enable(buffer_size INTEGER DEFAULT NULL) IS + PROCEDURE enable(buffer_size INTEGER) IS BEGIN PERFORM sys.ora_dbms_output_enable(buffer_size); END;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/pl/plisql/src/expected/plisql_dbms_output.outis excluded by!**/*.out
📒 Files selected for processing (6)
contrib/ivorysql_ora/Makefile(1 hunks)contrib/ivorysql_ora/ivorysql_ora_merge_sqls(1 hunks)contrib/ivorysql_ora/src/builtin_functions/dbms_output--1.0.sql(1 hunks)contrib/ivorysql_ora/src/builtin_functions/dbms_output.c(1 hunks)src/pl/plisql/src/Makefile(1 hunks)src/pl/plisql/src/sql/plisql_dbms_output.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-28T00:28:54.063Z
Learnt from: rophy
Repo: rophy/IvorySQL PR: 9
File: src/pl/plisql/src/sql/plisql_autonomous.sql:301-317
Timestamp: 2025-11-28T00:28:54.063Z
Learning: In PostgreSQL regression tests executed via pg_regress, errors do not abort script execution - they are captured in the expected output. The `\set ON_ERROR_STOP` directives are only needed for interactive psql sessions and should be commented out in regression test files like `plisql_autonomous.sql`.
Applied to files:
src/pl/plisql/src/Makefilesrc/pl/plisql/src/sql/plisql_dbms_output.sql
🔇 Additional comments (24)
contrib/ivorysql_ora/ivorysql_ora_merge_sqls (1)
3-3: LGTM!The module path addition correctly registers the new DBMS_OUTPUT functionality in the build manifest.
contrib/ivorysql_ora/Makefile (1)
28-28: LGTM!The object file addition correctly integrates the DBMS_OUTPUT C implementation into the build.
src/pl/plisql/src/Makefile (1)
59-60: LGTM!The test module addition correctly integrates DBMS_OUTPUT regression tests into the plisql test suite.
src/pl/plisql/src/sql/plisql_dbms_output.sql (7)
7-36: LGTM!The basic enable/disable and PUT_LINE tests comprehensively cover core functionality including empty strings, NULL values, and PUT/NEW_LINE combinations.
38-65: LGTM!The procedure and function tests validate DBMS_OUTPUT behavior within stored procedures and functions, including loop iterations and return values.
79-88: LGTM!The buffer size boundary tests correctly validate Oracle's constraints (2000-1000000 bytes), testing both minimum/maximum valid values and invalid boundary cases.
89-110: LGTM!The data type and special character tests ensure proper handling of integers, numerics, booleans, tabs, quotes, and backslashes.
204-217: LGTM!The GET_LINE test correctly uses Oracle data types (VARCHAR2, INTEGER) for OUT parameters and validates both successful retrieval and empty buffer scenarios.
219-234: LGTM!The GET_LINES test correctly uses Oracle data types (VARCHAR2[], INTEGER) with IN OUT semantics and validates multiple line retrieval.
236-238: LGTM!Proper cleanup of test objects ensures no side effects for subsequent tests.
contrib/ivorysql_ora/src/builtin_functions/dbms_output--1.0.sql (3)
14-58: LGTM!The C function declarations are correctly marked as VOLATILE and properly map to the MODULE_PATHNAME implementation with appropriate return types.
40-48: LGTM!The composite types correctly use Oracle data types (VARCHAR2, INTEGER) to support OUT parameter semantics for GET_LINE and GET_LINES.
78-112: LGTM!The package body procedures correctly delegate to C functions and properly handle OUT parameter transformation from composite types.
contrib/ivorysql_ora/src/builtin_functions/dbms_output.c (11)
45-60: LGTM!The DbmsOutputBuffer structure is well-designed with appropriate fields for transaction-scoped buffering. Using a per-backend global is the correct approach for PostgreSQL extensions.
85-143: LGTM! Well-designed buffer initialization.The memory management is correct with dedicated memory context for buffer contents. The transaction callback registration logic is subtle but correct: callbacks are unregistered during cleanup and re-registered on subsequent ENABLE calls, preventing unnecessary callback invocations while maintaining proper cleanup semantics.
151-170: LGTM!The cleanup function correctly unregisters the transaction callback, deletes the dedicated memory context (freeing all buffer contents), and frees the buffer structure itself.
177-197: LGTM!The dynamic array growth logic is sound, using a 50% growth factor with a minimum increment of 100 lines.
205-231: LGTM!The buffer overflow check correctly validates before adding (Oracle behavior), and the error message matches Oracle's ORU-10027. The byte counting via strlen is appropriate for text data.
239-255: LGTM!The transaction callback correctly cleans up the buffer at transaction boundaries (COMMIT, ABORT, PREPARE), ensuring transaction-scoped semantics that match Oracle's DBMS_OUTPUT behavior.
265-288: LGTM!The ENABLE function correctly validates Oracle's buffer size constraints (2000-1000000 bytes) and handles NULL as unlimited, matching Oracle 10g R2+ behavior.
296-303: LGTM!The DISABLE function correctly sets enabled=false without clearing the buffer, matching Oracle's behavior where you can still retrieve lines after disabling.
371-391: LGTM!The NEW_LINE function correctly flushes pending PUT text or creates an empty line if no PUT text is accumulated, matching Oracle's behavior.
401-436: LGTM!The GET_LINE function correctly implements Oracle's semantics with status=0 for success and status=1 when no more lines are available, returning NULL (not empty string) for the line when exhausted.
446-508: LGTM!The GET_LINES function correctly implements Oracle's semantics, retrieving up to the requested number of lines and returning the actual count retrieved.
rophy
changed the title
rophy
changed the title
rophy
force-pushed
the
feat/dbms_output
branch
from
cc82d18 to
a157391
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
design/dbms_output/README.md (2)
11-16: Add language specifiers to fenced code blocks.The code block showing module location should have a language specifier for consistency and linting compliance.
-``` +```text src/pl/plisql/src/ ├── pl_dbms_output.c # C implementation ├── plisql--1.0.sql # Package definition (SQL) └── Makefile # Build configuration--- `26-59`: **Add language specifier to ASCII diagram block.** For markdown linting compliance, use `text` or `plaintext` as the language specifier for ASCII diagrams. </blockquote></details> <details> <summary>design/dbms_output/oracle-comparison.md (1)</summary><blockquote> `16-23`: **Consider adding language specifiers to output blocks.** For markdown linting compliance, consider using `text` as the language specifier for test output blocks throughout this document. This is a low-priority improvement that can be addressed later. </blockquote></details> <details> <summary>src/pl/plisql/src/pl_dbms_output.c (1)</summary><blockquote> `324-328`: **Minor: text_to_cstring allocations accumulate until transaction end.** The `text_to_cstring` result is allocated in the current memory context and not explicitly freed after being copied to the buffer. For applications making many PUT_LINE calls, this could accumulate memory until transaction end. This is a low-priority optimization since the memory is freed at transaction end anyway. If desired, you could wrap in a temporary context: ```c else { text *line_text = PG_GETARG_TEXT_PP(0); line_str = text_to_cstring(line_text); } /* ... use line_str ... */ /* line_str is freed when transaction ends */
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/pl/plisql/src/expected/plisql_dbms_output.outis excluded by!**/*.outsrc/pl/plisql/src/expected/plisql_nested_subproc2.outis excluded by!**/*.out
📒 Files selected for processing (7)
design/dbms_output/README.md(1 hunks)design/dbms_output/oracle-comparison.md(1 hunks)src/bin/initdb/initdb.c(1 hunks)src/pl/plisql/src/Makefile(2 hunks)src/pl/plisql/src/pl_dbms_output.c(1 hunks)src/pl/plisql/src/plisql--1.0.sql(1 hunks)src/pl/plisql/src/sql/plisql_dbms_output.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pl/plisql/src/Makefile
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-28T00:28:54.063Z
Learnt from: rophy
Repo: rophy/IvorySQL PR: 9
File: src/pl/plisql/src/sql/plisql_autonomous.sql:301-317
Timestamp: 2025-11-28T00:28:54.063Z
Learning: In PostgreSQL regression tests executed via pg_regress, errors do not abort script execution - they are captured in the expected output. The `\set ON_ERROR_STOP` directives are only needed for interactive psql sessions and should be commented out in regression test files like `plisql_autonomous.sql`.
Applied to files:
src/pl/plisql/src/sql/plisql_dbms_output.sql
🪛 markdownlint-cli2 (0.18.1)
design/dbms_output/oracle-comparison.md
16-16: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
21-21: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
40-40: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
54-54: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
59-59: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
70-70: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
75-75: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
88-88: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
93-93: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
106-106: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
111-111: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
122-122: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
127-127: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
138-138: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
143-143: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
154-154: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
161-161: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
176-176: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
181-181: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
192-192: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
197-197: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
208-208: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
213-213: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
230-230: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
235-235: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
248-248: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
253-253: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
266-266: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
271-271: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
282-282: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
287-287: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
298-298: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
303-303: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
316-316: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
321-321: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
334-334: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
339-339: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
355-355: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
363-363: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
377-377: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
385-385: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
399-399: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
408-408: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
425-425: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
430-430: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
441-441: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
448-448: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
463-463: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
465-465: Hard tabs
Column: 17
(MD010, no-hard-tabs)
471-471: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
473-473: Hard tabs
Column: 8
(MD010, no-hard-tabs)
485-485: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
490-490: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
501-501: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
506-506: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
517-517: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
524-524: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
537-537: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
546-546: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
561-561: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
569-569: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
design/dbms_output/README.md
11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
195-195: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (12)
src/bin/initdb/initdb.c (1)
2094-2097: LGTM! Mode switching for Oracle-compatible package syntax.The approach correctly mirrors the pattern used in
load_ivorysql_ora(lines 2113-2117). This ensures thePACKAGE dbms_outputsyntax inplisql--1.0.sqlis parsed correctly during extension creation.design/dbms_output/README.md (1)
157-165: Documentation accurately reflects implementation.The buffer structure definition matches the actual
DbmsOutputBufferstruct inpl_dbms_output.c, which is good for maintainability.src/pl/plisql/src/plisql--1.0.sql (1)
33-77: C function registrations and composite types look correct.The functions are appropriately marked as
VOLATILEsince they modify session-level buffer state. The composite typesdbms_output_lineanddbms_output_linescorrectly model the OUT parameter semantics forGET_LINEandGET_LINES.design/dbms_output/oracle-comparison.md (1)
596-611: Well-documented compatibility differences with actionable recommendations.The summary and recommendations sections provide clear guidance for users migrating from Oracle. The prioritization of the re-ENABLE behavior difference as "High Priority" is appropriate given its potential impact on migrated applications.
src/pl/plisql/src/sql/plisql_dbms_output.sql (3)
1-22: Well-structured test file with comprehensive coverage.The test design is sound - using GET operations to verify PUT operations ensures actual buffer content is tested, not just syntax. The section organization makes it easy to identify which functionality is being tested.
257-277: Good defensive overflow testing pattern.The test properly catches the exception within the loop and reports the line number where overflow occurred, matching the Oracle comparison document's expected behavior.
503-507: Cleanup section properly removes test objects.The cleanup section correctly drops the test procedure and function created during the tests, preventing test artifacts from persisting.
src/pl/plisql/src/pl_dbms_output.c (5)
137-143: Transaction callback registration may cause issues on repeated ENABLE calls.After
cleanup_output_buffer()is called (which unregisters the callback and setscallback_registered = false), the new buffer allocation ininit_output_buffersetscallback_registered = falseat line 133. However, the check at line 138 will then re-register the callback. This is correct behavior.However, there's a subtle issue:
output_buffer->callback_registeredis checked after being set tofalseat line 133, which means the callback will always be re-registered on everyENABLEcall. This is actually the correct behavior sincecleanup_output_bufferunregisters it.
151-170: Memory cleanup logic is correct.The cleanup properly:
- Unregisters the transaction callback
- Deletes
buffer_mcxt(freeing all lines and StringInfo)- Frees the
output_bufferstruct itself (allocated in TopMemoryContext)
290-303: DISABLE behavior correctly preserves buffer for GET operations.The comment accurately describes Oracle behavior: DISABLE doesn't clear the buffer immediately, allowing existing lines to still be retrieved via GET_LINE. The buffer is only cleared when ENABLE is subsequently called. This matches the test behavior in Test 3.2.
505-505: Array construction parameters are correct for TEXT type.The
construct_arraycall correctly uses-1(varlena length),false(not pass-by-value), andTYPALIGN_INTfor TEXT arrays, matching PostgreSQL's internal representation.
217-220: Oracle-compatible error code ORU-10027 for buffer overflow.Good attention to detail using the Oracle-compatible error code format for buffer overflow, which aids migration from Oracle databases.
rophy
force-pushed
the
feat/dbms_output
branch
2 times, most recently
from
faace27 to
b2e00fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pl/plisql/src/pl_dbms_output.c (1)
178-197: Consider overflow protection in growth calculation.The 1.5x growth strategy is sound, but line 188's
new_capacity = old + (old / 2)could theoretically overflow iflines_allocatedapproachesINT_MAX / 2. While unlikely in practice (memory and buffer_size limits would be hit first), you could add overflow protection:- new_capacity = output_buffer->lines_allocated + (output_buffer->lines_allocated / 2); + /* Check for overflow in growth calculation */ + if (output_buffer->lines_allocated > INT_MAX / 3) + new_capacity = INT_MAX; + else + new_capacity = output_buffer->lines_allocated + (output_buffer->lines_allocated / 2);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/pl/plisql/src/expected/plisql_dbms_output.outis excluded by!**/*.outsrc/pl/plisql/src/expected/plisql_nested_subproc2.outis excluded by!**/*.out
📒 Files selected for processing (5)
src/bin/initdb/initdb.c(1 hunks)src/pl/plisql/src/Makefile(2 hunks)src/pl/plisql/src/pl_dbms_output.c(1 hunks)src/pl/plisql/src/plisql--1.0.sql(1 hunks)src/pl/plisql/src/sql/plisql_dbms_output.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/bin/initdb/initdb.c
- src/pl/plisql/src/sql/plisql_dbms_output.sql
🧰 Additional context used
🧬 Code graph analysis (1)
src/pl/plisql/src/pl_dbms_output.c (9)
src/backend/utils/mmgr/mcxt.c (4)
palloc0(1350-1370)MemoryContextDelete(453-488)pfree(1527-1541)palloc(1320-1348)src/common/stringinfo.c (3)
makeStringInfo(71-75)appendStringInfoString(229-233)resetStringInfo(125-134)src/backend/access/transam/xact.c (2)
RegisterXactCallback(3804-3815)UnregisterXactCallback(3817-3836)src/backend/utils/adt/varlena.c (1)
text_to_cstring(226-242)src/backend/executor/execTuples.c (1)
BlessTupleDesc(2314-2322)src/include/postgres.h (2)
Int32GetDatum(216-220)PointerGetDatum(326-330)src/backend/access/common/heaptuple.c (1)
heap_form_tuple(1149-1235)src/include/funcapi.h (1)
HeapTupleGetDatum(336-340)src/backend/utils/adt/arrayfuncs.c (2)
construct_empty_array(3599-3610)construct_array(3380-3393)
🔇 Additional comments (16)
src/pl/plisql/src/Makefile (1)
43-43: LGTM - Clean build integration.The addition of
pl_dbms_output.oto the build andplisql_dbms_outputto the regression test suite follows the established patterns in this Makefile.Also applies to: 62-62
src/pl/plisql/src/plisql--1.0.sql (3)
33-77: LGTM - C function registrations are correct.All functions are properly registered with
VOLATILEqualification (appropriate for state-modifying operations), and the composite types are defined before use. TheDEFAULT 20000onora_dbms_output_enablealigns with Oracle's default buffer size.
80-88: LGTM - Package specification matches Oracle DBMS_OUTPUT.The procedure signatures correctly mirror Oracle's DBMS_OUTPUT API, including
OUTandIN OUTparameter modes.
90-133: LGTM - Package body correctly implements the specification.The default value alignment issue from the previous review has been resolved. Each procedure properly delegates to the underlying C functions, and the composite-to-OUT parameter mapping is implemented correctly.
src/pl/plisql/src/pl_dbms_output.c (12)
45-60: LGTM - Well-designed buffer structure.The per-backend global approach is safe for PostgreSQL's single-threaded backend model. The structure correctly tracks byte-based limits (matching Oracle), uses a dedicated memory context for efficient cleanup, and includes a
callback_registeredflag to prevent double-registration.
86-143: LGTM - Robust initialization with proper memory management.The function correctly implements Oracle's "ENABLE clears existing buffer" behavior. Memory context usage is sound: buffer structure in
TopMemoryContextfor persistence, contents in a dedicated context for efficient bulk cleanup. The transaction callback lifecycle is correctly managed—callbacks are registered once per buffer and properly unregistered during cleanup to avoid accumulation.
152-170: LGTM - Clean resource deallocation.The cleanup sequence is correct: unregister the transaction callback, delete the memory context (which bulk-frees all lines and the StringInfo), then free the buffer structure itself. Setting
output_buffer = NULLproperly signals the absence of a buffer.
206-231: LGTM - Correct byte-based buffer enforcement.The function properly enforces Oracle's byte-based buffer limits by checking
buffer_used + line_bytes > buffer_sizebefore adding the line. Usingstrlen()for byte counting is correct, and the Oracle error codeORU-10027aids compatibility.
240-255: LGTM - Transaction-scoped cleanup matches Oracle semantics.Cleaning up the buffer on
COMMIT,ABORT, andPREPAREcorrectly implements Oracle's transaction-scoped (not session-scoped) buffer behavior.
266-288: LGTM - Proper ENABLE semantics with Oracle-compatible constraints.The function correctly handles NULL (unlimited), enforces Oracle's 2000–1,000,000 byte range for explicit values, and delegates to
init_output_buffer, which clears any existing buffer (matching Oracle behavior).
297-303: LGTM - DISABLE correctly preserves buffer for reading.Setting
enabled = falsewithout cleanup matches Oracle's behavior: new writes are ignored, but existing lines remain retrievable viaGET_LINE/GET_LINESuntil transaction end.
313-344: LGTM - PUT_LINE correctly handles accumulated PUT text.The function properly flushes any pending
PUTtext by appending the new line tocurrent_linebefore flushing (lines 331–335), matching Oracle's behavior. Silent discard when disabled and NULL-as-empty-string are both Oracle-compatible.
354-375: LGTM - PUT correctly accumulates without flushing.Appending to
current_linewithout callingadd_line_to_buffercorrectly implements Oracle's behavior:PUTtext isn't retrievable untilNEW_LINEorPUT_LINEflushes it.
384-403: LGTM - NEW_LINE properly flushes accumulated text.The function correctly creates an empty line when called without prior
PUTtext (line 399), matching Oracle's behavior of makingNEW_LINEobservable rather than a no-op.
414-448: LGTM - GET_LINE correctly implements Oracle return semantics.Returning
NULLfor the line field (not an empty string) when the buffer is exhausted, along withstatus = 1, matches Oracle's API. The composite type construction is correct.
459-520: LGTM - GET_LINES correctly handles batch retrieval.The function properly returns the minimum of requested and available lines, constructs the TEXT array with correct alignment (
TYPALIGN_INTon line 505), and manages temporary allocations appropriately. Returning an empty array (not NULL) when no lines are available is the expected behavior.
rophy
force-pushed
the
feat/dbms_output
branch
from
b2e00fc to
47cac50
Compare
rophy
force-pushed
the
feat/dbms_output
branch
from
47cac50 to
5ed5163
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c (1)
180-200: Potential integer overflow in capacity calculation.If
lines_allocatedis very large (close to INT_MAX), the calculationlines_allocated + (lines_allocated / 2)could overflow. While unlikely in practice due to buffer size limits, consider adding a check./* Grow by 50% */ new_capacity = output_buffer->lines_allocated + (output_buffer->lines_allocated / 2); + if (new_capacity < output_buffer->lines_allocated) + ereport(ERROR, + (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), + errmsg("too many lines in DBMS_OUTPUT buffer"))); if (new_capacity < output_buffer->lines_allocated + 100) new_capacity = output_buffer->lines_allocated + 100;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
contrib/ivorysql_ora/expected/ora_dbms_output.outis excluded by!**/*.out
📒 Files selected for processing (2)
contrib/ivorysql_ora/sql/ora_dbms_output.sql(1 hunks)contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-28T00:28:54.063Z
Learnt from: rophy
Repo: rophy/IvorySQL PR: 9
File: src/pl/plisql/src/sql/plisql_autonomous.sql:301-317
Timestamp: 2025-11-28T00:28:54.063Z
Learning: In PostgreSQL regression tests executed via pg_regress, errors do not abort script execution - they are captured in the expected output. The `\set ON_ERROR_STOP` directives are only needed for interactive psql sessions and should be commented out in regression test files like `plisql_autonomous.sql`.
Applied to files:
contrib/ivorysql_ora/sql/ora_dbms_output.sql
🔇 Additional comments (14)
contrib/ivorysql_ora/sql/ora_dbms_output.sql (5)
1-76: LGTM! Well-structured basic tests.The test cases for basic PUT_LINE and GET_LINE operations are comprehensive, covering normal usage, empty strings, NULL handling, and empty buffer scenarios. The verification pattern of using GET operations to confirm PUT operations is sound.
147-162: Verify expected output for Test 3.1.The test expects to retrieve "After re-enable" after the disable/enable cycle. However, the comment on line 157 says "should only see what was put after re-enable", which is correct. But note that "Before disable" was buffered before disable was called, and per line 170-171, DISABLE clears the buffer. The expected behavior seems correct, but ensure the expected output file matches this logic.
207-277: LGTM! Buffer size and overflow tests are comprehensive.The tests correctly verify the Oracle-compatible buffer size constraints (2000-1000000 bytes) and properly test buffer overflow behavior with exception handling.
503-543: LGTM! Line length limit tests are thorough.The tests correctly verify the Oracle-compatible 32767 byte line limit, testing exact boundary (should succeed), overflow by one byte (should fail with ORU-10028), and accumulated PUT overflow.
545-549: LGTM! Proper cleanup of test objects.The test file correctly drops the temporary procedure and function created for testing.
contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c (9)
154-173: LGTM! Cleanup function is well-structured.The cleanup properly unregisters the callback, deletes the memory context (which frees all allocated lines), and then frees the buffer structure itself.
209-237: LGTM! Line addition logic is correct.The function properly handles NULL lines, checks for buffer overflow before adding, and allocates the line copy in the correct memory context.
319-373: LGTM! PUT_LINE implementation is correct.The function properly handles:
- NULL input (stores actual NULL)
- Pending PUT text accumulation
- Line length limit checks before operations
- Memory allocation in appropriate context
383-415: LGTM! PUT implementation follows Oracle behavior.The function correctly treats NULL as appending nothing and accumulates text in
current_lineuntil NEW_LINE or PUT_LINE is called.
423-443: LGTM! NEW_LINE correctly flushes accumulated text.The function properly flushes pending PUT text or creates an empty line if nothing was accumulated.
453-497: LGTM! GET_LINE implementation is correct.The function properly builds the composite return type, handles NULL lines stored in the buffer, and returns appropriate status codes (0 for success, 1 for no more lines).
507-585: LGTM! GET_LINES implementation is comprehensive.The function correctly:
- Handles the case when buffer is NULL
- Calculates actual available lines
- Constructs a 1-based array with proper NULL handling
- Frees temporary allocations
245-261: Verify safety of unregistering callback from within callback.The callback calls
cleanup_output_buffer()which callsUnregisterXactCallback(). Unregistering a callback from within the callback itself may have unexpected behavior depending on PostgreSQL's implementation.#!/bin/bash # Check how PostgreSQL handles unregistering callbacks from within callbacks rg -n "UnregisterXactCallback" --type=c -B5 -A10
88-146: The callback registration logic is correct; no callback accumulation issue exists.The cleanup function properly unregisters the callback at line 163 before setting
output_buffer = NULLat line 172. SinceUnregisterXactCallbackandRegisterXactCallbackare called with identical arguments (dbms_output_xact_callbackandNULL), they correctly pair and match. Each call toENABLEfollows the sequence: cleanup (unregister old callback) → allocate fresh buffer withcallback_registered = false→ register new callback. No accumulation occurs.Likely an incorrect or invalid review comment.
Implement Oracle-compatible DBMS_OUTPUT package providing: - PUT_LINE, PUT, NEW_LINE for buffered output - GET_LINE, GET_LINES for retrieving output - ENABLE/DISABLE for buffer control - Buffer overflow detection (ORU-10027) Located in contrib/ivorysql_ora/src/builtin_packages/dbms_output/ following IvorySQL maintainer guidance (discussion IvorySQL#988). Includes design documentation and Oracle compatibility comparison with 82% test compatibility rate (27/33 tests pass). Known differences from Oracle: - NULL stored as empty string vs NULL - Re-ENABLE clears buffer vs preserves - Buffer size range 2000-1000000 (Oracle: 2000-unlimited) - No 32767 byte line length limit
Oracle treats empty strings as NULL. Both PUT_LINE('') and
PUT_LINE(NULL) should store actual NULL values, which GET_LINE
returns as SQL NULL.
Verified against Oracle 23.26 Free.
Fixes #25
Oracle enforces a maximum line length of 32767 bytes per line. Exceeding this limit raises ORU-10028 error. - Check line length in PUT_LINE before adding to buffer - Check accumulated line length in PUT before appending - Add test cases for line length boundary conditions Verified against Oracle 23.26 Free. Fixes #21
rophy
force-pushed
the
feat/dbms_output
branch
from
5ed5163 to
ce711ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c (2)
140-145: Callback registration flag appears redundant.The
callback_registeredflag check at line 141 will always evaluate tofalsebecause:
- Line 94-95 cleans up any existing buffer (which unregisters the callback)
- Line 100 allocates a new buffer with
palloc0(which zeroes all fields includingcallback_registered)This means the callback will be unconditionally registered every time
init_output_buffer()is called, making the conditional check unnecessary.Consider simplifying to:
- /* Register transaction callback (only once per buffer lifecycle) */ - if (!output_buffer->callback_registered) - { - RegisterXactCallback(dbms_output_xact_callback, NULL); - output_buffer->callback_registered = true; - } + /* Register transaction callback for this buffer lifecycle */ + RegisterXactCallback(dbms_output_xact_callback, NULL);And remove the
callback_registeredfield from theDbmsOutputBufferstruct.
568-569: Use the TEXT type's alignment from the catalog instead of hardcoding TYPALIGN_INT.The code at lines 568-569 constructs a TEXT array using a hardcoded
TYPALIGN_INTalignment parameter. While TEXT happens to be 4-byte aligned in current PostgreSQL/IvorySQL versions, this should be retrieved dynamically from the type's catalog entry usingget_typalign(TEXTOID)or similar. This makes the code more maintainable and resilient to any future changes in type definitions.lines_array = construct_md_array(line_datums, line_nulls, 1, &actual_lines, &lbound, TEXTOID, -1, false, TYPALIGN_INT);Change to fetch alignment dynamically rather than hardcoding it.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
contrib/ivorysql_ora/expected/ora_dbms_output.outis excluded by!**/*.outsrc/pl/plisql/src/expected/plisql_nested_subproc2.outis excluded by!**/*.out
📒 Files selected for processing (5)
contrib/ivorysql_ora/Makefile(2 hunks)contrib/ivorysql_ora/ivorysql_ora_merge_sqls(1 hunks)contrib/ivorysql_ora/sql/ora_dbms_output.sql(1 hunks)contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output--1.0.sql(1 hunks)contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- contrib/ivorysql_ora/ivorysql_ora_merge_sqls
- contrib/ivorysql_ora/Makefile
- contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output--1.0.sql
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-28T00:28:54.063Z
Learnt from: rophy
Repo: rophy/IvorySQL PR: 9
File: src/pl/plisql/src/sql/plisql_autonomous.sql:301-317
Timestamp: 2025-11-28T00:28:54.063Z
Learning: In PostgreSQL regression tests executed via pg_regress, errors do not abort script execution - they are captured in the expected output. The `\set ON_ERROR_STOP` directives are only needed for interactive psql sessions and should be commented out in regression test files like `plisql_autonomous.sql`.
Applied to files:
contrib/ivorysql_ora/sql/ora_dbms_output.sql
🧬 Code graph analysis (1)
contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c (9)
src/include/utils/palloc.h (1)
MemoryContextSwitchTo(123-130)src/backend/utils/mmgr/mcxt.c (4)
palloc0(1350-1370)pfree(1527-1541)repalloc(1547-1581)palloc(1320-1348)src/common/stringinfo.c (3)
makeStringInfo(71-75)appendStringInfoString(229-233)resetStringInfo(125-134)src/backend/access/transam/xact.c (2)
RegisterXactCallback(3804-3815)UnregisterXactCallback(3817-3836)src/backend/utils/adt/varlena.c (1)
text_to_cstring(226-242)src/backend/executor/execTuples.c (1)
BlessTupleDesc(2314-2322)src/include/postgres.h (2)
Int32GetDatum(216-220)PointerGetDatum(326-330)src/include/funcapi.h (1)
HeapTupleGetDatum(336-340)src/backend/utils/adt/arrayfuncs.c (2)
construct_empty_array(3599-3610)construct_md_array(3513-3594)
🔇 Additional comments (2)
contrib/ivorysql_ora/sql/ora_dbms_output.sql (1)
1-549: Comprehensive test suite - well structured!The test suite provides excellent coverage of DBMS_OUTPUT functionality including:
- Basic PUT_LINE/GET_LINE operations
- PUT and NEW_LINE combinations
- ENABLE/DISABLE behavior and buffer lifecycle
- Buffer size validation and overflow handling
- Line length limits (32767 bytes)
- NULL handling semantics
- Integration with procedures/functions
- Edge cases and special characters
The tests properly handle expected errors using EXCEPTION blocks, and the cleanup section properly drops test artifacts.
contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c (1)
1-585: Solid Oracle-compatible implementation with correct semantics.The C implementation demonstrates strong understanding of:
✓ Memory Management: Proper use of TopMemoryContext for buffer structure and dedicated buffer_mcxt for contents, correct context switching throughout
✓ Transaction Lifecycle: Transaction callback correctly ensures buffer is transaction-scoped (Oracle behavior)
✓ Oracle Semantics:
- Line length limit of 32767 bytes enforced
- NULL handling matches Oracle (PUT_LINE stores NULL, PUT treats NULL as empty)
- Buffer size constraints (2000-1000000 bytes)
- Correct ORU error codes (ORU-10027, ORU-10028)
- DISABLE preserves buffer for GET_LINE (Oracle behavior)
✓ Correctness:
- Buffer overflow checked BEFORE adding lines
- Dynamic array growth with reasonable strategy (50% increase)
- Proper NULL pointer handling throughout
- Status codes and return values match Oracle specification
The implementation is production-ready with only minor optimization opportunities identified in separate comments.
- Fix potential integer overflow in buffer check by using subtraction instead of addition (line_bytes > buffer_size - buffer_used) - Normalize negative numlines parameter to 0 in GET_LINES
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c (3)
340-341: Consider freeing text_to_cstring result to avoid memory accumulation.The
text_to_cstringcall allocates memory in the per-statement context. In long-running statements with many PUT_LINE calls (e.g., in loops), this memory accumulates until the statement completes. Consider addingpfree(line_str)after line 371 to release the memory immediately.Apply this pattern after line 371:
/* Just add the line (may be NULL) */ add_line_to_buffer(line_str); } + + if (line_str != NULL) + pfree(line_str); PG_RETURN_VOID();
398-401: Remove unnecessary code block.The braced scope serves no purpose here and can be simplified.
Apply this diff:
/* Handle NULL argument - treat as empty string (Oracle behavior) */ if (PG_ARGISNULL(0)) PG_RETURN_VOID(); /* NULL appends nothing */ - { - text *text_arg = PG_GETARG_TEXT_PP(0); - str = text_to_cstring(text_arg); - } + text *text_arg = PG_GETARG_TEXT_PP(0); + str = text_to_cstring(text_arg);
400-413: Consider freeing text_to_cstring result to avoid memory accumulation.Similar to PUT_LINE, the
text_to_cstringallocation can accumulate in loops. Consider addingpfree(str)after line 413.Apply this diff:
/* Accumulate in current_line without creating a line yet */ appendStringInfoString(output_buffer->current_line, str); + pfree(str); PG_RETURN_VOID();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c (7)
src/include/utils/palloc.h (1)
MemoryContextSwitchTo(123-130)src/backend/utils/mmgr/mcxt.c (5)
palloc0(1350-1370)pfree(1527-1541)repalloc(1547-1581)pstrdup(1702-1706)palloc(1320-1348)src/backend/utils/adt/varlena.c (1)
text_to_cstring(226-242)src/backend/executor/execTuples.c (1)
BlessTupleDesc(2314-2322)src/include/postgres.h (2)
Int32GetDatum(216-220)PointerGetDatum(326-330)src/include/funcapi.h (1)
HeapTupleGetDatum(336-340)src/backend/utils/adt/arrayfuncs.c (2)
construct_empty_array(3599-3610)construct_md_array(3513-3594)
🔇 Additional comments (2)
contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c (2)
88-146: LGTM with excellent memory context management.The initialization properly separates the buffer structure (TopMemoryContext) from its contents (dedicated buffer_mcxt), and the transaction callback registration logic correctly handles re-initialization scenarios. The overflow-safe pre-allocation strategy (lines 121-128) is well thought out.
220-226: Excellent overflow protection using subtraction.Using
line_bytes > buffer_size - buffer_usedinstead ofline_bytes + buffer_used > buffer_sizeprevents potential integer overflow. This is a best practice for bounds checking.
Address CodeRabbit review feedback: - Change "full Oracle compatibility" to "high Oracle compatibility" - Clarify that re-ENABLE clearing buffer is IvorySQL behavior (not Oracle) - Clarify that buffer size range is IvorySQL-enforced (stricter than Oracle) - Reference GitHub issues #22 and #26 for tracking differences
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c (2)
92-150: LGTM! Robust initialization with proper memory management.The initialization correctly manages memory contexts, handles unlimited buffer sizes, and uses reasonable pre-allocation heuristics. The use of a dedicated memory context (
buffer_mcxt) ensures clean teardown.Optional: The
callback_registeredflag check at line 145 is always true sincecleanup_output_buffer()(line 99) setsoutput_bufferto NULL, and the new buffer is allocated withpalloc0(line 104), which zeros all fields. The flag adds defensive safety but could be simplified.
392-424: LGTM! Correct PUT implementation.The function properly accumulates text without creating a line and handles NULL per Oracle semantics (appends nothing).
Optional: The block scope at lines 406-409 is unnecessary:
- { - text *text_arg = PG_GETARG_TEXT_PP(0); - str = text_to_cstring(text_arg); - } + text *text_arg = PG_GETARG_TEXT_PP(0); + str = text_to_cstring(text_arg);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c (1)
src/include/funcapi.h (1)
HeapTupleGetDatum(336-340)
🔇 Additional comments (11)
contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c (11)
46-64: LGTM! Well-designed buffer structure.The
DbmsOutputBufferstructure is well-organized with appropriate fields for managing per-backend buffering, memory contexts, and transaction lifecycle. The per-backend global is safe given PostgreSQL's process model, and the 32767-byte line limit matches Oracle's specification.
158-177: LGTM! Proper resource cleanup.The cleanup function correctly unregisters the transaction callback, deletes the memory context (which automatically frees all buffer allocations), and prevents dangling pointers by setting
output_bufferto NULL.
184-204: LGTM! Efficient dynamic array growth.The 50% growth strategy with a minimum increment of 100 lines provides good amortized performance. The pointer is correctly updated after
repalloc, and memory context management is proper.
213-242: LGTM! Careful overflow protection and correct NULL handling.The buffer overflow check at line 226 correctly uses subtraction (
line_bytes > buffer_size - buffer_used) to avoid potential integer overflow in the comparison. NULL lines are properly stored as NULL pointers, matching Oracle semantics.
250-266: LGTM! Correct transaction-scoped lifecycle.The callback correctly handles all transaction-end events (ABORT, COMMIT, PREPARE) to ensure the buffer is transaction-scoped, matching Oracle behavior.
280-303: LGTM! Proper parameter validation and initialization.The function correctly handles NULL as unlimited and enforces explicit buffer size limits. The documented differences from Oracle behavior (GitHub issues #22 and #26 referenced in comments) show good awareness of compatibility trade-offs.
311-318: LGTM! Correct disable semantics.The implementation correctly preserves the buffer content while disabling new writes, allowing subsequent
GET_LINEcalls to retrieve existing data (Oracle-compatible behavior).
328-382: LGTM! Correct PUT_LINE implementation with proper length validation.The function correctly combines pending PUT text with the new line, enforces the 32767-byte line limit, and handles NULL arguments per Oracle semantics. The length check logic is sound given the enforced invariants.
432-452: LGTM! Correct NEW_LINE semantics.The function properly flushes accumulated PUT text and creates an empty line when called with no pending text, matching Oracle behavior.
462-506: LGTM! Correct GET_LINE implementation with proper NULL handling.The function correctly builds composite return values, handles NULL lines stored in the buffer (Oracle behavior), and uses appropriate status codes (0 for success, 1 for no more lines).
516-598: LGTM! Correct GET_LINES implementation with proper array construction.The function correctly:
- Calculates actual retrievable lines as the minimum of requested and available
- Uses 1-based array indexing (PostgreSQL convention)
- Handles NULL lines in the array
- Uses proper TEXT type parameters for
construct_md_array(TEXTOID, -1 typlen, false typbyval, TYPALIGN_INT alignment)- Frees temporary allocations after the array copies the data
Add dbms_output_line and dbms_output_lines composite types to the expected output for the collation sanity check query.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
contrib/ivorysql_ora/sql/ora_dbms_output.sql (1)
211-212: Consider wrapping error cases in exception handlers for clearer test intent.Tests 4.1 and 4.4 (lines 212, 239) expect failures but don't explicitly verify the error. While pg_regress captures errors in expected output, wrapping these in DECLARE blocks with EXCEPTION handlers would make the test intent clearer and verify the specific error codes.
Optional: Explicit error verification
--- Test 4.1: Buffer size below minimum (should fail) -CALL dbms_output.enable(1000); +DECLARE +BEGIN + dbms_output.enable(1000); + RAISE NOTICE 'Test 4.1 - Should not reach here'; +EXCEPTION WHEN OTHERS THEN + RAISE NOTICE 'Test 4.1 - Expected error: %', SQLERRM; +END; +/Similar pattern for Test 4.4 at line 239.
contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c (1)
183-203: Growth calculation could benefit from overflow check.While the buffer size constraints make this unlikely, the line array growth calculation at line 194 could theoretically overflow for very large
lines_allocatedvalues. Consider adding an overflow check or capping the growth.Optional: Add overflow safety check
/* Grow by 50% */ new_capacity = output_buffer->lines_allocated + (output_buffer->lines_allocated / 2); + /* Guard against overflow */ + if (new_capacity < output_buffer->lines_allocated) + new_capacity = INT_MAX; if (new_capacity < output_buffer->lines_allocated + 100) new_capacity = output_buffer->lines_allocated + 100;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
contrib/ivorysql_ora/expected/ora_dbms_output.outis excluded by!**/*.outsrc/oracle_test/regress/expected/opr_sanity.outis excluded by!**/*.outsrc/test/regress/expected/opr_sanity_1.outis excluded by!**/*.out
📒 Files selected for processing (2)
contrib/ivorysql_ora/sql/ora_dbms_output.sqlcontrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-28T00:28:54.087Z
Learnt from: rophy
Repo: rophy/IvorySQL PR: 9
File: src/pl/plisql/src/sql/plisql_autonomous.sql:301-317
Timestamp: 2025-11-28T00:28:54.087Z
Learning: In PostgreSQL regression tests executed via pg_regress, errors do not abort script execution - they are captured in the expected output. The `\set ON_ERROR_STOP` directives are only needed for interactive psql sessions and should be commented out in regression test files like `plisql_autonomous.sql`.
Applied to files:
contrib/ivorysql_ora/sql/ora_dbms_output.sql
🧬 Code graph analysis (1)
contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c (6)
src/include/utils/palloc.h (1)
MemoryContextSwitchTo(123-130)src/backend/utils/mmgr/mcxt.c (3)
palloc0(1350-1370)pfree(1527-1541)palloc(1320-1348)src/backend/utils/adt/varlena.c (1)
text_to_cstring(226-242)src/backend/executor/execTuples.c (1)
BlessTupleDesc(2314-2322)src/include/funcapi.h (1)
HeapTupleGetDatum(336-340)src/backend/utils/adt/arrayfuncs.c (2)
construct_empty_array(3599-3610)construct_md_array(3513-3594)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: contrib_regression
- GitHub Check: oracle_pg_regression (ubuntu-latest)
- GitHub Check: pg_regression (ubuntu-latest)
- GitHub Check: meson_build (ubuntu-latest)
- GitHub Check: oracle_regression (ubuntu-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (7)
contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c (7)
212-241: LGTM! Excellent overflow prevention.The buffer overflow check at line 225 correctly uses subtraction to avoid potential integer overflow, and the explicit comment documents this design choice. NULL line handling is also correct.
282-306: LGTM! NULL handling and validation are correct.The function correctly handles NULL arguments by defaulting to maximum buffer size and properly validates the size range. The implementation matches the documented behavior.
331-385: LGTM! NULL handling with pending PUT text is correct.The function correctly handles the interaction between pending PUT text and PUT_LINE(NULL). When there's pending PUT text, NULL appends nothing but still finalizes the line; when there's no pending text, NULL is stored as an actual NULL line. This matches Oracle behavior.
395-427: LGTM! Text handling and line length checks are correct.The function correctly handles NULL (appends nothing), validates line length before accumulation, and properly manages memory by copying text into the buffer's memory context.
435-455: LGTM! NEW_LINE behavior is correct.The function correctly handles both cases: flushing accumulated PUT text and creating an empty line when there's no pending text. The distinction between empty string and NULL is properly maintained.
465-509: LGTM! Tuple return handling is correct.The function properly constructs and returns a composite type with correct NULL handling for both the line content and the status indicator. Oracle-compatible behavior is maintained.
519-601: LGTM! Array construction and NULL handling are correct.The function properly constructs a TEXT array with 1-based indexing, correctly handles NULL array elements, and uses appropriate type parameters for
construct_md_array. Memory management with temporary arrays is also correct.
Buffer now persists across COMMIT/ROLLBACK instead of being cleared. This matches Oracle's behavior where the buffer is local to the session. - Remove transaction callback registration - Remove dbms_output_xact_callback function - Remove callback_registered field from struct - Remove unused xact.h include
Test 10.1: Verify buffer persists after block ends (implicit commit) Test 10.2: Verify multiple blocks accumulate in buffer
Section 11 tests document current limitation where VARCHAR2 OUT parameters fail with "corresponding argument is not writable" error. Oracle's GET_LINE uses VARCHAR2 for OUT parameters, but IvorySQL's implementation uses TEXT. Implicit casts don't work for OUT parameters. Tests expect ERROR output to document current behavior before fix.
Oracle uses VARCHAR2 for GET_LINE and GET_LINES OUT parameters. When users declare variables as VARCHAR2 (common Oracle practice), implicit casts don't work for OUT parameters because the value cannot be written back through the cast. Add overloaded procedures: - get_line(line OUT VARCHAR2, status OUT INTEGER) - get_lines(lines OUT VARCHAR2[], numlines IN OUT INTEGER) This allows migrated Oracle code using VARCHAR2 to work correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contrib/ivorysql_ora/sql/ora_dbms_output.sql (1)
164-176: Clarify test comment: Re-ENABLE clears, not DISABLEThe comment "DISABLE clears existing buffer" is technically inaccurate. Based on the C implementation,
disable()only setsenabled=false(line 274 in dbms_output.c). The buffer is actually cleared by the subsequentenable()call at line 172, which callsinit_output_buffer()that clears any existing buffer. The test itself is correct and produces the expected result, but the comment could mislead readers about when the buffer is cleared.Suggested comment clarification
--- Test 3.2: DISABLE clears existing buffer +-- Test 3.2: Re-ENABLE clears existing buffer (not DISABLE itself)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
contrib/ivorysql_ora/expected/ora_dbms_output.outis excluded by!**/*.out
📒 Files selected for processing (3)
contrib/ivorysql_ora/sql/ora_dbms_output.sqlcontrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output--1.0.sqlcontrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-28T00:28:54.087Z
Learnt from: rophy
Repo: rophy/IvorySQL PR: 9
File: src/pl/plisql/src/sql/plisql_autonomous.sql:301-317
Timestamp: 2025-11-28T00:28:54.087Z
Learning: In PostgreSQL regression tests executed via pg_regress, errors do not abort script execution - they are captured in the expected output. The `\set ON_ERROR_STOP` directives are only needed for interactive psql sessions and should be commented out in regression test files like `plisql_autonomous.sql`.
Applied to files:
contrib/ivorysql_ora/sql/ora_dbms_output.sql
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: contrib_regression
- GitHub Check: oracle_regression (ubuntu-latest)
- GitHub Check: oracle_pg_regression (ubuntu-latest)
- GitHub Check: meson_build (ubuntu-latest)
- GitHub Check: pg_regression (ubuntu-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (15)
contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c (7)
58-70: LGTM - Safe per-backend designThe per-backend global buffer is safe in PostgreSQL's process model where each backend runs in a separate process. Constants are well-defined with appropriate limits.
97-138: Good memory context separationThe memory management design correctly separates the buffer structure (TopMemoryContext, long-lived) from buffer contents (dedicated context, can be bulk-freed). The initial capacity calculation with a minimum of 100 lines is reasonable.
194-223: Excellent overflow protection using subtractionLine 207 uses subtraction (
line_bytes > buffer_size - buffer_used) instead of addition to check for buffer overflow, preventing potential integer overflow. NULL lines are correctly stored as NULL pointers rather than empty strings, matching Oracle behavior.
238-262: Intentional stricter limits than OracleThe implementation enforces a 2000-1000000 byte range and treats NULL as maximum size (1M) rather than unlimited. This is documented as an "IvorySQL-enforced range" and "protective measure for the initial implementation." The behavior is intentional.
287-341: Correct NULL handling in PUT_LINEThe NULL handling correctly preserves Oracle semantics: PUT_LINE(NULL) stores an actual NULL line, and when combined with pending PUT text (e.g., PUT('hello') followed by PUT_LINE(NULL)), it creates a line with the accumulated text. The overflow checks properly account for NULL inputs (line_len=0).
421-465: Proper composite return type handlingThe function correctly constructs a composite return type (line TEXT, status INTEGER) and handles NULL lines by setting
nulls[0]=true. The read_position properly advances through the buffer.
475-557: Correct array handling with NULL supportThe function properly handles PostgreSQL's 1-based array indexing (line 518) and constructs arrays with NULL elements using
construct_md_arraywith theline_nullsarray. The actual line count (not requested count) is correctly returned.contrib/ivorysql_ora/sql/ora_dbms_output.sql (4)
211-212: Good boundary testing for buffer size limitsTests 4.1 and 4.4 correctly validate the enforced range (2000-1000000 bytes) by testing values just outside the boundaries. The bare CALL statements will capture errors in the regression test output as expected.
Based on learnings: In PostgreSQL regression tests, errors don't abort execution but are captured in expected output.
Also applies to: 238-239
549-608: Excellent session-scope testingThese tests properly validate that the buffer persists across transaction boundaries, which is essential for Oracle compatibility. Using separate DO blocks effectively simulates multiple transactions within the same session, confirming the buffer is stored in TopMemoryContext (per-backend, not per-transaction).
609-648: Good Oracle migration support with VARCHAR2These tests verify the overloaded procedures work with VARCHAR2 types, which is important for Oracle code migration. The package correctly provides both TEXT and VARCHAR2 variants of get_line/get_lines.
649-653: Proper test cleanupThe cleanup section correctly drops the test procedure and function created during Section 7 tests, maintaining good regression test hygiene.
contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output--1.0.sql (4)
26-71: Correct C function bindings with appropriate VOLATILE markingAll functions are correctly marked as VOLATILE since they have side effects (modifying session-level buffer state). Composite return types are properly defined before the functions that use them.
74-84: Well-designed package interface with Oracle compatibilityThe package specification correctly provides overloaded variants for TEXT and VARCHAR2 types, supporting Oracle code migration. The default buffer_size of 20000 bytes is a reasonable middle ground between the minimum (2000) and maximum (1000000).
86-111: Correct use of PERFORM for VOID-returning functionsThe wrapper procedures correctly use PERFORM to execute the C functions that return VOID. This is the appropriate pattern for discarding return values in PL/pgSQL.
113-143: Proper composite-to-OUT-parameter mappingThe get_line and get_lines procedures correctly unpack composite return values into OUT parameters using the
SELECT * INTO resultpattern. The overloaded VARCHAR2 variants are identical except for OUT parameter types, which is appropriate since PostgreSQL will handle the type conversion.
- Ring buffer with length-prefixed strings (2 bytes per line overhead) - Dynamic growth: expands by min(buffer_size, max(DEFAULT_BUFFER_SIZE, capacity * GROWTH_FACTOR)) - ENABLE(NULL) creates unlimited buffer starting at DEFAULT_BUFFER_SIZE - Values < MIN_BUFFER_SIZE silently clamped to minimum (Oracle behavior) - Buffer space recycled when lines are read via GET_LINE/GET_LINES - Per-line overhead not counted toward user-specified limit (Oracle behavior)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
contrib/ivorysql_ora/expected/ora_dbms_output.outis excluded by!**/*.out
📒 Files selected for processing (2)
contrib/ivorysql_ora/sql/ora_dbms_output.sqlcontrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: pg_regression (ubuntu-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: contrib_regression
- GitHub Check: meson_build (ubuntu-latest)
- GitHub Check: oracle_pg_regression (ubuntu-latest)
- GitHub Check: oracle_regression (ubuntu-latest)
🔇 Additional comments (7)
contrib/ivorysql_ora/sql/ora_dbms_output.sql (1)
1-719: Comprehensive test coverage!The test suite is well-organized and covers all major DBMS_OUTPUT functionality including edge cases (NULL handling, buffer overflow, line length limits, session scope, buffer recycling). The tests properly clean up created objects at the end.
contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c (6)
54-84: LGTM - Well-designed buffer structure.The ring buffer design with length-prefixed entries and NULL marker is solid. Constants are appropriate for Oracle compatibility.
284-354: LGTM - Ring buffer operations correctly handle wrap-around.The ring buffer read/write operations properly handle wrap-around using modulo arithmetic and split memcpy operations. The uint16 serialization is correct.
367-422: LGTM - Overflow checks and buffer management are correct.The function properly checks user-perceived limits before adding (Line 382 uses subtraction to avoid overflow), expands internal storage when needed, and correctly distinguishes between content bytes and internal overhead. The uint16 cast at Line 413 is safe because callers enforce
DBMS_OUTPUT_MAX_LINE_LENGTH(32767).
435-471: LGTM - Enable and disable functions are correctly implemented.The enable function properly handles NULL arguments (unlimited buffer), clamps below-minimum values per Oracle behavior, and stores the int32 argument in an int64 variable safely. The disable function correctly preserves the buffer while preventing new output.
481-607: LGTM - PUT operations correctly enforce line length limits.All PUT operations (put_line, put, new_line) properly check for
DBMS_OUTPUT_MAX_LINE_LENGTH(32767 bytes) before appending or adding lines, raisingORU-10028on overflow. NULL handling matches Oracle behavior.
617-783: LGTM - GET operations correctly recycle buffer space.Both get_line and get_lines properly advance the tail pointer to recycle buffer space (Oracle behavior), handle NULL lines using the NULL_MARKER, and return appropriate status codes. The 1-based array indexing at Line 728 is correct for Oracle compatibility.
| if (buffer_size == DBMS_OUTPUT_UNLIMITED) | ||
| initial_capacity = DBMS_OUTPUT_DEFAULT_BUFFER_SIZE; | ||
| else | ||
| initial_capacity = (int) buffer_size; | ||
|
|
||
| output_buffer->capacity = initial_capacity; | ||
| output_buffer->buffer = (char *) palloc(output_buffer->capacity); |
There was a problem hiding this comment.
Guard against integer overflow when casting buffer_size to int.
Line 158 casts buffer_size (int64) to int without checking for overflow. If a user calls dbms_output.enable() with a value greater than INT_MAX (2,147,483,647), this will cause integer overflow, leading to a small or negative initial_capacity, which will cause memory corruption or crash.
🔎 Proposed fix
if (buffer_size == DBMS_OUTPUT_UNLIMITED)
initial_capacity = DBMS_OUTPUT_DEFAULT_BUFFER_SIZE;
else
+ {
+ if (buffer_size > INT_MAX)
+ ereport(ERROR,
+ (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("buffer size %lld exceeds maximum allowed (%d bytes)",
+ (long long) buffer_size, INT_MAX)));
initial_capacity = (int) buffer_size;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (buffer_size == DBMS_OUTPUT_UNLIMITED) | |
| initial_capacity = DBMS_OUTPUT_DEFAULT_BUFFER_SIZE; | |
| else | |
| initial_capacity = (int) buffer_size; | |
| output_buffer->capacity = initial_capacity; | |
| output_buffer->buffer = (char *) palloc(output_buffer->capacity); | |
| if (buffer_size == DBMS_OUTPUT_UNLIMITED) | |
| initial_capacity = DBMS_OUTPUT_DEFAULT_BUFFER_SIZE; | |
| else | |
| { | |
| if (buffer_size > INT_MAX) | |
| ereport(ERROR, | |
| (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), | |
| errmsg("buffer size %lld exceeds maximum allowed (%d bytes)", | |
| (long long) buffer_size, INT_MAX))); | |
| initial_capacity = (int) buffer_size; | |
| } | |
| output_buffer->capacity = initial_capacity; | |
| output_buffer->buffer = (char *) palloc(output_buffer->capacity); |
| /* For fixed-size buffers, cap growth at buffer_size */ | ||
| if (output_buffer->buffer_size != DBMS_OUTPUT_UNLIMITED) | ||
| { | ||
| if (growth > output_buffer->buffer_size) | ||
| growth = (int) output_buffer->buffer_size; | ||
| } | ||
|
|
||
| /* Ensure we grow enough to fit needed_space */ | ||
| while (output_buffer->capacity + growth < internal_used + needed_space) | ||
| { | ||
| if (output_buffer->buffer_size != DBMS_OUTPUT_UNLIMITED) | ||
| growth += (int) output_buffer->buffer_size; | ||
| else | ||
| growth += DBMS_OUTPUT_DEFAULT_BUFFER_SIZE; |
There was a problem hiding this comment.
Guard against integer overflow in buffer expansion.
Lines 226-227 and 234 cast buffer_size (int64) to int without overflow checks. If buffer_size exceeds INT_MAX, these casts will overflow, leading to incorrect growth calculations and potential buffer corruption.
🔎 Proposed fix
/* For fixed-size buffers, cap growth at buffer_size */
if (output_buffer->buffer_size != DBMS_OUTPUT_UNLIMITED)
{
- if (growth > output_buffer->buffer_size)
- growth = (int) output_buffer->buffer_size;
+ if (output_buffer->buffer_size > INT_MAX)
+ ereport(ERROR,
+ (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("buffer size %lld exceeds maximum allowed (%d bytes)",
+ (long long) output_buffer->buffer_size, INT_MAX)));
+ if (growth > (int) output_buffer->buffer_size)
+ growth = (int) output_buffer->buffer_size;
}
/* Ensure we grow enough to fit needed_space */
while (output_buffer->capacity + growth < internal_used + needed_space)
{
if (output_buffer->buffer_size != DBMS_OUTPUT_UNLIMITED)
- growth += (int) output_buffer->buffer_size;
+ growth += (int) output_buffer->buffer_size; /* Safe after check above */
else
growth += DBMS_OUTPUT_DEFAULT_BUFFER_SIZE;
}🤖 Prompt for AI Agents
contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c around lines
223-236: the code casts output_buffer->buffer_size (int64) to int without
checking for overflow; change the growth logic to use a wider intermediate type
(e.g., int64_t or size_t) for calculations, clamp any value derived from
output_buffer->buffer_size to INT_MAX (or another safe upper bound) before
casting to int, and perform additions with overflow checks (or saturating
clamps) when updating growth and when testing capacity + growth < required size
so no integer overflow can occur.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.