Conversation
📝 WalkthroughWalkthroughAdds DBMS_UTILITY package to IvorySQL Oracle compatibility layer, providing FORMAT_ERROR_BACKTRACE, FORMAT_ERROR_STACK, and FORMAT_CALL_STACK functions. Implements C-level exception context and call stack accessors in PL/iSQL, with comprehensive SQL test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant User as PL/iSQL User Code
participant PLE as PL/iSQL Runtime
participant EH as Exception Handler
participant API as plisql API
participant DBU as DBMS_UTILITY C
participant Result as Caller
User->>PLE: Call procedure (may raise exception)
PLE-->>PLE: Execute code
alt Exception Occurs
PLE->>PLE: Store context in error_context_stack
PLE->>EH: Enter exception handler block
EH->>EH: Set exception_handling_estate = current_estate
EH->>User: WHEN exception =>
User->>DBU: DBMS_UTILITY.FORMAT_ERROR_BACKTRACE()
DBU->>API: plisql_get_current_exception_context()
API->>API: Lookup exception_handling_estate
API-->>DBU: Return exception context
DBU->>DBU: Transform lines to ORA-style format
DBU-->>Result: Return formatted backtrace
EH->>EH: Restore exception_handling_estate
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
src/bin/initdb/initdb.c (1)
2092-2098: Oracle-mode guard aroundCREATE EXTENSION plisqllooks correctSwitching
ivorysql.compatible_modetooraclefor the extension SQL and then back topgis a clean way to allowCREATE PACKAGEinside the extension while keeping the rest of initdb in PG mode. For future-proofing, ifload_plisqlever gets reused outside this initdb path, consider saving/restoring the previousivorysql.compatible_modevalue instead of hard-codingpg.src/pl/plisql/src/plisql.h (1)
1461-1468: New PL/iSQL exception/call stack accessors are well-scopedExporting
plisql_get_current_exception_context/message/sqlerrcodeandplisql_get_call_stackviaPGDLLEXPORTis a reasonable, minimal API for DBMS_UTILITY. Consider adding a brief comment (here or inpl_exec.c) clarifying ownership/lifetime of the returned strings (e.g., whether callers must copy or free them), sinceconst char *vschar *suggests different expectations.design/dbms_utility/PROPOSAL.md (3)
15-18: Add language specifier to fenced code block.Per static analysis (MD040), specify a language for the code block to improve rendering and syntax highlighting.
-``` +```text src/pl/plisql/src/ → plisql.so (PL/iSQL language runtime) contrib/ivorysql_ora/ → ivorysql_ora.so (Oracle compatibility extension)--- `63-73`: **Add language specifier to fenced code block.** Per static analysis (MD040), specify a language for the directory structure block. ```diff -``` +```text contrib/ivorysql_ora/ ├── src/builtin_packages/ │ └── dbms_utility/
144-146: Replace placeholder author name.The author field contains a placeholder that should be updated before merging.
contrib/ivorysql_ora/sql/dbms_utility.sql (1)
346-374: Consider adding an assertion for expected function count.The test counts function entries but only reports the count without verifying it matches the expected value (3 functions: level1 → level2 → level3). Adding an assertion would catch regressions.
v_count := (LENGTH(v_stack) - LENGTH(REPLACE(v_stack, 'function ', ''))) / 9; RAISE INFO 'Call stack has % function entries', v_count; + IF v_count < 3 THEN + RAISE INFO 'FAILURE: Expected at least 3 function entries'; + END IF;contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.c (3)
74-99: Windows platform limitation documented but consider a runtime log.Functions silently return NULL on Windows. Consider emitting a DEBUG-level notice on first call to help diagnose when DBMS_UTILITY functions are called in a Windows environment where they're unsupported.
#endif /* On Windows, function pointers remain NULL - features require plisql */ +#ifdef WIN32 + elog(DEBUG1, "DBMS_UTILITY functions require plisql which is not available on Windows"); +#endif }
170-171: Consider usingstrtolinstead ofatoifor safer line number parsing.
atoi()provides no error detection. If the format changes or there's unexpected input,strtol()would allow validation. This is a minor defensive coding improvement.- line_num_start = line_marker + 6; /* Skip " line " */ - line_num = atoi(line_num_start); + line_num_start = line_marker + 6; /* Skip " line " */ + char *endptr; + line_num = (int) strtol(line_num_start, &endptr, 10); + if (endptr == line_num_start) + { + pfree(func_name); + return false; + }
249-267: StringInfo memory is managed by PostgreSQL's memory context.The
result.dataallocated byinitStringInfois not explicitly freed beforecstring_to_textmakes a copy. This is acceptable because both allocations usepallocand will be cleaned up when the memory context is reset (typically at end of transaction). However, for long-running functions, explicitly freeingresult.dataafter copying would be more memory-efficient.pfree(context_copy); - PG_RETURN_TEXT_P(cstring_to_text(result.data)); + text *result_text = cstring_to_text(result.data); + pfree(result.data); + PG_RETURN_TEXT_P(result_text); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
contrib/ivorysql_ora/expected/dbms_utility.outis excluded by!**/*.out
📒 Files selected for processing (9)
contrib/ivorysql_ora/Makefile(2 hunks)contrib/ivorysql_ora/ivorysql_ora_merge_sqls(1 hunks)contrib/ivorysql_ora/sql/dbms_utility.sql(1 hunks)contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility--1.0.sql(1 hunks)contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.c(1 hunks)design/dbms_utility/PROPOSAL.md(1 hunks)src/bin/initdb/initdb.c(1 hunks)src/pl/plisql/src/pl_exec.c(4 hunks)src/pl/plisql/src/plisql.h(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: rophy
Repo: IvorySQL/IvorySQL PR: 998
File: contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c:82-146
Timestamp: 2025-12-03T06:50:27.735Z
Learning: In contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c, the behavior where ENABLE() clears the existing buffer (rather than preserving it like Oracle does) is an intentional known difference tracked in issue #26 at https://github.com/rophy/IvorySQL/issues/26 (medium impact, open).
Learnt from: rophy
Repo: IvorySQL/IvorySQL PR: 998
File: contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c:264-295
Timestamp: 2025-12-03T06:42:16.023Z
Learning: In contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c, the buffer size validation (2000–1000000 bytes) is an intentional IvorySQL-enforced constraint that diverges from Oracle behavior. The minimum constraint behavior is tracked in issue #22, and the maximum limit discussion is deferred as future work per rophy's conservative approach for initial implementation.
📚 Learning: 2025-12-03T06:42:16.023Z
Learnt from: rophy
Repo: IvorySQL/IvorySQL PR: 998
File: contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c:264-295
Timestamp: 2025-12-03T06:42:16.023Z
Learning: In contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c, the buffer size validation (2000–1000000 bytes) is an intentional IvorySQL-enforced constraint that diverges from Oracle behavior. The minimum constraint behavior is tracked in issue #22, and the maximum limit discussion is deferred as future work per rophy's conservative approach for initial implementation.
Applied to files:
contrib/ivorysql_ora/ivorysql_ora_merge_sqlscontrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility--1.0.sqlcontrib/ivorysql_ora/Makefilecontrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.cdesign/dbms_utility/PROPOSAL.mdcontrib/ivorysql_ora/sql/dbms_utility.sql
📚 Learning: 2025-12-03T06:50:27.735Z
Learnt from: rophy
Repo: IvorySQL/IvorySQL PR: 998
File: contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c:82-146
Timestamp: 2025-12-03T06:50:27.735Z
Learning: In contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c, the behavior where ENABLE() clears the existing buffer (rather than preserving it like Oracle does) is an intentional known difference tracked in issue #26 at https://github.com/rophy/IvorySQL/issues/26 (medium impact, open).
Applied to files:
contrib/ivorysql_ora/ivorysql_ora_merge_sqlssrc/bin/initdb/initdb.ccontrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility--1.0.sqlcontrib/ivorysql_ora/Makefilecontrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.cdesign/dbms_utility/PROPOSAL.mdcontrib/ivorysql_ora/sql/dbms_utility.sql
📚 Learning: 2025-09-02T10:43:25.257Z
Learnt from: syasirs
Repo: IvorySQL/IvorySQL PR: 858
File: src/Makefile.global.in:513-514
Timestamp: 2025-09-02T10:43:25.257Z
Learning: In IvorySQL Makefile.global.in, $(wildcard t/oracle/*.pl) is used instead of t/oracle/*.pl in Oracle TAP test discovery because the t/oracle directory is optional and may not exist in most builds. The wildcard function safely returns empty string when the directory is missing, preventing shell expansion errors.
Applied to files:
contrib/ivorysql_ora/Makefile
🧬 Code graph analysis (1)
contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.c (3)
src/backend/utils/mmgr/mcxt.c (3)
pnstrdup(1713-1725)pfree(1527-1541)pstrdup(1702-1706)src/common/stringinfo.c (2)
appendStringInfo(144-166)initStringInfo(96-100)src/backend/utils/adt/varlena.c (1)
cstring_to_text(193-197)
🪛 markdownlint-cli2 (0.18.1)
design/dbms_utility/PROPOSAL.md
15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
63-63: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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: meson_build (ubuntu-latest)
- GitHub Check: oracle_regression (ubuntu-latest)
- GitHub Check: oracle_pg_regression (ubuntu-latest)
- GitHub Check: contrib_regression
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (17)
contrib/ivorysql_ora/ivorysql_ora_merge_sqls (1)
5-5: Includingdbms_utilityin merged SQL inputs is consistentThe added
src/builtin_packages/dbms_utility/dbms_utilityentry matches the existing pattern and should correctly pull the new package SQL into the generatedivorysql_orascript.contrib/ivorysql_ora/Makefile (2)
5-32: Object list and regression targets correctly wired fordbms_utilityAdding
src/builtin_packages/dbms_utility/dbms_utility.otoOBJSanddbms_utility(plusora_xml_functions) toORA_REGRESScleanly integrates the new package into the build and test matrix. Naming and paths are consistent with the existing layout.Also applies to: 58-75
78-80: Extra include path forplisql.h—verify PGXS usageThe added
PG_CPPFLAGS += -I$(top_srcdir)/src/pl/plisql/srcis appropriate for in-tree builds and makes the PL/iSQL headers available todbms_utility.c. If you intend to support building this extension via PGXS from an installed tree, it’s worth double-checking thattop_srcdirstill points to a location whereplisql.his available; otherwise this include may need to be conditional or rely on the installed server include directories instead.contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility--1.0.sql (1)
1-48: DBMS_UTILITY SQL wiring for the three FORMAT_ routines looks sound*The C wrappers in
sys.*and thedbms_utilitypackage spec/body cleanly exposeFORMAT_ERROR_BACKTRACE,FORMAT_ERROR_STACK, andFORMAT_CALL_STACKwith Oracle-style signatures, while keeping the implementation details in internalsys.ora_*functions.LANGUAGE C VOLATILEis appropriate given their dependence on the current error and call stack. Just ensure this file runs in the same schema/search-path setup as your other builtin packages (e.g., DBMS_OUTPUT) so that unqualifiedDBMS_UTILITY.FORMAT_*calls resolve as expected.src/pl/plisql/src/pl_exec.c (1)
10024-10126: Accessor helpers for exception and call stack context look correct; verify external callers’ expectationsThe four new helpers:
plisql_get_current_exception_contextplisql_get_current_exception_messageplisql_get_current_exception_sqlerrcodeplisql_get_call_stackare consistent with existing PL/iSQL state:
- They only expose data when
exception_handling_estateis non‑NULL andcur_erroris set, i.e., while inside an EXCEPTION handler.- They return raw
ErrorDatafields for context/message/sqlerrcode, matching howGET STACKED DIAGNOSTICSusescur_error.plisql_get_call_stackwalkserror_context_stackand filters onplisql_exec_error_callback, which is exactly how PL/iSQL frames are identified; usingerr_stmt/err_varfor line numbers keeps it consistent withplisql_exec_error_callback.Two minor points to double‑check in the extension that consumes these APIs:
plisql_get_call_stackreturns a tab‑separated, newline‑delimited list of frames in the form
"<PLiSQL_function*> \t <lineno> \t <fn_signature>".
Ensure the dbms_utility side treats this as an internal format (not user‑visible) and parses it accordingly.The
contextandmessagepointers are owned by PL/iSQL’s exception machinery and live in the handler’sstmt_mcontext. Callers should treat them as read‑only, ephemeral C strings and copy into their own context when turning them into SQLtext.From the pl_exec side, the implementation looks sound.
contrib/ivorysql_ora/sql/dbms_utility.sql (4)
1-19: Good basic test structure with proper cleanup.The test correctly validates FORMAT_ERROR_BACKTRACE behavior in a simple exception scenario and cleans up after itself.
21-93: Good coverage of nested procedure call scenarios.Tests 2 and 3 properly exercise the backtrace functionality across multiple call stack depths, ensuring the full call chain is captured.
235-270: Critical test for context preservation across nested exception handlers.This test validates that calling a procedure with its own exception handler from within another exception handler doesn't corrupt the outer handler's backtrace context. This is essential for verifying the
save_exception_handling_estatemechanism described in the proposal.
394-441: Comprehensive integration test for all three DBMS_UTILITY functions.Test 17 effectively validates that all three functions return expected content patterns when used together in an exception handler. The boolean flag pattern provides clear pass/fail reporting.
contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.c (8)
56-60: Static function pointer caching is appropriate for PostgreSQL's execution model.Since PostgreSQL backends are single-threaded per connection, the non-atomic access to
lookup_attemptedand the cached function pointers is safe in this context.
190-192: Acknowledged: Schema lookup deferred as TODO.The hardcoded "PUBLIC" schema is a known limitation. The TODO comment appropriately marks this for future enhancement to look up the actual schema from
pg_proc.
275-300: SQLSTATE to Oracle error mapping provides reasonable coverage.The switch covers common error cases with appropriate Oracle equivalents. The generic fallback to ORA-06502 is reasonable for an initial implementation. Additional mappings can be added as needed.
331-333: Correct handling of no-exception case.Checking
sqlerrcode == 0(which corresponds toERRCODE_SUCCESSFUL_COMPLETION) correctly identifies when there's no active exception.
408-416: Frame skipping logic is correct.Skipping the first frame (the FORMAT_CALL_STACK function itself) is appropriate since Oracle's FORMAT_CALL_STACK also excludes itself from the output. The frame counting approach handles this cleanly.
458-463: Edge case handled correctly.Returning NULL when only the FORMAT_CALL_STACK frame exists (no user code in the stack) is the correct behavior. The
pfree(result.data)ensures no memory is leaked in this path.
1-42: Well-structured implementation following PostgreSQL extension conventions.The file is well-organized with clear separation between dynamic lookup, transformation logic, and the three exported functions. The use of function pointer typedefs and cached lookups is a clean approach for the cross-module integration.
455-456: Verify memory allocation contract forget_call_stack_fn()return value.The code calls
pfree(raw_stack)assuming palloc'd memory. Given that palloc'd memory is context-dependent in PL/iSQL (similar to plpgsql), verify thatget_call_stack_fn()returns memory allocated in a context that will survive the caller, or usesSPI_palloc/appropriate context switching. If standardpallocis used with a transient context, this could cause memory corruption. Document the memory allocation contract to prevent issues during future maintenance.
|
We're ready to merge this PR. Before that, could you supplement the contrib/ivorysql_ora/meson.build file to enable this feature's compilation via meson? |
|
Added commit 2043e1a, does this work? |
2043e1a to
8e9490c
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/pl/plisql/src/pl_exec.c (1)
118-123: Globalexception_handling_estatecan still dangle across transactions when errors escape a handlerThe save/restore logic around
exec_stmts(estate, exception->action)correctly setsexception_handling_estatewhile inside a handler and restores the previous value on normal handler exit. However, if code inside the handler raises an unhandledERRORthat propagates out of the PL/iSQL function, the restore path is never executed, so:
exception_handling_estatecontinues to point at a stack-localPLiSQL_execstatewhose frame has been unwound.- At the end of the transaction (ABORT),
plisql_xact_cbdoes not clearexception_handling_estate, so this stale pointer survives into later transactions.- The new public helpers (
plisql_get_current_exception_context/message/sqlerrcode) dereferenceexception_handling_estatewithout additional lifetime checks, so any caller that uses them when no active handler exists can hit freed memory and potentially crash.This is the same lifetime hole previously discussed in the earlier review; the missing piece here is to clear the global at transaction end so no stale estate pointer survives beyond the failed function call.
A minimal fix is to NULL out
exception_handling_estateon all xact-end events (commit/abort/prepare), keeping the fast-path behavior of the save/restore inexec_stmt_blockunchanged:Proposed fix: clear handler estate at transaction end
void plisql_xact_cb(XactEvent event, void *arg) { @@ - if (event == XACT_EVENT_COMMIT || - event == XACT_EVENT_PARALLEL_COMMIT || - event == XACT_EVENT_PREPARE) - { - simple_econtext_stack = NULL; - - if (shared_simple_eval_estate) - FreeExecutorState(shared_simple_eval_estate); - shared_simple_eval_estate = NULL; - if (shared_simple_eval_resowner) - ReleaseAllPlanCacheRefsInOwner(shared_simple_eval_resowner); - shared_simple_eval_resowner = NULL; - } - else if (event == XACT_EVENT_ABORT || - event == XACT_EVENT_PARALLEL_ABORT) - { - simple_econtext_stack = NULL; - shared_simple_eval_estate = NULL; - shared_simple_eval_resowner = NULL; - } + if (event == XACT_EVENT_COMMIT || + event == XACT_EVENT_PARALLEL_COMMIT || + event == XACT_EVENT_PREPARE) + { + simple_econtext_stack = NULL; + + if (shared_simple_eval_estate) + FreeExecutorState(shared_simple_eval_estate); + shared_simple_eval_estate = NULL; + if (shared_simple_eval_resowner) + ReleaseAllPlanCacheRefsInOwner(shared_simple_eval_resowner); + shared_simple_eval_resowner = NULL; + } + else if (event == XACT_EVENT_ABORT || + event == XACT_EVENT_PARALLEL_ABORT) + { + simple_econtext_stack = NULL; + shared_simple_eval_estate = NULL; + shared_simple_eval_resowner = NULL; + } + + /* + * Any unhandled ERROR that escaped PL/iSQL will have ended the current + * transaction; at this point no execstate from the old transaction is + * valid, so clear any leftover handler pointer. + */ + exception_handling_estate = NULL; }This preserves correct behavior for nested handlers (the local save/restore still governs intra-block semantics) while ensuring the exported helpers can’t see a stale
PLiSQL_execstateafter transaction boundaries. Based on past review comments.Also applies to: 1988-2153, 9515-9547
🧹 Nitpick comments (4)
src/pl/plisql/src/plisql.h (1)
1464-1471: Document memory ownership forplisql_get_call_stack.The
plisql_get_call_stack()function returns dynamically allocated memory (fromStringInfo.data). The API contract should explicitly document that:
- Callers are responsible for
pfree()-ing the returned pointer- The function returns
NULLwhen no PL/iSQL context is activeConsider adding a comment block documenting these new exports, their purpose, ownership semantics, and when they return NULL.
📝 Suggested documentation
+/* + * Exception context access API for DBMS_UTILITY and similar extensions. + * These functions expose the current PL/iSQL exception state. + * + * - plisql_get_current_exception_context(): Returns error context string or NULL + * - plisql_get_current_exception_message(): Returns error message or NULL + * - plisql_get_current_exception_sqlerrcode(): Returns SQLSTATE code or 0 + * - plisql_get_call_stack(): Returns allocated call stack string or NULL + * Caller must pfree() the returned string when non-NULL. + * + * All functions return NULL/0 when called outside an exception handler. + */ extern PGDLLEXPORT const char *plisql_get_current_exception_context(void);contrib/ivorysql_ora/sql/dbms_utility.sql (1)
326-375: Consider more robust call stack validation.Lines 333 and 353 use fragile string pattern matching:
- Line 333: Hard-codes the format header
----- PL/SQL Call Stack ------ Line 353: Counts function entries by substring occurrence of
'function 'While this validates Oracle-compatible formatting (which is the goal), the string-counting approach at line 353 is particularly brittle. Consider whether a regex-based approach or testing for minimum expected patterns would be more maintainable.
💡 Alternative validation approach
-- Instead of counting 'function ' substrings: -- Use pattern matching to verify structural elements IF v_stack ~ '.*function\s+\w+.*function\s+\w+.*function\s+\w+' THEN RAISE INFO 'Call stack has expected nested structure'; END IF;design/dbms_utility/PROPOSAL.md (1)
15-18: Add language specifiers to fenced code blocks.The code blocks at lines 15-18 and 63-73 should specify a language identifier for proper rendering. Consider using
textorplaintextfor directory tree structures.🔎 Proposed fixes
-``` +```text src/pl/plisql/src/ → plisql.so (PL/iSQL language runtime) contrib/ivorysql_ora/ → ivorysql_ora.so (Oracle compatibility extension)And for the second block: ```diff -``` +```text contrib/ivorysql_ora/ ├── src/builtin_packages/Also applies to: 63-73
contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.c (1)
190-192: Hardcoded schema needs attention.The schema is hardcoded to "PUBLIC" for all functions. The TODO comment indicates this should look up the actual schema from
pg_proccatalog. This may produce incorrect output when functions exist in other schemas.Do you want me to generate code that performs the schema lookup from
pg_proc, or should this be tracked as a separate issue?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
contrib/ivorysql_ora/expected/dbms_utility.outis excluded by!**/*.out
📒 Files selected for processing (12)
contrib/ivorysql_ora/Makefilecontrib/ivorysql_ora/ivorysql_ora_merge_sqlscontrib/ivorysql_ora/meson.buildcontrib/ivorysql_ora/sql/dbms_utility.sqlcontrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility--1.0.sqlcontrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.cdesign/dbms_utility/PROPOSAL.mdsrc/bin/initdb/initdb.csrc/pl/plisql/src/Makefilesrc/pl/plisql/src/meson.buildsrc/pl/plisql/src/pl_exec.csrc/pl/plisql/src/plisql.h
🚧 Files skipped from review as they are similar to previous changes (2)
- contrib/ivorysql_ora/ivorysql_ora_merge_sqls
- contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility--1.0.sql
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: rophy
Repo: IvorySQL/IvorySQL PR: 998
File: contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c:82-146
Timestamp: 2025-12-03T06:50:27.755Z
Learning: In contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c, the behavior where ENABLE() clears the existing buffer (rather than preserving it like Oracle does) is an intentional known difference tracked in issue #26 at https://github.com/rophy/IvorySQL/issues/26 (medium impact, open).
Learnt from: rophy
Repo: IvorySQL/IvorySQL PR: 998
File: contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c:264-295
Timestamp: 2025-12-03T06:42:16.044Z
Learning: In contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c, the buffer size validation (2000–1000000 bytes) is an intentional IvorySQL-enforced constraint that diverges from Oracle behavior. The minimum constraint behavior is tracked in issue #22, and the maximum limit discussion is deferred as future work per rophy's conservative approach for initial implementation.
📚 Learning: 2025-12-03T06:42:16.044Z
Learnt from: rophy
Repo: IvorySQL/IvorySQL PR: 998
File: contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c:264-295
Timestamp: 2025-12-03T06:42:16.044Z
Learning: In contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c, the buffer size validation (2000–1000000 bytes) is an intentional IvorySQL-enforced constraint that diverges from Oracle behavior. The minimum constraint behavior is tracked in issue #22, and the maximum limit discussion is deferred as future work per rophy's conservative approach for initial implementation.
Applied to files:
contrib/ivorysql_ora/sql/dbms_utility.sqlcontrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.cdesign/dbms_utility/PROPOSAL.mdcontrib/ivorysql_ora/meson.build
📚 Learning: 2025-12-03T06:50:27.755Z
Learnt from: rophy
Repo: IvorySQL/IvorySQL PR: 998
File: contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c:82-146
Timestamp: 2025-12-03T06:50:27.755Z
Learning: In contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c, the behavior where ENABLE() clears the existing buffer (rather than preserving it like Oracle does) is an intentional known difference tracked in issue #26 at https://github.com/rophy/IvorySQL/issues/26 (medium impact, open).
Applied to files:
contrib/ivorysql_ora/sql/dbms_utility.sqlcontrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.ccontrib/ivorysql_ora/Makefiledesign/dbms_utility/PROPOSAL.mdcontrib/ivorysql_ora/meson.build
📚 Learning: 2025-09-02T10:43:25.257Z
Learnt from: syasirs
Repo: IvorySQL/IvorySQL PR: 858
File: src/Makefile.global.in:513-514
Timestamp: 2025-09-02T10:43:25.257Z
Learning: In IvorySQL Makefile.global.in, $(wildcard t/oracle/*.pl) is used instead of t/oracle/*.pl in Oracle TAP test discovery because the t/oracle directory is optional and may not exist in most builds. The wildcard function safely returns empty string when the directory is missing, preventing shell expansion errors.
Applied to files:
src/pl/plisql/src/Makefilecontrib/ivorysql_ora/Makefilecontrib/ivorysql_ora/meson.build
🧬 Code graph analysis (2)
contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.c (3)
src/backend/utils/mmgr/mcxt.c (3)
pnstrdup(1713-1725)pfree(1527-1541)pstrdup(1702-1706)src/common/stringinfo.c (2)
appendStringInfo(144-166)initStringInfo(96-100)src/backend/utils/adt/varlena.c (1)
cstring_to_text(193-197)
src/pl/plisql/src/plisql.h (1)
src/pl/plisql/src/pl_exec.c (4)
plisql_get_current_exception_context(10074-10081)plisql_get_current_exception_message(10089-10096)plisql_get_current_exception_sqlerrcode(10104-10111)plisql_get_call_stack(10123-10166)
🪛 markdownlint-cli2 (0.18.1)
design/dbms_utility/PROPOSAL.md
15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
63-63: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (10)
src/pl/plisql/src/Makefile (1)
62-66: LGTM - Test registration looks correct.The addition of
dbms_utilityto the REGRESS test list properly registers the new test suite for the DBMS_UTILITY package functionality.src/pl/plisql/src/meson.build (1)
56-61: LGTM - Good refactoring for header reusability.Centralizing the include directory definition in
plisql_incallows external modules (likeivorysql_ora) to properly reference PL/iSQL headers. This is a clean approach that maintains consistency across the build system.contrib/ivorysql_ora/sql/dbms_utility.sql (2)
1-270: Excellent test coverage for FORMAT_ERROR_BACKTRACE.The test suite comprehensively covers:
- Basic error scenarios (Tests 1-9)
- Edge cases like no-exception handling (Test 6)
- Context preservation across nested exception handlers (Test 10)
Test 10 is particularly valuable as it validates that outer exception context remains accessible after inner handlers complete—an important correctness property for the API.
394-441: LGTM - Comprehensive integration test.Test 17 effectively validates all three DBMS_UTILITY functions together using appropriate Oracle-compatible format markers:
FORMAT_ERROR_BACKTRACE: Checks forORA-06512patternFORMAT_ERROR_STACK: Checks forORA-prefixFORMAT_CALL_STACK: Validates call stack header formatThe boolean aggregation pattern provides clear test pass/fail indication.
contrib/ivorysql_ora/meson.build (2)
29-29: LGTM - Proper build integration.The addition of
dbms_utility.cto sources andplisql_incto include directories correctly integrates the new module with access to the PL/iSQL API.Also applies to: 48-48
68-93: LGTM - Clean test organization.The reorganized test structure with separate
sqlandtapsections improves maintainability, anddbms_utilityis properly registered in the regression suite.contrib/ivorysql_ora/Makefile (2)
30-31: LGTM - Proper Makefile integration.The additions of
dbms_utility.otoOBJSanddbms_utilitytoORA_REGRESScorrectly integrate the new module into the build and test workflow.Also applies to: 73-74
78-79: LGTM - Well-documented include path.The addition of the PL/iSQL include path with an explanatory comment clearly documents why
dbms_utilityneeds access toplisql.h. This is consistent with the Meson build approach.src/bin/initdb/initdb.c (1)
2094-2098: The deferred error handling is consistent with codebase design and sufficient.The code uses the established pattern throughout initdb: all
PG_CMD_PUTScalls write to a buffered file descriptor, and all write failures (including mode switch failures) are captured via theoutput_failedflag and checked comprehensively incheck_ok(). The mode reset at line 2097 is queued immediately after the extension creation, ensuring it's included in the command batch. This matches the identical pattern inload_ivorysql_oraand is the standard error handling approach in the file.src/pl/plisql/src/pl_exec.c (1)
10065-10166: New exception accessors and call-stack helper look consistent with existing PL/iSQL mechanismsThe four helpers:
- Respect the
exception_handling_estateguard (returning NULL/0 when no current handler),- Read from
estate->cur_errorwithout modifying it, matching how GET STACKED DIAGNOSTICS usescur_error,- Use
error_context_stackwith a strictcallback == plisql_exec_error_callbackcheck to identify PL/iSQL frames, and- Format the call stack into a single
StringInfoowned by the caller’s memory context.This aligns with the surrounding executor and error-context machinery and should be sufficient for DBMS_UTILITY’s FORMAT_ERROR_* and FORMAT_CALL_STACK implementations.
| #ifndef WIN32 | ||
| #include <dlfcn.h> | ||
| #endif |
There was a problem hiding this comment.
Windows platform not supported.
The dynamic symbol lookup using dlsym is only available on non-Windows platforms. On Windows, all function pointers remain NULL, making the entire DBMS_UTILITY package non-functional. Consider either:
- Implementing Windows support using
GetProcAddressfromwindows.h - Documenting this platform limitation clearly in user-facing documentation
- Returning an explicit error message on Windows rather than silently returning NULL
💡 Windows implementation approach
If Windows support is desired, you could add:
#ifdef WIN32
#include <windows.h>
static void
lookup_plisql_functions(void)
{
if (lookup_attempted)
return;
lookup_attempted = true;
HMODULE plisql_module = GetModuleHandle("plisql.dll");
if (plisql_module != NULL)
{
get_exception_context_fn = (plisql_get_context_fn)
GetProcAddress(plisql_module, "plisql_get_current_exception_context");
// ... similar for other functions
}
}
#endifAlso applies to: 74-99
| initStringInfo(&result); | ||
|
|
||
| /* Make a copy since strtok_r modifies the string */ | ||
| context_copy = pstrdup(pg_context); | ||
|
|
||
| /* Parse and transform each line */ | ||
| line = strtok_r(context_copy, "\n", &saveptr); | ||
| while (line != NULL) | ||
| { | ||
| /* Skip empty lines */ | ||
| if (line[0] != '\0') | ||
| transform_and_append_line(&result, line); | ||
|
|
||
| line = strtok_r(NULL, "\n", &saveptr); | ||
| } | ||
|
|
||
| pfree(context_copy); | ||
|
|
||
| PG_RETURN_TEXT_P(cstring_to_text(result.data)); | ||
| } |
There was a problem hiding this comment.
Memory leak: StringInfo buffer not freed.
The result.data buffer allocated by initStringInfo() at line 249 is never freed. The cstring_to_text() function creates a copy of the string, so the original buffer leaks on every call.
🔎 Proposed fix
pfree(context_copy);
+
+ /* Convert to text and free the StringInfo buffer */
+ text *result_text = cstring_to_text(result.data);
+ pfree(result.data);
- PG_RETURN_TEXT_P(cstring_to_text(result.data));
+ PG_RETURN_TEXT_P(result_text);
}📝 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.
| initStringInfo(&result); | |
| /* Make a copy since strtok_r modifies the string */ | |
| context_copy = pstrdup(pg_context); | |
| /* Parse and transform each line */ | |
| line = strtok_r(context_copy, "\n", &saveptr); | |
| while (line != NULL) | |
| { | |
| /* Skip empty lines */ | |
| if (line[0] != '\0') | |
| transform_and_append_line(&result, line); | |
| line = strtok_r(NULL, "\n", &saveptr); | |
| } | |
| pfree(context_copy); | |
| PG_RETURN_TEXT_P(cstring_to_text(result.data)); | |
| } | |
| initStringInfo(&result); | |
| /* Make a copy since strtok_r modifies the string */ | |
| context_copy = pstrdup(pg_context); | |
| /* Parse and transform each line */ | |
| line = strtok_r(context_copy, "\n", &saveptr); | |
| while (line != NULL) | |
| { | |
| /* Skip empty lines */ | |
| if (line[0] != '\0') | |
| transform_and_append_line(&result, line); | |
| line = strtok_r(NULL, "\n", &saveptr); | |
| } | |
| pfree(context_copy); | |
| /* Convert to text and free the StringInfo buffer */ | |
| text *result_text = cstring_to_text(result.data); | |
| pfree(result.data); | |
| PG_RETURN_TEXT_P(result_text); | |
| } |
🤖 Prompt for AI Agents
In contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.c around
lines 249-268, the StringInfo buffer created by initStringInfo(&result) is never
freed causing a memory leak because cstring_to_text() makes its own copy; after
calling cstring_to_text(result.data) capture the returned text
Datum/PG_RETURN_TEXT_P value, pfree(result.data) to free the StringInfo buffer,
then return the text — ensure you free result.data before returning from the
function.
| initStringInfo(&result); | ||
|
|
||
| /* Format: ORA-XXXXX: <message> */ | ||
| appendStringInfo(&result, "%s: %s\n", sqlstate_to_ora_errnum(sqlerrcode), message); | ||
|
|
||
| PG_RETURN_TEXT_P(cstring_to_text(result.data)); | ||
| } |
There was a problem hiding this comment.
Memory leak: StringInfo buffer not freed.
Same issue as ora_format_error_backtrace - the result.data buffer allocated at line 335 is never freed.
🔎 Proposed fix
/* Format: ORA-XXXXX: <message> */
appendStringInfo(&result, "%s: %s\n", sqlstate_to_ora_errnum(sqlerrcode), message);
+
+ /* Convert to text and free the StringInfo buffer */
+ text *result_text = cstring_to_text(result.data);
+ pfree(result.data);
- PG_RETURN_TEXT_P(cstring_to_text(result.data));
+ PG_RETURN_TEXT_P(result_text);
}🤖 Prompt for AI Agents
In contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.c around
lines 335-341, the StringInfo buffer (result.data) allocated by initStringInfo
is passed to cstring_to_text but never freed, causing a memory leak; fix by
first converting result.data to a Text* (or Datum) using cstring_to_text, then
pfree(result.data) to release the StringInfo buffer, and finally return the
Text* (or Datum) value with PG_RETURN_TEXT_P (or PG_RETURN_DATUM) so the
temporary buffer is freed before returning.
| initStringInfo(&result); | ||
|
|
||
| /* Add Oracle-style header */ | ||
| appendStringInfo(&result, "----- PL/SQL Call Stack -----\n"); | ||
| appendStringInfo(&result, " object line object\n"); | ||
| appendStringInfo(&result, " handle number name\n"); | ||
|
|
||
| /* | ||
| * Parse the raw stack returned by plisql_get_call_stack. | ||
| * Format from plisql: "handle\tlineno\tsignature" per line | ||
| * | ||
| * Skip the first frame which is the FORMAT_CALL_STACK package function. | ||
| */ | ||
| stack_copy = pstrdup(raw_stack); | ||
| line = strtok_r(stack_copy, "\n", &saveptr); | ||
|
|
||
| while (line != NULL) | ||
| { | ||
| char *handle_str; | ||
| char *lineno_str; | ||
| char *signature; | ||
| char *tab1; | ||
| char *tab2; | ||
| char *func_upper; | ||
| int i; | ||
|
|
||
| frame_count++; | ||
|
|
||
| /* Skip the first frame (FORMAT_CALL_STACK itself) */ | ||
| if (frame_count == 1) | ||
| { | ||
| line = strtok_r(NULL, "\n", &saveptr); | ||
| continue; | ||
| } | ||
|
|
||
| /* Parse: handle\tlineno\tsignature */ | ||
| tab1 = strchr(line, '\t'); | ||
| if (!tab1) | ||
| { | ||
| line = strtok_r(NULL, "\n", &saveptr); | ||
| continue; | ||
| } | ||
|
|
||
| handle_str = pnstrdup(line, tab1 - line); | ||
| tab2 = strchr(tab1 + 1, '\t'); | ||
| if (!tab2) | ||
| { | ||
| pfree(handle_str); | ||
| line = strtok_r(NULL, "\n", &saveptr); | ||
| continue; | ||
| } | ||
|
|
||
| lineno_str = pnstrdup(tab1 + 1, tab2 - tab1 - 1); | ||
| signature = tab2 + 1; | ||
|
|
||
| /* Convert function name to uppercase for Oracle compatibility */ | ||
| func_upper = pstrdup(signature); | ||
| for (i = 0; func_upper[i]; i++) | ||
| func_upper[i] = pg_toupper((unsigned char) func_upper[i]); | ||
|
|
||
| /* Format each stack frame */ | ||
| appendStringInfo(&result, "%s %5s function %s\n", | ||
| handle_str, lineno_str, func_upper); | ||
| found_any = true; | ||
|
|
||
| pfree(handle_str); | ||
| pfree(lineno_str); | ||
| pfree(func_upper); | ||
|
|
||
| line = strtok_r(NULL, "\n", &saveptr); | ||
| } | ||
|
|
||
| pfree(stack_copy); | ||
| pfree(raw_stack); | ||
|
|
||
| /* If we only had the FORMAT_CALL_STACK frame, return NULL */ | ||
| if (!found_any) | ||
| { | ||
| pfree(result.data); | ||
| PG_RETURN_NULL(); | ||
| } | ||
|
|
||
| PG_RETURN_TEXT_P(cstring_to_text(result.data)); | ||
| } |
There was a problem hiding this comment.
Memory leak: StringInfo buffer not freed in success path.
The code correctly frees result.data at line 461 when returning NULL, but fails to free it at line 465 in the success path. This inconsistency suggests awareness of the need to free but an oversight in the main return.
🔎 Proposed fix
pfree(stack_copy);
pfree(raw_stack);
/* If we only had the FORMAT_CALL_STACK frame, return NULL */
if (!found_any)
{
pfree(result.data);
PG_RETURN_NULL();
}
+
+ /* Convert to text and free the StringInfo buffer */
+ text *result_text = cstring_to_text(result.data);
+ pfree(result.data);
- PG_RETURN_TEXT_P(cstring_to_text(result.data));
+ PG_RETURN_TEXT_P(result_text);
}📝 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.
| initStringInfo(&result); | |
| /* Add Oracle-style header */ | |
| appendStringInfo(&result, "----- PL/SQL Call Stack -----\n"); | |
| appendStringInfo(&result, " object line object\n"); | |
| appendStringInfo(&result, " handle number name\n"); | |
| /* | |
| * Parse the raw stack returned by plisql_get_call_stack. | |
| * Format from plisql: "handle\tlineno\tsignature" per line | |
| * | |
| * Skip the first frame which is the FORMAT_CALL_STACK package function. | |
| */ | |
| stack_copy = pstrdup(raw_stack); | |
| line = strtok_r(stack_copy, "\n", &saveptr); | |
| while (line != NULL) | |
| { | |
| char *handle_str; | |
| char *lineno_str; | |
| char *signature; | |
| char *tab1; | |
| char *tab2; | |
| char *func_upper; | |
| int i; | |
| frame_count++; | |
| /* Skip the first frame (FORMAT_CALL_STACK itself) */ | |
| if (frame_count == 1) | |
| { | |
| line = strtok_r(NULL, "\n", &saveptr); | |
| continue; | |
| } | |
| /* Parse: handle\tlineno\tsignature */ | |
| tab1 = strchr(line, '\t'); | |
| if (!tab1) | |
| { | |
| line = strtok_r(NULL, "\n", &saveptr); | |
| continue; | |
| } | |
| handle_str = pnstrdup(line, tab1 - line); | |
| tab2 = strchr(tab1 + 1, '\t'); | |
| if (!tab2) | |
| { | |
| pfree(handle_str); | |
| line = strtok_r(NULL, "\n", &saveptr); | |
| continue; | |
| } | |
| lineno_str = pnstrdup(tab1 + 1, tab2 - tab1 - 1); | |
| signature = tab2 + 1; | |
| /* Convert function name to uppercase for Oracle compatibility */ | |
| func_upper = pstrdup(signature); | |
| for (i = 0; func_upper[i]; i++) | |
| func_upper[i] = pg_toupper((unsigned char) func_upper[i]); | |
| /* Format each stack frame */ | |
| appendStringInfo(&result, "%s %5s function %s\n", | |
| handle_str, lineno_str, func_upper); | |
| found_any = true; | |
| pfree(handle_str); | |
| pfree(lineno_str); | |
| pfree(func_upper); | |
| line = strtok_r(NULL, "\n", &saveptr); | |
| } | |
| pfree(stack_copy); | |
| pfree(raw_stack); | |
| /* If we only had the FORMAT_CALL_STACK frame, return NULL */ | |
| if (!found_any) | |
| { | |
| pfree(result.data); | |
| PG_RETURN_NULL(); | |
| } | |
| PG_RETURN_TEXT_P(cstring_to_text(result.data)); | |
| } | |
| initStringInfo(&result); | |
| /* Add Oracle-style header */ | |
| appendStringInfo(&result, "----- PL/SQL Call Stack -----\n"); | |
| appendStringInfo(&result, " object line object\n"); | |
| appendStringInfo(&result, " handle number name\n"); | |
| /* | |
| * Parse the raw stack returned by plisql_get_call_stack. | |
| * Format from plisql: "handle\tlineno\tsignature" per line | |
| * | |
| * Skip the first frame which is the FORMAT_CALL_STACK package function. | |
| */ | |
| stack_copy = pstrdup(raw_stack); | |
| line = strtok_r(stack_copy, "\n", &saveptr); | |
| while (line != NULL) | |
| { | |
| char *handle_str; | |
| char *lineno_str; | |
| char *signature; | |
| char *tab1; | |
| char *tab2; | |
| char *func_upper; | |
| int i; | |
| frame_count++; | |
| /* Skip the first frame (FORMAT_CALL_STACK itself) */ | |
| if (frame_count == 1) | |
| { | |
| line = strtok_r(NULL, "\n", &saveptr); | |
| continue; | |
| } | |
| /* Parse: handle\tlineno\tsignature */ | |
| tab1 = strchr(line, '\t'); | |
| if (!tab1) | |
| { | |
| line = strtok_r(NULL, "\n", &saveptr); | |
| continue; | |
| } | |
| handle_str = pnstrdup(line, tab1 - line); | |
| tab2 = strchr(tab1 + 1, '\t'); | |
| if (!tab2) | |
| { | |
| pfree(handle_str); | |
| line = strtok_r(NULL, "\n", &saveptr); | |
| continue; | |
| } | |
| lineno_str = pnstrdup(tab1 + 1, tab2 - tab1 - 1); | |
| signature = tab2 + 1; | |
| /* Convert function name to uppercase for Oracle compatibility */ | |
| func_upper = pstrdup(signature); | |
| for (i = 0; func_upper[i]; i++) | |
| func_upper[i] = pg_toupper((unsigned char) func_upper[i]); | |
| /* Format each stack frame */ | |
| appendStringInfo(&result, "%s %5s function %s\n", | |
| handle_str, lineno_str, func_upper); | |
| found_any = true; | |
| pfree(handle_str); | |
| pfree(lineno_str); | |
| pfree(func_upper); | |
| line = strtok_r(NULL, "\n", &saveptr); | |
| } | |
| pfree(stack_copy); | |
| pfree(raw_stack); | |
| /* If we only had the FORMAT_CALL_STACK frame, return NULL */ | |
| if (!found_any) | |
| { | |
| pfree(result.data); | |
| PG_RETURN_NULL(); | |
| } | |
| /* Convert to text and free the StringInfo buffer */ | |
| text *result_text = cstring_to_text(result.data); | |
| pfree(result.data); | |
| PG_RETURN_TEXT_P(result_text); | |
| } |
🤖 Prompt for AI Agents
In contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.c around
lines 383-466, the StringInfo buffer result.data is freed on the NULL return
path but not on the success path: calling
PG_RETURN_TEXT_P(cstring_to_text(result.data)) leaks result.data. Fix by
converting result.data to a text* into a temporary variable (text *t =
cstring_to_text(result.data)), then pfree(result.data), and finally
PG_RETURN_TEXT_P(t) so the buffer is freed before returning.
8e9490c to
67f14dd
Compare
|
Fixed the meson build commit to only include dbms_utility.c following this comment #1141 (comment) Rebased to latest master to resolve merge conflicts. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/pl/plisql/src/pl_exec.c (1)
118-124: Re‑raise: globalexception_handling_estatecan dangle across transactions without xact‑end cleanupThe save/restore pattern around
exception_handling_estateinside the EXCEPTION handler body (Lines 1988, 2140-2145, 2150-2152) is correct for the handled path, but the lifetime hole from the previous review still exists:
exception_handling_estateis a static global (Lines 118-124).- When an error is matched by a handler, you set
exception_handling_estate = estatebeforeexec_stmts(estate, exception->action)and restore it afterwards.- If code inside
exception->actionraises an unhandledERRORthat propagates out of the PL/iSQL function, control never reaches the restore;exception_handling_estateremains pointing at that function’s stack-localestateafter its frame is unwound.- On the next transaction (or later in the same backend), calls to
plisql_get_current_exception_*or anything that dereferencesexception_handling_estatecan hit freed stack memory and crash.
plisql_xact_cb(Lines 9515-9548) currently resets the simple‑eval state and resowner but does not clearexception_handling_estate, so this stale pointer can survive a transaction abort or commit.To close this hole, clear the global in the xact callback (for both COMMIT/PREPARE and ABORT paths is fine, COMMIT is harmless):
Proposed fix in
plisql_xact_cbvoid plisql_xact_cb(XactEvent event, void *arg) { @@ - if (event == XACT_EVENT_COMMIT || - event == XACT_EVENT_PARALLEL_COMMIT || - event == XACT_EVENT_PREPARE) + if (event == XACT_EVENT_COMMIT || + event == XACT_EVENT_PARALLEL_COMMIT || + event == XACT_EVENT_PREPARE) { simple_econtext_stack = NULL; @@ shared_simple_eval_resowner = NULL; } else if (event == XACT_EVENT_ABORT || event == XACT_EVENT_PARALLEL_ABORT) { simple_econtext_stack = NULL; shared_simple_eval_estate = NULL; shared_simple_eval_resowner = NULL; } + + /* + * Any unhandled ERROR that escaped PL/iSQL will have ended the current + * transaction; no execstate from the old transaction is valid anymore. + * Ensure we don't leave a dangling pointer for the exported helpers. + */ + exception_handling_estate = NULL; }Also applies to: 1988-2153, 9515-9548
🧹 Nitpick comments (3)
design/dbms_utility/PROPOSAL.md (1)
15-18: Optional: Add language specifiers to fenced code blocks.For better syntax highlighting and readability, consider adding language identifiers to the fenced code blocks.
🔎 Proposed fix
At line 15:
-``` +```text src/pl/plisql/src/ → plisql.so (PL/iSQL language runtime) contrib/ivorysql_ora/ → ivorysql_ora.so (Oracle compatibility extension)At line 63: ```diff -``` +```text contrib/ivorysql_ora/ ├── src/builtin_packages/Also applies to: 63-73
contrib/ivorysql_ora/sql/dbms_utility.sql (1)
346-374: Consider more robust call stack depth verification.The string counting logic to verify function entries is fragile:
v_count := (LENGTH(v_stack) - LENGTH(REPLACE(v_stack, 'function ', ''))) / 9;This assumes 'function ' appears consistently and is exactly 9 characters. If the output format changes (e.g., capitalization, spacing), this test will break.
🔎 More robust alternative
Consider using a pattern-based approach or simply checking for the presence of expected function names rather than counting occurrences:
CREATE OR REPLACE PROCEDURE test_call_stack_level3 AS v_stack VARCHAR2(4000); - v_count INTEGER; BEGIN v_stack := DBMS_UTILITY.FORMAT_CALL_STACK; - -- Count the number of function entries (look for 'function ' pattern) - v_count := (LENGTH(v_stack) - LENGTH(REPLACE(v_stack, 'function ', ''))) / 9; - RAISE INFO 'Call stack has % function entries', v_count; + -- Verify expected functions are present in the stack + IF v_stack LIKE '%TEST_CALL_STACK_LEVEL3%' AND + v_stack LIKE '%TEST_CALL_STACK_LEVEL2%' AND + v_stack LIKE '%TEST_CALL_STACK_LEVEL1%' THEN + RAISE INFO 'Call stack contains all expected functions'; + END IF; END; /src/pl/plisql/src/plisql.h (1)
1464-1471: Add documentation to header declarations clarifying memory ownership.The new API has inconsistent return patterns:
plisql_get_current_exception_context(),plisql_get_current_exception_message()returnconst char*(borrowed, managed by PL/iSQL)plisql_get_call_stack()returnschar*(palloc'd, caller must pfree)While the implementations in
pl_exec.cdocument this behavior and callers properly manage memory, the header declarations lack this clarity. Add documentation comments to the extern declarations inplisql.h(lines 1464-1471) to establish the memory ownership contract at the API boundary, e.g.:/* Returns borrowed reference - do not free */ extern PGDLLEXPORT const char *plisql_get_current_exception_context(void); /* Returns newly allocated string - caller must pfree */ extern PGDLLEXPORT char *plisql_get_call_stack(void);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
contrib/ivorysql_ora/expected/dbms_utility.outis excluded by!**/*.out
📒 Files selected for processing (11)
contrib/ivorysql_ora/Makefilecontrib/ivorysql_ora/ivorysql_ora_merge_sqlscontrib/ivorysql_ora/meson.buildcontrib/ivorysql_ora/sql/dbms_utility.sqlcontrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility--1.0.sqlcontrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.cdesign/dbms_utility/PROPOSAL.mdsrc/bin/initdb/initdb.csrc/pl/plisql/src/Makefilesrc/pl/plisql/src/pl_exec.csrc/pl/plisql/src/plisql.h
🚧 Files skipped from review as they are similar to previous changes (3)
- src/bin/initdb/initdb.c
- src/pl/plisql/src/Makefile
- contrib/ivorysql_ora/Makefile
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: rophy
Repo: IvorySQL/IvorySQL PR: 998
File: contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c:82-146
Timestamp: 2025-12-03T06:50:27.755Z
Learning: In contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c, the behavior where ENABLE() clears the existing buffer (rather than preserving it like Oracle does) is an intentional known difference tracked in issue #26 at https://github.com/rophy/IvorySQL/issues/26 (medium impact, open).
Learnt from: rophy
Repo: IvorySQL/IvorySQL PR: 998
File: contrib/ivorysql_ora/sql/ora_dbms_output.sql:211-212
Timestamp: 2025-12-26T07:43:20.955Z
Learning: In contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c (lines 447-449), buffer_size values below 2000 are silently clamped to DBMS_OUTPUT_MIN_BUFFER_SIZE (2000), matching Oracle behavior. This is not an error condition.
📚 Learning: 2025-12-03T06:50:27.755Z
Learnt from: rophy
Repo: IvorySQL/IvorySQL PR: 998
File: contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c:82-146
Timestamp: 2025-12-03T06:50:27.755Z
Learning: In contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c, the behavior where ENABLE() clears the existing buffer (rather than preserving it like Oracle does) is an intentional known difference tracked in issue #26 at https://github.com/rophy/IvorySQL/issues/26 (medium impact, open).
Applied to files:
contrib/ivorysql_ora/ivorysql_ora_merge_sqlscontrib/ivorysql_ora/sql/dbms_utility.sqldesign/dbms_utility/PROPOSAL.mdcontrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.ccontrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility--1.0.sqlcontrib/ivorysql_ora/meson.build
📚 Learning: 2025-12-26T07:43:20.955Z
Learnt from: rophy
Repo: IvorySQL/IvorySQL PR: 998
File: contrib/ivorysql_ora/sql/ora_dbms_output.sql:211-212
Timestamp: 2025-12-26T07:43:20.955Z
Learning: In contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c (lines 447-449), buffer_size values below 2000 are silently clamped to DBMS_OUTPUT_MIN_BUFFER_SIZE (2000), matching Oracle behavior. This is not an error condition.
Applied to files:
contrib/ivorysql_ora/ivorysql_ora_merge_sqlscontrib/ivorysql_ora/sql/dbms_utility.sqldesign/dbms_utility/PROPOSAL.mdcontrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.ccontrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility--1.0.sqlcontrib/ivorysql_ora/meson.build
🧬 Code graph analysis (2)
contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.c (3)
src/backend/utils/mmgr/mcxt.c (3)
pnstrdup(1713-1725)pfree(1527-1541)pstrdup(1702-1706)src/common/stringinfo.c (2)
appendStringInfo(144-166)initStringInfo(96-100)src/backend/utils/adt/varlena.c (1)
cstring_to_text(193-197)
src/pl/plisql/src/plisql.h (1)
src/pl/plisql/src/pl_exec.c (4)
plisql_get_current_exception_context(10074-10081)plisql_get_current_exception_message(10089-10096)plisql_get_current_exception_sqlerrcode(10104-10111)plisql_get_call_stack(10123-10166)
🪛 markdownlint-cli2 (0.18.1)
design/dbms_utility/PROPOSAL.md
15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
63-63: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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: oracle_regression (ubuntu-latest)
- GitHub Check: meson_build (ubuntu-latest)
- GitHub Check: pg_regression (ubuntu-latest)
- GitHub Check: oracle_pg_regression (ubuntu-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: contrib_regression
🔇 Additional comments (7)
contrib/ivorysql_ora/ivorysql_ora_merge_sqls (1)
5-5: LGTM!The addition correctly registers the new DBMS_UTILITY package in the merge scope, following the established pattern.
contrib/ivorysql_ora/sql/dbms_utility.sql (1)
1-441: Excellent test coverage!The test suite comprehensively covers the three DBMS_UTILITY functions across diverse scenarios including nested calls, exception handling, packages, anonymous blocks, and schema-qualified calls. The use of pattern matching for validation (rather than exact string matches) ensures test stability across minor output format changes.
contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility--1.0.sql (1)
1-48: LGTM!The package definition correctly follows IvorySQL conventions:
- C wrappers placed in
sysschema withora_prefix- Marked as
VOLATILE(appropriate for runtime state access)- Package specification and body properly delegate to C implementations
- Consistent with existing built-in packages like DBMS_OUTPUT
contrib/ivorysql_ora/meson.build (1)
29-29: Add dbms_utility to the registered SQL tests in meson.build.The C source file is correctly added at line 29. However,
sql/dbms_utility.sqland its expected output exist but are not registered in thetestssection. This test needs to be explicitly added to the 'sql' array (lines 67-70) to ensure it runs during test execution.src/pl/plisql/src/pl_exec.c (1)
10064-10166: Accessor helpers for current exception and call stack look soundThe new helpers (
plisql_get_current_exception_context/message/sqlerrcodeandplisql_get_call_stack) are minimal and correctly:
- Guard on
exception_handling_estateandcur_errorbeing non‑NULL, returning NULL/0 otherwise.- Avoid copying data, returning pointers owned by
ErrorData/PL/iSQL (matching comment contracts).- Walk
error_context_stackfiltering onplisql_exec_error_callback, so only PL/iSQL frames are emitted and format is stable for dbms_utility.No functional issues spotted here given the current design of the exception context tracking.
contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.c (2)
116-214: LGTM!The line transformation logic is well-structured with proper memory management. All allocated strings are correctly freed before returning. The special handling for anonymous blocks and the uppercase conversion for Oracle compatibility are appropriate.
275-300: LGTM!The SQLSTATE to Oracle error number mapping is clean and handles common error cases appropriately. The use of string literals avoids memory management concerns.
Implement Oracle-compatible DBMS_UTILITY package in PL/iSQL extension: - Add FORMAT_ERROR_BACKTRACE function that returns PL/iSQL call stack during exception handling, matching Oracle's behavior - Implement pl_dbms_utility.c with stack trace formatting logic - Use per-estate storage to fix nested handler context corruption - Add regression tests including nested exception handler scenarios
Implement two additional Oracle-compatible functions in the DBMS_UTILITY package: - FORMAT_ERROR_STACK: Returns the error message with Oracle error code (e.g., ORA-01476 for division by zero, ORA-06510 for user exceptions) - FORMAT_CALL_STACK: Returns the current PL/iSQL call stack in Oracle format with header and function entries New PL/iSQL API functions in pl_exec.c: - plisql_get_current_exception_message() - returns error message - plisql_get_current_exception_sqlerrcode() - returns SQLSTATE code - plisql_get_call_stack() - traverses error_context_stack for call stack All three DBMS_UTILITY functions now work correctly: - Return appropriate values when in exception handler - Return NULL when not in exception context (except CALL_STACK) - Output matches Oracle format conventions
67f14dd to
802c1bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/pl/plisql/src/pl_exec.c (1)
118-123: Clearexception_handling_estateat transaction end to avoid dangling estate pointer after unhandled errors
exception_handling_estateis a static global that can remain pointing at a stack-localPLiSQL_execstateif anERRORescapes an exception handler before the restore path runs (e.g., error insideexception->action). That estate and its memory are then torn down by error abort, but:
plisql_xact_cbdoes not currently resetexception_handling_estate.- Later calls to
plisql_get_current_exception_*/plisql_get_call_stackoutside an active handler can see a non‑NULLexception_handling_estateand dereference freed memory (estate->cur_error,estate->func, etc.), leading to backend crashes.This is exactly the lifetime hole previously flagged; it still needs a concrete guard at transaction end.
To make the global safe across transactions (commit, abort, prepare), clear it in
plisql_xact_cbunconditionally at the end of the callback:Proposed fix in
plisql_xact_cbvoid plisql_xact_cb(XactEvent event, void *arg) { @@ - if (event == XACT_EVENT_COMMIT || - event == XACT_EVENT_PARALLEL_COMMIT || - event == XACT_EVENT_PREPARE) + if (event == XACT_EVENT_COMMIT || + event == XACT_EVENT_PARALLEL_COMMIT || + event == XACT_EVENT_PREPARE) { simple_econtext_stack = NULL; @@ shared_simple_eval_estate = NULL; shared_simple_eval_resowner = NULL; } + + /* + * Any unhandled ERROR that escaped a PL/iSQL exception handler will + * have ended or aborted the current transaction; no saved estate from + * the old transaction is valid past this point. + */ + exception_handling_estate = NULL; }This preserves the fast-path behavior while ensuring the exported helpers can never see a stale
PLiSQL_execstateacross transaction boundaries.Also applies to: 9515-9548
contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.c (3)
249-268: Memory leak already identified in prior review.The
result.databuffer leak at line 267 was flagged in a previous review. Please apply the suggested fix.
335-341: Memory leak already identified in prior review.The
result.databuffer leak at line 340 was flagged in a previous review. Please apply the suggested fix.
455-466: Memory leak already identified in prior review.The
result.databuffer leak at line 465 was flagged in a previous review. Please apply the suggested fix to match the cleanup pattern at line 461.
🧹 Nitpick comments (1)
contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.c (1)
190-192: TODO: Schema lookup is placeholder.The hardcoded
"PUBLIC"schema name doesn't match Oracle's behavior where actual schema names are displayed. This could cause confusion when debugging cross-schema procedure calls. Consider tracking this as a follow-up issue if accurate schema resolution is needed for Oracle compatibility.Would you like me to open an issue to track implementing proper schema lookup from
pg_proc?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
contrib/ivorysql_ora/expected/dbms_utility.outis excluded by!**/*.out
📒 Files selected for processing (9)
contrib/ivorysql_ora/Makefilecontrib/ivorysql_ora/ivorysql_ora_merge_sqlscontrib/ivorysql_ora/meson.buildcontrib/ivorysql_ora/sql/dbms_utility.sqlcontrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility--1.0.sqlcontrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.csrc/bin/initdb/initdb.csrc/pl/plisql/src/pl_exec.csrc/pl/plisql/src/plisql.h
🚧 Files skipped from review as they are similar to previous changes (5)
- src/pl/plisql/src/plisql.h
- src/bin/initdb/initdb.c
- contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility--1.0.sql
- contrib/ivorysql_ora/meson.build
- contrib/ivorysql_ora/ivorysql_ora_merge_sqls
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: rophy
Repo: IvorySQL/IvorySQL PR: 998
File: contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c:82-146
Timestamp: 2025-12-03T06:50:27.755Z
Learning: In contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c, the behavior where ENABLE() clears the existing buffer (rather than preserving it like Oracle does) is an intentional known difference tracked in issue #26 at https://github.com/rophy/IvorySQL/issues/26 (medium impact, open).
Learnt from: rophy
Repo: IvorySQL/IvorySQL PR: 998
File: contrib/ivorysql_ora/sql/ora_dbms_output.sql:211-212
Timestamp: 2025-12-26T07:43:20.955Z
Learning: In contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c (lines 447-449), buffer_size values below 2000 are silently clamped to DBMS_OUTPUT_MIN_BUFFER_SIZE (2000), matching Oracle behavior. This is not an error condition.
📚 Learning: 2025-12-26T07:43:20.955Z
Learnt from: rophy
Repo: IvorySQL/IvorySQL PR: 998
File: contrib/ivorysql_ora/sql/ora_dbms_output.sql:211-212
Timestamp: 2025-12-26T07:43:20.955Z
Learning: In contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c (lines 447-449), buffer_size values below 2000 are silently clamped to DBMS_OUTPUT_MIN_BUFFER_SIZE (2000), matching Oracle behavior. This is not an error condition.
Applied to files:
contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.ccontrib/ivorysql_ora/sql/dbms_utility.sql
📚 Learning: 2025-12-03T06:50:27.755Z
Learnt from: rophy
Repo: IvorySQL/IvorySQL PR: 998
File: contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c:82-146
Timestamp: 2025-12-03T06:50:27.755Z
Learning: In contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c, the behavior where ENABLE() clears the existing buffer (rather than preserving it like Oracle does) is an intentional known difference tracked in issue #26 at https://github.com/rophy/IvorySQL/issues/26 (medium impact, open).
Applied to files:
contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.ccontrib/ivorysql_ora/Makefilecontrib/ivorysql_ora/sql/dbms_utility.sql
📚 Learning: 2025-09-02T10:43:25.257Z
Learnt from: syasirs
Repo: IvorySQL/IvorySQL PR: 858
File: src/Makefile.global.in:513-514
Timestamp: 2025-09-02T10:43:25.257Z
Learning: In IvorySQL Makefile.global.in, $(wildcard t/oracle/*.pl) is used instead of t/oracle/*.pl in Oracle TAP test discovery because the t/oracle directory is optional and may not exist in most builds. The wildcard function safely returns empty string when the directory is missing, preventing shell expansion errors.
Applied to files:
contrib/ivorysql_ora/Makefile
🧬 Code graph analysis (1)
contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.c (3)
src/backend/utils/mmgr/mcxt.c (3)
pnstrdup(1713-1725)pfree(1527-1541)pstrdup(1702-1706)src/common/stringinfo.c (2)
appendStringInfo(144-166)initStringInfo(96-100)src/backend/utils/adt/varlena.c (1)
cstring_to_text(193-197)
⏰ 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: build (ubuntu-latest)
- GitHub Check: oracle_regression (ubuntu-latest)
- GitHub Check: pg_regression (ubuntu-latest)
- GitHub Check: meson_build (ubuntu-latest)
- GitHub Check: contrib_regression
- GitHub Check: oracle_pg_regression (ubuntu-latest)
🔇 Additional comments (15)
src/pl/plisql/src/pl_exec.c (2)
1978-2172: Exception handler save/restore ofexception_handling_estatelooks correct for nested handlersThe new logic in
exec_stmt_block’sPG_CATCH:
- Saves the previous
exception_handling_estatebefore exception handling (save_exception_handling_estate).- Sets
estate->cur_errorand thenexception_handling_estate = estateimmediately beforeexec_stmts(estate, exception->action), so any code (e.g., DBMS_UTILITY) called inside the handler can locate the right estate, including nested PL/iSQL calls.- Restores
exception_handling_estatetosave_exception_handling_estateimmediately after the handler completes normally, so outer handlers (or no handler) are visible again.- Leaves the pointer unchanged when no handler matches, correctly preserving prior state before
ReThrowError(edata).Within a single transaction and for nested blocks/functions, this pattern yields the expected “innermost active handler” semantics for the new introspection APIs. Once the transaction-end cleanup for
exception_handling_estateis added (see previous comment), the overall design is sound.
10065-10166: New exception/context and call-stack accessors are functionally sound; ensure callers treat NULL/0 as “no active handler”The four helpers:
plisql_get_current_exception_context()/plisql_get_current_exception_message()return the current handler’sErrorDatafields whenexception_handling_estateandcur_errorare set, elseNULL.plisql_get_current_exception_sqlerrcode()returnssqlerrcodeor0when not in a handler.plisql_get_call_stack()walkserror_context_stack, filtering onplisql_exec_error_callback, and emits one tab‑separated line perPLiSQL_execstatewith(func pointer, lineno, fn_signature). It correctly derives line numbers fromerr_stmt/err_varand returnsNULLwhen no PL/iSQL frames are present.Memory/lifetime considerations are reasonable (strings live in the caller’s current context,
cur_errorfields live in the statement mcontext while the handler runs). With the transaction‑end clearing ofexception_handling_estate, these APIs are safe; call sites should simply interpretNULL/0as “no active PL/iSQL exception context/stack”.No changes needed here.
contrib/ivorysql_ora/Makefile (3)
30-31: LGTM!The new
dbms_utility.oobject is correctly added following the existing directory structure pattern for builtin packages.
73-74: LGTM!The
dbms_utilityregression test is correctly added toORA_REGRESS.
78-79: LGTM!The include path for
plisql.his correctly added to enable the dbms_utility module to access PL/iSQL APIs.contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.c (3)
1-41: LGTM on the includes and declarations.The conditional include for
<dlfcn.h>and function declarations are correctly structured. The Windows limitation is already noted in prior review comments.
56-60: Thread-safety consideration.The cached function pointers and
lookup_attemptedflag are static variables accessed without synchronization. This is acceptable in PostgreSQL's process-per-connection model where backends are single-threaded, but worth noting for documentation purposes.
275-300: LGTM on the SQLSTATE to ORA mapping.The mapping covers common error cases. The generic fallback to
ORA-06502is reasonable. Additional mappings can be added incrementally as compatibility needs are identified.contrib/ivorysql_ora/sql/dbms_utility.sql (7)
5-94: LGTM on FORMAT_ERROR_BACKTRACE basic and nested tests.Tests 1-3 provide good coverage of basic exceptions, nested procedure calls, and deeply nested calls (5 levels). Each test properly cleans up created objects.
95-144: LGTM on function and anonymous block tests.Tests 4-6 properly cover function calls, anonymous blocks, and the no-exception case (verifying NULL/empty return).
146-209: LGTM on re-raise and package tests.Test 7 covers the important case of exceptions propagating through multiple handlers with re-raise. Test 8 verifies package procedure support.
211-270: LGTM on schema-qualified and nested handler tests.Test 9 verifies schema-qualified procedure calls. Test 10 is particularly valuable as it validates that the outer exception handler's backtrace context is preserved after calling a procedure with its own exception handler.
272-320: LGTM on FORMAT_ERROR_STACK tests.Tests 11-13 cover user-defined exceptions, built-in errors (division by zero), and the no-exception case. Good coverage for verifying the ORA-style error message formatting.
322-392: LGTM on FORMAT_CALL_STACK tests.Tests 14-16 appropriately verify call stack structure using pattern matching (avoiding brittle assertions on memory addresses). The technique at line 353 to count function entries using string replacement is clever.
394-441: LGTM on integration test.Test 17 is an excellent integration test that verifies all three DBMS_UTILITY functions work together in a single exception handler context. The pattern-based assertions appropriately verify Oracle-compatible output format.
Summary
DBMS_UTILITYpackage with three functions:FORMAT_ERROR_BACKTRACE- returns exception location stack in ORA-06512 formatFORMAT_ERROR_STACK- returns exception message with Oracle error codeFORMAT_CALL_STACK- returns current PL/iSQL call stackcontrib/ivorysql_ora/following upstream conventionTest plan
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.