feat: DBMS_UTILITY.FORMAT_ERROR_BACKTRACE #20
Conversation
WalkthroughAdds an Oracle-compatible DBMS_UTILITY package with FORMAT_ERROR_BACKTRACE, implements session-level PL/iSQL exception-context storage and a public accessor, provides a C wrapper that formats stored PostgreSQL-style error contexts into Oracle-style ORA-06512 backtraces, updates initdb to enable Oracle compatibility while installing PL/iSQL, and adds tests, expected outputs, and design docs. Changes
Sequence DiagramsequenceDiagram
autonumber
participant User
participant PLiSQL as "PL/SQL engine"
participant pl_exec as "pl_exec.c (estate)"
participant Session as "session/state storage"
participant DBMS_UTIL as "pl_dbms_utility.c"
User->>PLiSQL: Execute block that RAISEs
PLiSQL->>pl_exec: enter function / exec_stmt_block
pl_exec->>pl_exec: PG_CATCH -> capture edata->context
pl_exec->>Session: store current_exception_context in estate
pl_exec->>pl_exec: set exception_handling_estate / restore estates
User->>PLiSQL: CALL dbms_utility.FORMAT_ERROR_BACKTRACE()
PLiSQL->>DBMS_UTIL: call sys.ora_format_error_backtrace()
DBMS_UTIL->>Session: call plisql_get_current_exception_context()
Session-->>DBMS_UTIL: return context string or NULL
DBMS_UTIL->>DBMS_UTIL: transform lines -> ORA-06512 format
DBMS_UTIL-->>PLiSQL: return TEXT backtrace (or NULL)
PLiSQL-->>User: deliver result
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)
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 (3)
design/dbms_utility/ARCHITECTURE.md (1)
1-174: Architecture doc is clear and matches implementation; consider noting semantics/limits of FORMAT_ERROR_BACKTRACE.The description of session-level exception context in
pl_exec.c, the C wrapper, and the package definition matches the current code and gives future contributors a good map. You might optionally add a short note that:
DBMS_UTILITY.FORMAT_ERROR_BACKTRACEreturnsNULLwhen no exception is currently being handled (per Oracle docs), and- behavior with deeply nested exception handlers is “best effort” Oracle compatibility, so any intentional deviations or edge cases (if you find them in testing) are documented here for users.
This keeps expectations explicit without changing the design.
src/pl/plisql/src/pl_dbms_utility.c (1)
128-150: Schema always hardcoded to "PUBLIC" - verify this meets Oracle compatibility requirements.The TODO at line 129 acknowledges the limitation. For schema-qualified procedures (like Test 9 in dbms_utility.sql), the backtrace will show "PUBLIC.SCHEMA_ERROR" instead of "TEST_SCHEMA.SCHEMA_ERROR".
Also, the null check at line 149 is redundant since
schema_nameis unconditionally assigned at line 130.- if (schema_name) - pfree(schema_name); + pfree(schema_name);src/pl/plisql/src/sql/dbms_utility.sql (1)
1-233: Test coverage is comprehensive for core functionality.The test suite covers the key scenarios well: basic exceptions, nested calls, deep nesting, functions, anonymous blocks, no-error case, re-raise, packages, and schema-qualified calls.
Consider adding these edge cases in future iterations:
- Procedure/function names containing special characters or mixed case
- Very long backtrace chains (stress test)
- Concurrent exception handling scenarios
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
design/dbms_utility/ARCHITECTURE.md(1 hunks)design/dbms_utility/DEPENDENCY_ANALYSIS.md(1 hunks)design/dbms_utility/EXISTING_PACKAGES.md(1 hunks)design/dbms_utility/README.md(1 hunks)src/bin/initdb/initdb.c(1 hunks)src/pl/plisql/src/Makefile(2 hunks)src/pl/plisql/src/expected/dbms_utility.out(1 hunks)src/pl/plisql/src/pl_dbms_utility.c(1 hunks)src/pl/plisql/src/pl_exec.c(4 hunks)src/pl/plisql/src/plisql--1.0.sql(1 hunks)src/pl/plisql/src/plisql.h(1 hunks)src/pl/plisql/src/sql/dbms_utility.sql(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/expected/*.out
⚙️ CodeRabbit configuration file
**/expected/*.out: These are expected test outputs from pg_regress. Review for:
- Correctness of query results
- Proper Oracle compatibility behavior
- Edge case coverage
- Consistency with corresponding .sql test files
Files:
src/pl/plisql/src/expected/dbms_utility.out
**/sql/*.sql
⚙️ CodeRabbit configuration file
**/sql/*.sql: Test SQL files. Ensure comprehensive coverage of features.
Files:
src/pl/plisql/src/sql/dbms_utility.sql
🔇 Additional comments (20)
src/pl/plisql/src/Makefile (2)
43-43: LGTM - Build integration looks correct.The addition of
pl_dbms_utility.oto the OBJS list properly links the new DBMS_UTILITY implementation into the plisql shared library.
60-61: LGTM - Test suite integration is correct.The addition of
dbms_utilityto the REGRESS test list ensures the new functionality is covered by the regression test suite.src/bin/initdb/initdb.c (1)
2094-2097: Oracle compatibility mode switch for PACKAGE syntax - verify necessity.This change temporarily switches to Oracle compatibility mode during
CREATE EXTENSION plisqlto allow theCREATE PACKAGEsyntax used inplisql--1.0.sqlfor the new DBMS_UTILITY package.The pattern is:
- Switch to Oracle mode
- Load extension (which creates packages)
- Switch back to PostgreSQL mode
This approach isolates the Oracle-mode behavior to just the extension load phase. However, please confirm:
- Is this mode switch necessary for all plisql extension loads, or only when DBMS_UTILITY is present?
- Does this change affect existing plisql extension upgrades or installations?
- Should this behavior be documented in the extension's README or upgrade notes?
src/pl/plisql/src/plisql.h (1)
1459-1462: LGTM - Clean public API for exception context access.The new
plisql_get_current_exception_context()function declaration provides a clean, read-only interface for accessing the current PL/iSQL exception context. ThePGDLLEXPORTmarking properly exports this symbol from the plisql shared library, and theconst char*return type ensures callers cannot modify the exception context.design/dbms_utility/README.md (1)
1-58: LGTM - Excellent design documentation.This README provides clear, well-structured documentation of the DBMS_UTILITY implementation. The documentation:
- Clearly identifies the implementation location and rationale
- Documents the key design decisions (location, exception context storage, output format)
- Provides appropriate cross-references to detailed design documents
- Tracks implementation status
The documentation will help future maintainers understand the architectural decisions and implementation approach.
design/dbms_utility/EXISTING_PACKAGES.md (1)
1-56: LGTM - Important context documentation.This document provides valuable context by:
- Clarifying that DBMS_UTILITY is IvorySQL's first built-in Oracle DBMS package
- Comparing with Oracle's extensive package ecosystem
- Establishing the architectural pattern for future package implementations (location based on dependencies)
This context helps reviewers understand the significance and precedent-setting nature of this implementation.
design/dbms_utility/DEPENDENCY_ANALYSIS.md (1)
1-144: LGTM - Thorough architectural analysis.This document provides excellent architectural documentation:
- Clearly identifies the cross-module dependency problem
- Explains why avoiding cross-module dependencies is important (layering, encapsulation, maintenance, compatibility)
- Details the session-level storage solution with code examples
- Provides clear data flow diagram
- Documents alternatives considered and why they weren't chosen
This level of architectural documentation will be invaluable for future maintainers and for similar design decisions in other Oracle compatibility features.
src/pl/plisql/src/expected/dbms_utility.out (1)
1-227: LGTM - Comprehensive test coverage.The test suite provides excellent coverage of FORMAT_ERROR_BACKTRACE functionality:
Test scenarios covered:
- ✅ Basic exception in procedure
- ✅ Nested procedure calls (3 levels)
- ✅ Deep nesting (5 levels)
- ✅ Function calls
- ✅ Anonymous blocks
- ✅ No exception case (returns
<NULL>)- ✅ Multiple exception levels with re-raise
- ✅ Package procedures
- ✅ Schema-qualified calls
Oracle format validation:
- Proper
ORA-06512prefix- Schema-qualified names in quotes:
"PUBLIC.PROCEDURE_NAME"- Line number reporting
- Anonymous block format:
at line NThe test coverage is thorough and validates both normal and edge cases.
As per coding guidelines, expected test outputs should align with corresponding .sql test files and demonstrate proper Oracle compatibility behavior.
src/pl/plisql/src/plisql--1.0.sql (1)
25-52: LGTM - Clean Oracle-compatible package definition.The SQL package definition is well-structured:
C Function Wrapper (lines 33-35):
- Properly declared with
MODULE_PATHNAMEfor dynamic loading- Marked
VOLATILE(correct - depends on exception context state)- Marked
STRICT(correct - should not be called with NULL)- Placed in
sysschema (appropriate for internal function)Package Definition (lines 43-52):
- Package spec declares
FORMAT_ERROR_BACKTRACEreturningTEXT- Package body delegates to
sys.ora_format_error_backtrace()- Provides clean Oracle-compatible public API
- Comments explain the purpose and internal nature
The layering is clean: public
dbms_utilitypackage → internalsysfunction → C implementation.src/pl/plisql/src/pl_dbms_utility.c (2)
29-44: LGTM on includes and declaration.The includes are appropriate. Some catalog headers (pg_proc.h, pg_namespace.h, lsyscache.h, syscache.h) appear unused currently but are likely staged for the TODO at line 129 to look up actual schema from pg_proc catalog.
165-203: Implementation looks correct.The function properly:
- Returns NULL when no exception context exists
- Uses
strtok_rsafely with a copy of the input- Cleans up
context_copyafter parsingNote: If all lines are filtered out (none transformed), this returns an empty string rather than NULL. Verify this matches Oracle's behavior for edge cases.
src/pl/plisql/src/sql/dbms_utility.sql (9)
5-19: Good basic test case with proper setup and cleanup.
21-49: Good test for nested call backtrace.Tests the core use case of capturing a stack trace through multiple procedure levels.
51-93: Good depth test.5-level nesting validates that the backtrace formatter handles longer call stacks correctly.
95-118: Good test for function-originated exceptions.Ensures backtrace works correctly when the error originates from a function rather than a procedure.
120-131: Good anonymous block test.Tests the
inline_code_blockcode path intransform_and_append_linewhich produces "ORA-06512: at line X" format.
133-144: Good negative test case.Validates that
FORMAT_ERROR_BACKTRACEreturns NULL when called outside an exception handler context.
146-180: Good re-raise test.Tests that the backtrace is properly preserved/updated when an exception is caught and re-raised.
182-209: Good package context test.Tests backtrace within Oracle-style packages, which is a key use case for DBMS_UTILITY.
211-233: Schema-qualified test is valuable but output may not match Oracle behavior.Due to the hardcoded "PUBLIC" schema in
transform_and_append_line(line 130 of pl_dbms_utility.c), this test will output "PUBLIC.SCHEMA_ERROR" instead of "TEST_SCHEMA.SCHEMA_ERROR". Verify that the expected output file (dbms_utility.out) reflects this limitation and consider adding a comment in the test noting this known limitation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/pl/plisql/src/pl_exec.c (1)
1964-1965: Exception context tracking still has stale‑pointer and nesting bugs; needs tighter lifecycle managementTwo related problems around
exception_handling_estate/current_exception_context:
Nested handlers lose the outer error’s backtrace
- In the
exec_stmt_blockPG_CATCH, on a matching exception you:
- Copy
edata->contextintoestate->current_exception_contextand setexception_handling_estate = estate(Lines 2114–2131, 2134–2140).- After running the handler body, you restore
estate->cur_error = save_cur_errorand then unconditionallypfree(estate->current_exception_context); estate->current_exception_context = NULL;(Lines 2159–2167).- Scenario: inside a handler for error A, a nested block raises and handles error B.
- Inner PG_CATCH overwrites and then clears
estate->current_exception_context.- On exit from the inner handler,
estate->cur_erroris restored to A, andexception_handling_estateis restored to the outer estate, butcurrent_exception_contextstays NULL.- Subsequent calls to
plisql_get_current_exception_context()from the outer handler (or callees) see NULL even thoughestate->cur_errorstill refers to A and we are still “in an exception handler”. This diverges from the intended “current exception being handled” semantics and from whatGET STACKED DIAGNOSTICS ... ERROR_CONTEXTexposes.Error escaping a handler can leave
exception_handling_estatepointing at freed stack memory
- In the same PG_CATCH, you set
exception_handling_estate = estatebeforeexec_stmts(estate, exception->action)and restore it afterwards (Lines 2134–2140, 2143–2147).- If
exec_stmtsinside the handler body raises a new ERROR that is not caught by an inner PL/iSQL block (e.g.,RAISEor some uncaught ERROR inside the handler), control jumps out of this PG_CATCH before the restoration and before thecurrent_exception_contextclear block runs.estateis stack‑allocated inplisql_exec_function/ trigger / event‑trigger. After the ERROR unwinds and the function returns via longjmp,exception_handling_estatestill points at that deadPLiSQL_execstate.
plisql_xact_cb()(Lines 9444–9475) currently only resets the simple expression state and resowner; it does not null outexception_handling_estate(oractive_estate).
A later call toplisql_get_current_exception_context()(Lines 9993–10014) in a new transaction can then dereference a dangling pointer (exception_handling_estate->current_exception_context), which is undefined behaviour and can crash the backend.
This is essentially the same class of “stale/leaked backtrace across abort” issue that was raised against the previous global‑string design.Suggested fixes
A. Preserve outer handler’s context across nested handlers
Track the previous context string per
exec_stmt_block, and restore it when leaving the handler, instead of just zeroing it:
- Extend the local saves in the EXCEPTION path:
- ErrorData *save_cur_error = estate->cur_error; - PLiSQL_execstate *save_exception_handling_estate = exception_handling_estate; + ErrorData *save_cur_error = estate->cur_error; + PLiSQL_execstate *save_exception_handling_estate = exception_handling_estate; + char *save_current_exception_context = estate->current_exception_context;
- Replace the clear block at the end of the PG_CATCH with a restore‑and‑free of just this block’s context:
- /* - * Clear the exception context now that we've finished - * handling the exception. - */ - if (estate->current_exception_context) - { - pfree(estate->current_exception_context); - estate->current_exception_context = NULL; - } + /* + * We're leaving this handler: drop the context string we + * allocated for this error and restore the previous handler's + * context (if any). + */ + if (estate->current_exception_context) + pfree(estate->current_exception_context); + estate->current_exception_context = save_current_exception_context;That way:
- Inner handler B allocates and frees its own copy of
edata->context.- After B finishes,
estate->cur_errorandestate->current_exception_contextare both restored to A.- The outer handler’s context is only freed once, when its own EXCEPTION block finishes.
B. Clear global estate pointers on transaction abort to avoid dangling references
In
plisql_xact_cb, make sure abort paths drop the new globals: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; + + /* Avoid dangling pointers into stack-allocated execstates after ERROR */ + active_estate = NULL; + exception_handling_estate = NULL; }(Optionally, you can also null them in the COMMIT/PREPARE branch for hygiene, but the abort case is the one that prevents UAF.)
C. Consider simplifying
plisql_get_current_exception_contextto usecur_errordirectlyGiven you already maintain a proper stack of
estate->cur_erroracross nested handlers, andexec_stmt_getdiaguses that to exposeERROR_CONTEXT, you could avoid copying/freeing strings entirely and just do:const char * plisql_get_current_exception_context(void) { if (exception_handling_estate != NULL && exception_handling_estate->cur_error != NULL) return exception_handling_estate->cur_error->context; return NULL; }This:
- Automatically tracks the same nesting semantics as
GET STACKED DIAGNOSTICS.- Removes the need for the
current_exception_contextfield and its manual MemoryContextStrdup/pfree lifecycle.- Eliminates another potential source of leaks or stale pointers if future changes forget to update the clear logic.
Also applies to: 2114-2147, 2159-2167, 4364-4367, 9444-9475, 9993-10014
🧹 Nitpick comments (7)
src/pl/plisql/src/plisql.h (2)
1160-1165: Per-estate exception context field looks correct; consider tightening type toconst char *.The new
current_exception_contextonPLiSQL_execstateis a sensible place to hang per-execution backtrace state for DBMS_UTILITY, and adding it near other error-context fields is consistent.Given the exported getter returns
const char *, you might consider declaring the struct field asconst char *as well to document that callers must treat the buffer as read‑only, while still allowing the executor to reassign the pointer as needed. Not mandatory, but it would align the internal field with the public API and make misuse harder.
1465-1468: Header export forplisql_get_current_exception_contextis reasonable; ensure lifetime contract is documented.The
PGDLLEXPORT const char *plisql_get_current_exception_context(void)declaration matches the intended usage from DBMS_UTILITY and other consumers.It would be good to clearly document (here or in pl_exec.c) that:
- The pointer is either
NULLor points to memory owned by PL/iSQL for the duration of the currently active exception handler.- Callers must not free or cache it beyond the handler’s lifetime.
If that contract is already enforced in pl_exec.c, no functional changes are needed; this is just about making the usage guarantees explicit for external callers of the new API.
src/pl/plisql/src/plisql--1.0.sql (1)
32-56: DBMS_UTILITY wiring and C wrapper look sound.
sys.ora_format_error_backtrace()as aLANGUAGE C VOLATILE STRICTfunction, and thedbms_utilitypackage spec/body delegating straight to it, is a clean and minimal integration. The schema-qualification of thesyshelper also avoids search_path surprises.The only thing to keep in mind operationally is that
CREATE PACKAGErequires Oracle compatibility mode; the comment already calls this out. If not already covered elsewhere, consider documenting for users that installing this extension in a non‑Oracle‑mode database will require setting the appropriate compatibility GUC before running this script.src/pl/plisql/src/expected/dbms_utility.out (1)
177-223: Verify backtrace object naming format for package and schema-qualified procedures against Oracle behavior.In the expected output:
- Package procedures appear as
"PUBLIC.PKG_ERROR"and"PUBLIC.PKG_CALLER"(Test 8).- Schema-qualified procedures appear as
"PUBLIC.TEST_SCHEMA.SCHEMA_ERROR"and"PUBLIC.TEST_SCHEMA.SCHEMA_CALLER"(Test 9).In Oracle, backtrace frames are typically reported as
"OWNER.PROC"for standalone routines and"OWNER.PACKAGE.PROC"(or"OWNER.PACKAGE") for packaged routines, without an extra leading owner segment for schema-qualified standalone procedures.If your goal is strict Oracle compatibility, it’s worth double‑checking that this naming scheme is intentional and matches your documented design for IvorySQL object naming in error stacks. If not, adjusting either the implementation or the expected outputs here would keep FORMAT_ERROR_BACKTRACE closer to Oracle’s conventions.
src/pl/plisql/src/pl_exec.c (1)
116-128: Active/exception estate stacking looks sound; consider documenting or pruning unusedactive_estateThe save/restore pattern for
active_estateandexception_handling_estateinplisql_exec_function,plisql_exec_trigger,plisql_exec_event_trigger, andplisql_estate_setupis correct for nesting (functions calling functions, triggers from PL/iSQL, etc.) and keeps the handler estate separate from the “currently executing” estate.Right now
active_estateis only written (inplisql_estate_setupand the entry/exit save/restore) and not read anywhere in this file. If nothing else in the extension references it, you might either:
- Drop
active_estateentirely, or- Add a small accessor (and a brief comment here) clarifying its intended external use, so future readers don’t wonder why this static exists.
Also applies to: 533-543, 945-949, 1092-1099, 1317-1320, 1339-1346, 1404-1408, 4364-4367
src/pl/plisql/src/sql/dbms_utility.sql (2)
235-270: Add explicit assertion to Test 10's context preservation check.Lines 259–263 log whether outer backtrace is preserved but don't fail the test if it isn't. The comparison should trigger an exception or use
ASSERTto make the test self-validating.IF v_bt_before = v_bt_after THEN RAISE INFO 'SUCCESS: Outer backtrace preserved'; ELSE RAISE INFO 'FAILURE: Outer backtrace changed'; + RAISE EXCEPTION 'Outer backtrace was not preserved after nested exception handler'; END IF;
1-270: Consider adding edge-case tests for error clarity.Current coverage is strong, but consider adding:
- Exception with NULL/empty error message
- Exception in nested transaction contexts (if applicable)
- FORMAT_ERROR_BACKTRACE called multiple times in same exception handler (should return consistent output)
- Very long procedure names or stack traces to test buffer limits
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/pl/plisql/src/expected/dbms_utility.out(1 hunks)src/pl/plisql/src/pl_dbms_utility.c(1 hunks)src/pl/plisql/src/pl_exec.c(12 hunks)src/pl/plisql/src/plisql--1.0.sql(1 hunks)src/pl/plisql/src/plisql.h(2 hunks)src/pl/plisql/src/sql/dbms_utility.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pl/plisql/src/pl_dbms_utility.c
🧰 Additional context used
📓 Path-based instructions (2)
**/expected/*.out
⚙️ CodeRabbit configuration file
**/expected/*.out: These are expected test outputs from pg_regress. Review for:
- Correctness of query results
- Proper Oracle compatibility behavior
- Edge case coverage
- Consistency with corresponding .sql test files
Files:
src/pl/plisql/src/expected/dbms_utility.out
**/sql/*.sql
⚙️ CodeRabbit configuration file
**/sql/*.sql: Test SQL files. Ensure comprehensive coverage of features.
Files:
src/pl/plisql/src/sql/dbms_utility.sql
🧠 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/pl_exec.c
🔇 Additional comments (4)
src/pl/plisql/src/expected/dbms_utility.out (1)
1-265: Test coverage and expected behavior for FORMAT_ERROR_BACKTRACE look comprehensive.The expected output exercises:
- Simple and deeply nested procedure call stacks (Tests 1–3).
- Function callers (Test 4).
- Anonymous blocks with line-only backtraces (Test 5).
- The no-error scenario returning
NULL(Test 6).- Re-raised exceptions across multiple handler levels (Test 7).
- Package procedures (Test 8).
- Schema-qualified procedures (Test 9).
- Preservation of the outer backtrace across an inner handler that catches its own error (Test 10).
The ORA‑06512 formatting, the placement of INFO messages, and the NULL handling (
[<NULL>]) are all consistent with the intended Oracle‑style behavior and with the PL/iSQL usage in the implied.sqltest script. This should give good confidence that the new DBMS_UTILITY plumbing behaves as designed.src/pl/plisql/src/sql/dbms_utility.sql (3)
1-270: Expected output file exists and covers all assertions.The corresponding expected output file (
expected/dbms_utility.out) is present and validates all 10 test scenarios comprehensively, including backtrace format, nested procedure calls, deeply nested chains, function calls, anonymous blocks, null backtraces, multiple exception handlers, package procedures, schema-qualified procedures, and preservation of outer exception context across handler boundaries.
121-131: This hybrid syntax is supported and working correctly.Test 5 successfully mixes PostgreSQL syntax (
DO $$) with Oracle-style PL/iSQL (DECLARE,VARCHAR2, exception handling). The expected output file confirms the test passes, executing the anonymous block and producing the expected backtrace output:INFO: Anonymous block backtrace: ORA-06512: at line 5. No compatibility issues exist.
96-101: PL/iSQL syntax is correct and intentional—no issues identified.The original concerns about
FUNCTION...RETURNsyntax (line 96) and theDO $$block with Oracle syntax (lines 121-131) are based on a misunderstanding of PL/iSQL's design. PL/iSQL is explicitly an Oracle-compatible procedural language that intentionally supports Oracle syntax alongside PostgreSQL syntax. TheFUNCTION...RETURN NUMBERdeclaration and Oracle-style exception handling within PostgreSQL'sDO $$blocks are both correct and intentional in this context.Test coverage is comprehensive, covering 10 scenarios: basic exceptions, nested calls, deep nesting (5 levels), function contexts, anonymous blocks, no-error cases, exception propagation, package procedures, schema-qualified calls, and nested exception handler context preservation. The tests follow standard PostgreSQL regression test patterns with
RAISE INFOoutput verification against the expected output file (expected/dbms_utility.out), which is the appropriate approach for this testing framework.
- Add docker-compose setup with Ubuntu-based dev container - Add CLAUDE.md with git policy and build instructions - Configure build with libxml and uuid support - Update .gitignore for docker_library
So that docker-compose works for multiple git worktrees
- Install gdb in Dockerfile - Enable SYS_PTRACE capability for debugging - Document gdb usage in CLAUDE.md
Add critical reminder to verify installed files are newer than source after make install. Includes examples for rebuilding initdb and reinstalling extension SQL files.
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 - Register package in plisql--1.0.sql extension script - Add regression tests for the new functionality
- Condense ARCHITECTURE.md from exploratory discussion to final design - Document session-level context storage approach in pl_exec.c - Update DEPENDENCY_ANALYSIS.md with chosen solution details - Streamline EXISTING_PACKAGES.md and README.md
- Add pfree() for func_name on early return paths in pl_dbms_utility.c - Replace global plisql_current_exception_context with per-estate storage - Add exception_handling_estate pointer to track active handler context - This fixes issues where nested exception handlers could overwrite the outer handler's backtrace context
Verifies that calling a procedure with its own exception handler from within an outer exception handler preserves the outer context.
rophy
force-pushed
the
design/dbms_utility-v2
branch
from
189a1c0 to
84f4128
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
CLAUDE.md (1)
11-15: Add language identifier to fenced code blocks.Code blocks should specify a language for syntax highlighting. Update line 11 to use triple backticks with
bashidentifier.-``` +```bash <type>: <short description>This applies to all fenced code blocks in the file. Most blocks can use
bashfor shell commands or appropriate language tags (e.g.,sql,json).design/dbms_utility/README.md (1)
9-17: Add language identifier to file tree code block.The directory structure block should specify a language. Update to include
textor remove the language spec if plain formatting is preferred.-``` +```text src/pl/plisql/src/Or use a markdown list format without fenced code blocks.
src/pl/plisql/src/pl_dbms_utility.c (1)
45-159: Backtrace transformation logic looks correct; a few optional refinementsThe parsing and formatting here correctly:
- Filters out
SQL statementcontext lines.- Recognizes both
PL/pgSQL functionandPL/iSQL functionframes.- Handles
inline_code_blockas an anonymous block (ORA-06512: at line N).- Extracts function name and line number and formats
ORA-06512: at "SCHEMA.FUNC", line N.Memory management is careful (all pallocs are freed on every path, including early returns) and
ora_format_error_backtrace()safely handles the “no context” case by returning NULL.If you want to extend this later without changing behavior:
- Implement the existing TODO by resolving
schema_namefrompg_proc/pg_namespaceinstead of always using"PUBLIC", so backtraces reflect the real schema.- Consider an encoding-aware uppercasing helper (e.g., using a wide‑char aware routine) if you expect non‑ASCII identifiers; the current bytewise
pg_toupperis fine for pure ASCII.- Optionally, if no lines are transformed (e.g., all lines are skipped as
SQL statement), you might choose to return NULL instead of an empty string to make “no usable PL/iSQL frames” easier to detect.None of these are blockers for the current implementation.
Also applies to: 171-209
src/pl/plisql/src/pl_exec.c (1)
2141-2158: Per-estatecurrent_exception_contextmanagement is sound; consider a small helper to centralize free/set logic.The new
estate->current_exception_contexthandling correctly:
- stores
edata->contextin the function-lifespandatum_context,- frees any previous value before overwriting, and
- clears the pointer once exception handling finishes.
This fixes the earlier session-global leak and nested-handler semantics. To avoid duplicating the
pfree/NULL/MemoryContextStrduppattern in multiple places and reduce the chance of future divergence, you could wrap it in a tiny helper:+static void +plisql_set_current_exception_context(PLiSQL_execstate *estate, ErrorData *edata) +{ + if (estate->current_exception_context) + { + pfree(estate->current_exception_context); + estate->current_exception_context = NULL; + } + + if (edata && edata->context) + estate->current_exception_context = + MemoryContextStrdup(estate->datum_context, edata->context); +} ... - if (estate->current_exception_context) - { - pfree(estate->current_exception_context); - estate->current_exception_context = NULL; - } - if (edata->context) - { - estate->current_exception_context = - MemoryContextStrdup(estate->datum_context, edata->context); - } + plisql_set_current_exception_context(estate, edata); ... - if (estate->current_exception_context) - { - pfree(estate->current_exception_context); - estate->current_exception_context = NULL; - } + plisql_set_current_exception_context(estate, NULL);Not required for correctness, but would make future changes harder to get subtly wrong.
Also applies to: 2186-2194
src/pl/plisql/src/sql/dbms_utility.sql (1)
1-270: Consider adding a file header comment explaining the test structure and expected output.To improve maintainability, consider adding a brief header (before line 1) that explains the test file's purpose, the scenarios being tested, and references to the expected output file (e.g.,
src/pl/plisql/src/expected/dbms_utility.out).Example header:
-- -- Tests for DBMS_UTILITY package and FORMAT_ERROR_BACKTRACE function. -- -- This file contains 10 comprehensive test scenarios covering: -- 1. Basic exception handling -- 2-3. Nested and deeply nested procedure calls -- 4. Function call errors -- 5. Anonymous blocks -- 6. No-error case (backtrace should be empty) -- 7. Multiple exception handler levels with re-raising -- 8. Package procedures -- 9. Schema-qualified procedure references -- 10. Session-level exception context preservation across nested calls -- -- Expected outputs are in src/pl/plisql/src/expected/dbms_utility.out --
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/pl/plisql/src/expected/dbms_utility.outis excluded by!**/*.out
📒 Files selected for processing (19)
.devcontainer/Dockerfile(1 hunks).devcontainer/devcontainer.json(1 hunks).gitignore(1 hunks).gitmodules(1 hunks)CLAUDE.md(1 hunks)design/dbms_utility/ARCHITECTURE.md(1 hunks)design/dbms_utility/DEPENDENCY_ANALYSIS.md(1 hunks)design/dbms_utility/EXISTING_PACKAGES.md(1 hunks)design/dbms_utility/README.md(1 hunks)docker-compose.yaml(1 hunks)docker_library(1 hunks)scripts/build-docker.sh(1 hunks)src/bin/initdb/initdb.c(1 hunks)src/pl/plisql/src/Makefile(2 hunks)src/pl/plisql/src/pl_dbms_utility.c(1 hunks)src/pl/plisql/src/pl_exec.c(12 hunks)src/pl/plisql/src/plisql--1.0.sql(1 hunks)src/pl/plisql/src/plisql.h(2 hunks)src/pl/plisql/src/sql/dbms_utility.sql(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docker_library
🚧 Files skipped from review as they are similar to previous changes (2)
- src/bin/initdb/initdb.c
- design/dbms_utility/EXISTING_PACKAGES.md
🧰 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/pl_exec.c
🧬 Code graph analysis (2)
src/pl/plisql/src/pl_exec.c (2)
src/pl/plisql/src/pl_subproc_function.c (1)
plisql_function_from(1121-1135)src/backend/utils/mmgr/mcxt.c (1)
MemoryContextStrdup(1689-1700)
src/pl/plisql/src/plisql.h (1)
src/pl/plisql/src/pl_exec.c (1)
plisql_get_current_exception_context(10109-10121)
🪛 LanguageTool
CLAUDE.md
[style] ~26-~26: To form a complete sentence, be sure to include a subject.
Context: ...anch Convention (IMPORTANT) - master should be based on upstream/master and match o...
(MISSING_IT_THERE)
🪛 markdownlint-cli2 (0.18.1)
CLAUDE.md
11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
design/dbms_utility/DEPENDENCY_ANALYSIS.md
11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
20-20: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
81-81: 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)
design/dbms_utility/README.md
9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (16)
CLAUDE.md (1)
55-82: Excellent build verification guidance.The critical build verification steps (checking timestamps, forcing rebuilds, reinstalling artifacts) are comprehensive and will help prevent subtle build issues. This section is well-documented and actionable.
scripts/build-docker.sh (1)
1-19: Well-structured Docker build script with proper error handling.The script correctly resolves paths, archives source from git HEAD, and builds the image. The
set -eensures failures halt execution. Ensure thedocker_librarysubmodule is cloned and initialized before running this script.design/dbms_utility/README.md (1)
37-43: Clear design documentation structure.The references to architectural and dependency analysis docs provide good navigation. Ensure related design docs (ARCHITECTURE.md, DEPENDENCY_ANALYSIS.md, EXISTING_PACKAGES.md) are present and cross-referenced consistently.
.gitignore (1)
44-44: Correct generalization of ignore pattern.Changing from
/tmp_install/to/tmp_installbroadens the pattern to match both files and directories, which is appropriate for build artifacts..gitmodules (1)
1-4: Submodule URL and branch selection requires verification.The docker_library submodule references a personal fork (
rophy/ivorysql-docker_library) on thedevelopbranch. For upstream contribution and long-term maintenance, consider:
- URL appropriateness: Is this the official IvorySQL docker library repository, or should it reference an official org/repo?
- Branch stability: Using
developmay introduce instability. Consider pinning to a specific tag or commit hash instead.If this is intentional for development workflows, document the requirement clearly. If this PR is intended for upstream submission, coordinate with maintainers on the appropriate submodule reference.
docker-compose.yaml (1)
1-21: Docker Compose configuration is well-structured for development.The
devservice correctly enables debugging capabilities (SYS_PTRACE, seccomp:unconfined) and mounts the project. The optionaloracleservice with profile gating prevents unnecessary resource consumption.Verify:
.devcontainer/Dockerfileexists and provides necessary build tools- Oracle container image URL (
container-registry.oracle.com/database/free:23.26.0.0-lite) is accessible and currentORACLE_PWD=orapwdis acceptable for local dev; ensure documentation warns against production use.devcontainer/devcontainer.json (1)
1-22: Dev Container configuration is well-aligned with project build system.The configuration correctly references the docker-compose service and sets up VSCode for C/Makefile-based development. Extension choices (C++, Makefile, Git tools) are appropriate for the IvorySQL codebase. The
.devcontainer/Dockerfileproperly creates theivorysqluser (line 50), configures the home directory at/home/ivorysql(matching the workspace folder), and switches to this user for builds (line 57).src/pl/plisql/src/Makefile (1)
43-43: Build integration correctly supports new module and tests.Adding
pl_dbms_utility.oto OBJS anddbms_utilityto REGRESS follows the established pattern for extending the PL/iSQL module. The corresponding test files (sql/dbms_utility.sqlandexpected/dbms_utility.out) are present and properly formatted.Also applies to: 62-62
src/pl/plisql/src/plisql.h (1)
1162-1167: Execstate exception-context field and accessor look consistentThe added
current_exception_contextfield onPLiSQL_execstateand the exportedplisql_get_current_exception_context()match the usage pattern shown inpl_exec.cand cleanly separate internal state from the public accessor. No issues from a correctness or ABI standpoint.Also applies to: 1467-1471
.devcontainer/Dockerfile (1)
1-60: Dev container setup is solid for local IvorySQL developmentBase image, dependencies, non-root user, locale, and
tinientrypoint are all appropriate for a PL/iSQL / server build-and-test workflow. No blocking concerns here.src/pl/plisql/src/plisql--1.0.sql (1)
32-56: Confirmsysschema is guaranteed to exist when this script runs
CREATE FUNCTION sys.ora_format_error_backtrace()assumes thesysschema is already present. If this extension can ever be installed outside theinitdb -m oraclepath that sets upsys, function creation will fail. Ifsysis indeed created unconditionally before this script, you’re fine; otherwise consider creating or validating the schema here (or documenting the ordering constraint).src/pl/plisql/src/pl_exec.c (3)
534-544:active_estatesave/restore around all PL/iSQL entrypoints looks correct.Saving the previous
active_estatebeforeplisql_estate_setup()and restoring it on normal function/trigger/event-trigger exit ensures a well-formed “top-of-stack” pointer for nested PL/iSQL calls, without affecting exception-handling semantics (which useexception_handling_estate). No issues spotted here.Also applies to: 955-959, 1102-1109, 1327-1331, 1349-1356, 1413-1417
4419-4426: Initializingcurrent_exception_contextand updatingactive_estateinplisql_estate_setupis appropriate.Zeroing
estate->current_exception_contextand treating the freshly set up estate as the currentactive_estatehere keeps the state consistent for plugins and for later exception-context tracking. No functional issues noted.
10099-10121:plisql_get_current_exception_contextbehavior matches intended semantics once the global pointer is sanitized.The accessor correctly returns the stored context string for the estate currently handling an exception (via
exception_handling_estate) andNULLotherwise, and leaves ownership with PL/iSQL. With the additional clearing ofexception_handling_estateat transaction end (see earlier comment), this will have the expected Oracle-like contract without risking dereference of stale estates.src/pl/plisql/src/sql/dbms_utility.sql (2)
1-19: Comprehensive and well-structured test suite for FORMAT_ERROR_BACKTRACE.This file demonstrates excellent test coverage for the DBMS_UTILITY.FORMAT_ERROR_BACKTRACE function across 10 distinct scenarios. Each test follows a consistent pattern (create → call → verify → clean up), with proper cleanup using DROP statements. The test scenarios systematically exercise important use cases: basic exceptions, nested procedure calls (multiple depth levels), functions, anonymous blocks, no-error conditions, exception propagation, packages, schema-qualified references, and session-level context preservation.
235-270: Excellent sophisticated test for session-level exception context preservation (Test 10).This test validates critical behavior: that the session-level exception context (captured via FORMAT_ERROR_BACKTRACE) is preserved when an inner procedure with its own exception handler is called from within an outer exception handler. The test captures the backtrace before and after the inner call (lines 254 and 257), then verifies they match. This directly validates the PR objective of "session-level PL/iSQL exception-context storage" and tests an important edge case that could easily be missed. The logic is sound: the 'Outer error' context should remain unchanged because the 'Inner error' is caught and handled locally in
test_nested_inner, never propagating up.
| ### Exception Context Storage | ||
|
|
||
| The key challenge is accessing exception context from a C function that's called from PL/iSQL package body. | ||
|
|
||
| **Solution:** Session-level storage in `pl_exec.c` | ||
|
|
||
| ```c | ||
| // pl_exec.c - Static session storage | ||
| static char *plisql_current_exception_context = NULL; | ||
|
|
||
| // When entering exception handler (in exec_stmt_block): | ||
| if (edata->context) | ||
| { | ||
| plisql_current_exception_context = | ||
| MemoryContextStrdup(TopMemoryContext, edata->context); | ||
| } | ||
|
|
||
| // When exiting exception handler: | ||
| if (plisql_current_exception_context) | ||
| { | ||
| pfree(plisql_current_exception_context); | ||
| plisql_current_exception_context = NULL; | ||
| } | ||
|
|
||
| // Public API for retrieval | ||
| const char * | ||
| plisql_get_current_exception_context(void) | ||
| { | ||
| return plisql_current_exception_context; | ||
| } | ||
| ``` | ||
| **Why this approach:** | ||
| 1. Exception context is stored when entering handler (before user code runs) | ||
| 2. C function can retrieve it via public API without accessing `PLiSQL_execstate` | ||
| 3. Context is cleared after exception handling completes | ||
| 4. Memory allocated in `TopMemoryContext` survives function calls | ||
There was a problem hiding this comment.
Exception-context storage description is stale vs current implementation
This section still documents a global static char *plisql_current_exception_context in pl_exec.c, but the code now uses per‑PLiSQL_execstate storage (current_exception_context) exposed via plisql_get_current_exception_context(). It would be good to adjust the sample code and bullets here to match the final design (per‑estate context plus exception_handling_estate indirection) so future readers don’t chase an outdated pattern.
| ``` | ||
| src/pl/plisql/src/ → plisql.so (language runtime) | ||
| contrib/ivorysql_ora/ → ivorysql_ora.so (Oracle compatibility) | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Tighten fenced code blocks and sync storage design with implementation
Two small points here:
-
Several fenced blocks lack a language (
src/pl/...layout, “User SQL” sketch, data‑flow diagram, final architecture tree). Adding something liketext/cto the opening ``` will address the MD040 markdownlint warnings and improve rendering. -
The example under “Implementation Approach” still documents a global
static char *plisql_current_exception_context, whereas the code now usesPLiSQL_execstate.current_exception_contextplusplisql_get_current_exception_context(). Mirroring the actual API and per‑estate storage model here will keep the analysis accurate.
Also applies to: 20-30, 49-67, 81-101, 127-133
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
design/dbms_utility/DEPENDENCY_ANALYSIS.md around lines 11-15 (and also apply to
ranges 20-30, 49-67, 81-101, 127-133): several fenced code blocks lack a
language tag and one example still shows the old global static variable; update
each opening triple-backtick to include an appropriate language (e.g., text, c)
to satisfy MD040 and improve rendering, and replace the example that references
static char *plisql_current_exception_context with the current API usage (access
PLiSQL_execstate.current_exception_context and call
plislq_get_current_exception_context() where applicable), ensuring the
documentation reflects per-exec-state storage and matches the implementation.
| /* | ||
| * Stack of currently active execution states. The topmost entry is the | ||
| * currently executing function. | ||
| */ | ||
| static PLiSQL_execstate *active_estate = NULL; | ||
|
|
||
| /* | ||
| * Pointer to the estate currently handling an exception. This is separate | ||
| * from active_estate because when we call functions (like DBMS_UTILITY | ||
| * package functions) from within an exception handler, active_estate | ||
| * changes but we still need access to the original handler's context. | ||
| */ | ||
| static PLiSQL_execstate *exception_handling_estate = NULL; |
There was a problem hiding this comment.
Guard exception_handling_estate against dangling pointers across unhandled errors and transaction end.
exception_handling_estate points at a PLiSQL_execstate that is stack-allocated in plisql_exec_function/plisql_exec_trigger/plisql_exec_event_trigger, is set only in exec_stmt_block’s PG_CATCH, and is never cleared in the xact/subxact callbacks. If an error is raised inside an exception handler and escapes all PL/iSQL EXCEPTION blocks (unhandled ERROR), control longjmps out of the inner PG_CATCH before:
- restoring
exception_handling_estateto the saved value, and - clearing
estate->current_exception_context.
When the outer call handler unwinds, the stack frame containing that PLiSQL_execstate disappears, but exception_handling_estate still holds its address. A later call to plisql_get_current_exception_context() will then dereference a dangling pointer, which is undefined behavior and can crash or return garbage. Because plisql_xact_cb() currently only resets the simple eval estate/resource owner and does not touch exception_handling_estate, the stale pointer can survive across transaction boundaries.
To avoid this, the global pointer should be explicitly nulled on transaction end (and, minimally, on abort) so that plisql_get_current_exception_context() reliably returns NULL when there is no active handler and never dereferences freed stack memory.
A minimal, contained fix:
void
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;
@@
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;
}
+
+ /*
+ * No exception handler should survive a top-level transaction boundary.
+ * Drop any stale pointer to a stack-allocated estate so
+ * plisql_get_current_exception_context() cannot dereference freed memory.
+ */
+ exception_handling_estate = NULL;
}If you’re concerned about semantics around COMMIT/ROLLBACK inside non-atomic procedures, you could restrict the exception_handling_estate = NULL to the abort events only, but some explicit clearing at transaction end is still needed to prevent UB on subsequent calls into PL/iSQL.
Also applies to: 1991-1992, 2161-2167, 2186-2194, 9550-9582, 10099-10121
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.