feat: add DBMS_UTILITY.FORMAT_ERROR_BACKTRACE #16
Conversation
📝 WalkthroughWalkthroughImplements DBMS_UTILITY.FORMAT_ERROR_BACKTRACE, FORMAT_ERROR_STACK, and FORMAT_CALL_STACK by exposing four new public APIs from the PL/iSQL runtime to retrieve exception context and call stacks, then providing Oracle-compatible wrappers in ivorysql_ora that call these APIs and transform PostgreSQL output to Oracle format. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User<br/>(PL/iSQL)
participant Handler as Exception<br/>Handler
participant DBMSUtil as DBMS_UTILITY<br/>(ivorysql_ora)
participant PlISQLAPI as PL/iSQL<br/>Runtime APIs
User->>Handler: Raise exception in block
Handler->>Handler: Set exception_handling_estate
Handler->>DBMSUtil: FORMAT_ERROR_BACKTRACE()
Note over DBMSUtil: Dynamic lookup:<br/>load plisql APIs if needed
DBMSUtil->>PlISQLAPI: plisql_get_current_exception_context()
PlISQLAPI-->>DBMSUtil: Exception context string
Note over DBMSUtil: Parse & transform context<br/>PostgreSQL → Oracle format
DBMSUtil->>PlISQLAPI: plisql_get_current_exception_sqlerrcode()
PlISQLAPI-->>DBMSUtil: SQLSTATE code
Note over DBMSUtil: Map to ORA-##### number
DBMSUtil-->>User: Oracle-formatted<br/>backtrace string
Handler->>Handler: Clear exception_handling_estate
sequenceDiagram
participant Caller as Caller
participant Func1 as func_level1()
participant Func2 as func_level2()
participant Func3 as func_level3()
participant PlISQLAPI as PL/iSQL<br/>Runtime
participant DBMSUtil as FORMAT_CALL_STACK()
Caller->>Func1: Call
Func1->>Func2: Call
Func2->>Func3: Call
Func3->>DBMSUtil: FORMAT_CALL_STACK()
Note over PlISQLAPI: Walk error_context_stack<br/>from innermost to outermost
DBMSUtil->>PlISQLAPI: plisql_get_call_stack()
PlISQLAPI-->>DBMSUtil: Formatted call stack<br/>(func_level1 → func_level2 → func_level3)
DBMSUtil-->>Func3: Oracle-style<br/>call stack text
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
contrib/ivorysql_ora/src/builtin_functions/builtin_functions--1.0.sql (1)
1395-1406: Consider adding STRICT and PARALLEL SAFE qualifiers for consistency and safety.Comparing to similar C functions in this file (e.g., lines 1197–1228), most include STRICT (reject NULL inputs) and many include PARALLEL SAFE. Evaluate whether these new functions should be marked similarly. For instance, if NULL input to the internal variant should be rejected, add STRICT. If the C implementation is thread-safe with no global state, add PARALLEL SAFE.
If the functions should reject NULL inputs and are thread-safe, apply this diff:
CREATE FUNCTION sys.ora_format_error_backtrace_internal(text) RETURNS VARCHAR2 AS 'MODULE_PATHNAME','ora_format_error_backtrace_internal' LANGUAGE C + STRICT + PARALLEL SAFE STABLE; CREATE FUNCTION sys.ora_format_error_backtrace() RETURNS VARCHAR2 AS 'MODULE_PATHNAME','ora_format_error_backtrace' LANGUAGE C + PARALLEL SAFE VOLATILE;contrib/ivorysql_ora/Makefile (1)
77-88: Add build-time verification for plisql.so availability.Currently, there is no check that plisql.so exists or is linkable. If the library is missing, the linker will fail with an unclear error. Consider adding a build target or check to verify the library is available before attempting to link.
Optionally, add a build-time check (example using shell):
# After the SHLIB_LINK definition, add: $(shell $(PG_CONFIG) --pkglibdir)/plisql.so: @if [ ! -f "$$@" ]; then \ echo "Error: plisql.so not found at $$@"; \ echo "Ensure PL/iSQL extension is installed."; \ exit 1; \ fiAlternatively, document the prerequisite in a README or INSTALL section that users must have plisql installed before building ivorysql_ora.
src/pl/plisql/src/pl_exec.c (1)
2071-2085: Exception context capture logic is correct; consider a narrower memory contextThe capture logic correctly:
- Frees any previous
plisql_current_exception_contextbefore overwriting it.- Copies
edata->contextso later changes toedatawon’t affect callers.- Restricts the lifetime of the stored context to “inside the active exception handler”.
One possible refinement is to allocate the duplicated context string in a transaction- or statement‑scoped context (e.g.,
TopTransactionContextor the block’sstmt_mcontext) instead ofTopMemoryContext, so that any missed explicit free (for example in a future code path) cannot outlive the transaction. That’s optional given the current balanced free/assign pattern.contrib/ivorysql_ora/src/builtin_functions/dbms_utility.c (3)
145-150: Redundant NULL check forschema_name.The
schema_namevariable is unconditionally assigned on line 129 viapstrdup("PUBLIC"), so the NULL check on line 148 is always false and can be simplified.pfree(func_name); pfree(func_upper); pfree(schema_upper); - if (schema_name) - pfree(schema_name); + pfree(schema_name);
127-129: Schema lookup TODO affects Oracle compatibility fidelity.The hardcoded
"PUBLIC"schema means functions in other schemas will show incorrect schema names in backtraces. Test 9 (test_schema.schema_caller) will show"PUBLIC.SCHEMA_CALLER"instead of"TEST_SCHEMA.SCHEMA_CALLER".Consider implementing the catalog lookup to retrieve the actual schema. This would involve querying
pg_procto get the function's namespace OID, then resolving viapg_namespace.Would you like me to provide an implementation for the schema lookup, or would you prefer to track this as a follow-up issue?
213-257: Code duplication withora_format_error_backtrace.The line parsing and transformation logic (lines 234-254) is duplicated from the main function. Consider extracting a shared helper:
static text * format_backtrace_from_context(const char *pg_context) { StringInfoData result; char *context_copy; char *line; char *saveptr; if (pg_context == NULL || pg_context[0] == '\0') return NULL; initStringInfo(&result); context_copy = pstrdup(pg_context); line = strtok_r(context_copy, "\n", &saveptr); while (line != NULL) { if (line[0] != '\0') transform_and_append_line(&result, line); line = strtok_r(NULL, "\n", &saveptr); } pfree(context_copy); return cstring_to_text(result.data); }Then both public functions become thin wrappers. This reduces maintenance burden and ensures consistent behavior.
📜 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 (8)
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_functions/builtin_functions--1.0.sql(1 hunks)contrib/ivorysql_ora/src/builtin_functions/dbms_utility--1.0.sql(1 hunks)contrib/ivorysql_ora/src/builtin_functions/dbms_utility.c(1 hunks)src/pl/plisql/src/pl_exec.c(4 hunks)src/pl/plisql/src/plisql.h(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
contrib/ivorysql_ora/src/builtin_functions/dbms_utility.c (3)
src/common/stringinfo.c (2)
appendStringInfo(144-166)initStringInfo(96-100)src/pl/plisql/src/pl_exec.c (1)
plisql_get_current_exception_context(9941-9945)src/backend/utils/adt/varlena.c (2)
cstring_to_text(193-197)text_to_cstring(226-242)
src/pl/plisql/src/plisql.h (1)
src/pl/plisql/src/pl_exec.c (1)
plisql_get_current_exception_context(9941-9945)
🔇 Additional comments (12)
contrib/ivorysql_ora/ivorysql_ora_merge_sqls (1)
3-3: Module registration is correct.The new DBMS_UTILITY module path follows the established pattern and correctly points to the C implementation directory.
contrib/ivorysql_ora/src/builtin_functions/builtin_functions--1.0.sql (1)
1395-1399: Verify STABLE is appropriate for internal function accepting explicit input.The internal variant accepts an explicit text parameter representing exception context. Assuming the transformation logic is deterministic (same context → same formatted output), STABLE is reasonable. However, confirm with the C implementation that there are no side effects or time-dependent behaviors.
Please verify the C implementation (contrib/ivorysql_ora/src/builtin_functions/dbms_utility.c) for
ora_format_error_backtrace_internalis pure/deterministic with no side effects or dependencies on external state.contrib/ivorysql_ora/Makefile (1)
28-28: Build object addition is correct.The dbms_utility.o object file is properly added to the OBJS list following the established pattern.
src/pl/plisql/src/pl_exec.c (3)
116-122: Session-level exception-context storage design looks soundHaving a backend-local static
plisql_current_exception_contextand documenting it as session-level storage matches the single-backend-per-session model, and the choice to expose it via a small accessor keeps the surface area tight. No functional or safety issues spotted here.
2103-2110: Explicit clearing of exception context after handler is robustThe post‑handler cleanup unconditionally frees and NULLs
plisql_current_exception_contextbefore deciding whether to rethrow, which prevents stale context from leaking into unrelated code paths and keeps the accessor semantics (“non‑NULL only while in a handler”) clear and predictable. This looks correct and symmetric with the capture code.
9932-9945: Accessor API is minimal and correctly documents ownership
plisql_get_current_exception_context()cleanly exposes the internal pointer asconst char *and the comment clearly states that callers must not free it. Combined with the explicit set/clear points, this should be safe for DBMS_UTILITY and similar consumers.src/pl/plisql/src/plisql.h (1)
1459-1463: LGTM!The new function declaration follows the existing patterns in the header. The
PGDLLEXPORTannotation correctly exports the symbol for use by the contrib module, and theconst char *return type appropriately indicates the returned string should not be modified by callers.contrib/ivorysql_ora/src/builtin_functions/dbms_utility--1.0.sql (1)
37-44: LGTM!The package body implementation is clean - it correctly delegates to the underlying C function via
sys.ora_format_error_backtrace(), maintaining a clear separation between the Oracle-compatible API and the implementation.contrib/ivorysql_ora/sql/dbms_utility.sql (3)
1-19: Good test coverage with comprehensive scenarios.The test suite covers essential cases: basic exceptions, nested calls, deep nesting, functions, anonymous blocks, packages, and schema-qualified calls. The use of
RAISE INFOto output backtraces facilitates verification.
133-144: Test 6 verifies NULL/empty behavior correctly.This test appropriately checks that
FORMAT_ERROR_BACKTRACEreturns NULL (or empty) when called outside an exception handler, which aligns with Oracle's behavior.Note: The output format
[%]will show either[]for empty or nothing visible for NULL depending on how NULL is rendered. Consider usingCOALESCEto make the output more explicit if needed:RAISE INFO 'No error - backtrace: [%]', COALESCE(v_backtrace, '<NULL>');
211-233: LGTM!Test 9 properly tests schema-qualified procedures and cleans up with
DROP SCHEMA ... CASCADE. Note that due to the hardcoded"PUBLIC"schema in the C implementation, the actual backtrace output will show"PUBLIC.SCHEMA_CALLER"instead of"TEST_SCHEMA.SCHEMA_CALLER"- this relates to the TODO mentioned in the C code.contrib/ivorysql_ora/src/builtin_functions/dbms_utility.c (1)
198-202: Based on my verification, the review comment identifies a real potential edge case, though Oracle's exact behavior in this scenario is not fully documented. Let me provide the rewritten review comment:
Handle empty result when all context lines are skipped or unparseable.
The function returns NULL only when
pg_contextis NULL or empty (line 177-178), which aligns with Oracle's documented behavior. However, ifpg_contextis non-empty but contains only "SQL statement" lines or unparseable formats,transform_and_append_linewill skip all of them, resulting in an emptyresult.data. The function will then return an empty TEXT string instead of NULL.This edge case exists in both
ora_format_error_backtrace(line 201) andora_format_error_backtrace_internal(line 256). Consider whether empty backtrace should return NULL (indicating no formattable content) or empty TEXT (indicating handler was active but nothing to format):pfree(context_copy); - /* Oracle always ends with a newline - don't remove it */ - /* The transform_and_append_line function already adds newlines */ + /* If no lines were transformed, return NULL */ + if (result.len == 0) + { + pfree(result.data); + PG_RETURN_NULL(); + } PG_RETURN_TEXT_P(cstring_to_text(result.data));
contrib/ivorysql_ora/Makefile
Outdated
| ifdef USE_PGXS | ||
| SHLIB_LINK += $(shell $(PG_CONFIG) --pkglibdir)/plisql.so | ||
| else | ||
| # Link directly to installed plisql.so with absolute path | ||
| # This avoids the relative path issue and ensures the library is found at runtime | ||
| SHLIB_LINK += /home/ivorysql/ivorysql/lib/postgresql/plisql.so | ||
| endif |
There was a problem hiding this comment.
Critical: Hardcoded absolute path breaks portability across installations.
Line 84 embeds a user-specific, machine-specific absolute path: /home/ivorysql/ivorysql/lib/postgresql/plisql.so. This path will fail on any other user or machine. The comment mentions avoiding RPATH issues, but this absolute path approach is worse for portability than RPATH.
Remove the hardcoded path. Instead, use one of these approaches:
Option 1 (preferred): Derive the path dynamically from the PostgreSQL installation
- ifdef USE_PGXS
- SHLIB_LINK += $(shell $(PG_CONFIG) --pkglibdir)/plisql.so
- else
- # Link directly to installed plisql.so with absolute path
- # This avoids the relative path issue and ensures the library is found at runtime
- SHLIB_LINK += /home/ivorysql/ivorysql/lib/postgresql/plisql.so
- endif
+ ifdef USE_PGXS
+ SHLIB_LINK += $(shell $(PG_CONFIG) --pkglibdir)/plisql.so
+ else
+ SHLIB_LINK += $(shell $(PG_CONFIG) --pkglibdir)/plisql.so
+ endifOption 2: Use a variable or search path (if Option 1 doesn't work in your environment)
- ifdef USE_PGXS
- SHLIB_LINK += $(shell $(PG_CONFIG) --pkglibdir)/plisql.so
- else
- # Link directly to installed plisql.so with absolute path
- # This avoids the relative path issue and ensures the library is found at runtime
- SHLIB_LINK += /home/ivorysql/ivorysql/lib/postgresql/plisql.so
- endif
+ # Link to plisql shared library using pg_config
+ SHLIB_LINK += $(shell $(PG_CONFIG) --pkglibdir)/plisql.soThis ensures the path is resolved at build time relative to the actual PostgreSQL installation, making the build portable.
📝 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.
| ifdef USE_PGXS | |
| SHLIB_LINK += $(shell $(PG_CONFIG) --pkglibdir)/plisql.so | |
| else | |
| # Link directly to installed plisql.so with absolute path | |
| # This avoids the relative path issue and ensures the library is found at runtime | |
| SHLIB_LINK += /home/ivorysql/ivorysql/lib/postgresql/plisql.so | |
| endif | |
| # Link to plisql shared library using pg_config | |
| SHLIB_LINK += $(shell $(PG_CONFIG) --pkglibdir)/plisql.so |
| ifdef USE_PGXS | |
| SHLIB_LINK += $(shell $(PG_CONFIG) --pkglibdir)/plisql.so | |
| else | |
| # Link directly to installed plisql.so with absolute path | |
| # This avoids the relative path issue and ensures the library is found at runtime | |
| SHLIB_LINK += /home/ivorysql/ivorysql/lib/postgresql/plisql.so | |
| endif | |
| ifdef USE_PGXS | |
| SHLIB_LINK += $(shell $(PG_CONFIG) --pkglibdir)/plisql.so | |
| else | |
| SHLIB_LINK += $(shell $(PG_CONFIG) --pkglibdir)/plisql.so | |
| endif |
🤖 Prompt for AI Agents
In contrib/ivorysql_ora/Makefile around lines 79 to 85, the SHLIB_LINK currently
uses a hardcoded absolute path
(/home/ivorysql/ivorysql/lib/postgresql/plisql.so) which breaks portability;
replace that hardcoded path by deriving the installed pkglibdir from pg_config
(e.g. use $(shell $(PG_CONFIG) --pkglibdir)/plisql.so) so the correct lib
location is computed at build time, and if that approach is not viable add a
configurable variable (e.g. PLISQL_LIBDIR ?= $(shell $(PG_CONFIG) --pkglibdir))
and reference $(PLISQL_LIBDIR)/plisql.so so packagers can override it or provide
a fallback search mechanism instead of an absolute path.
| CREATE FUNCTION sys.ora_format_error_backtrace() | ||
| RETURNS VARCHAR2 | ||
| AS 'MODULE_PATHNAME','ora_format_error_backtrace' | ||
| LANGUAGE C | ||
| STABLE; |
There was a problem hiding this comment.
Function marked STABLE but depends on session state—should be VOLATILE.
ora_format_error_backtrace() retrieves the current exception context and produces different output as exceptions change. Per PostgreSQL semantics, STABLE functions must return identical results for identical inputs within a transaction, but this function takes no parameters and depends on session state (the exception context). This violates the STABLE contract.
Change STABLE to VOLATILE:
- CREATE FUNCTION sys.ora_format_error_backtrace()
- RETURNS VARCHAR2
- AS 'MODULE_PATHNAME','ora_format_error_backtrace'
- LANGUAGE C
- STABLE;
+ CREATE FUNCTION sys.ora_format_error_backtrace()
+ RETURNS VARCHAR2
+ AS 'MODULE_PATHNAME','ora_format_error_backtrace'
+ LANGUAGE C
+ VOLATILE;🤖 Prompt for AI Agents
In contrib/ivorysql_ora/src/builtin_functions/builtin_functions--1.0.sql around
lines 1402 to 1406, the function declaration marks ora_format_error_backtrace()
as STABLE though it reads session exception context and can return different
values within a transaction; change the volatility from STABLE to VOLATILE in
the function header to reflect its dependence on session state so PostgreSQL's
planner/runtime treats it correctly.
| * This Oracle-compatible function automatically retrieves the exception | ||
| * context when called from within an exception handler. Returns empty | ||
| * string if called outside an exception handler. |
There was a problem hiding this comment.
Documentation states "empty string" but implementation returns NULL.
The comment says "Returns empty string if called outside an exception handler," but the C implementation (ora_format_error_backtrace) returns NULL when not in an exception handler context (via PG_RETURN_NULL()). This matches Oracle's actual behavior where FORMAT_ERROR_BACKTRACE returns NULL outside exception handlers.
Consider updating the comment for accuracy:
- * context when called from within an exception handler. Returns empty
- * string if called outside an exception handler.
+ * context when called from within an exception handler. Returns NULL
+ * if called outside an exception handler.📝 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.
| * This Oracle-compatible function automatically retrieves the exception | |
| * context when called from within an exception handler. Returns empty | |
| * string if called outside an exception handler. | |
| * This Oracle-compatible function automatically retrieves the exception | |
| * context when called from within an exception handler. Returns NULL | |
| * if called outside an exception handler. |
🤖 Prompt for AI Agents
In contrib/ivorysql_ora/src/builtin_functions/dbms_utility--1.0.sql around lines
15 to 17, the comment incorrectly states the function "Returns empty string if
called outside an exception handler" while the C implementation returns NULL;
update the comment to state that the function returns NULL (not an empty string)
when called outside an exception handler to match the implementation and Oracle
behavior.
rophy
changed the title
rophy
force-pushed
the
feat/dbms_utility
branch
from
e5495a3 to
2b5e118
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
contrib/ivorysql_ora/src/builtin_functions/builtin_functions--1.0.sql (1)
1394-1406:sys.ora_format_error_backtrace()should be marked VOLATILE, not STABLE
sys.ora_format_error_backtrace_internal(text)is fine as STABLE (it’s a pure transform of its argument), butsys.ora_format_error_backtrace()has no arguments and depends on session exception context. Its result can change within a single transaction as different exceptions are raised, which violates the contract of STABLE functions.Recommend changing its volatility to VOLATILE:
CREATE FUNCTION sys.ora_format_error_backtrace() RETURNS VARCHAR2 AS 'MODULE_PATHNAME','ora_format_error_backtrace' LANGUAGE C -STABLE; +VOLATILE;contrib/ivorysql_ora/src/builtin_functions/dbms_utility--1.0.sql (1)
15-17: UpdateFORMAT_ERROR_BACKTRACEcomment to say “returns NULL” instead of “empty string”The package spec currently documents:
Returns empty string if called outside an exception handler.
But
sys.ora_format_error_backtrace()(and the underlying C implementation) returnNULLwhen there is no current exception context, which also matches Oracle behavior described in the PR. To avoid confusion, adjust the comment accordingly, e.g.:- * context when called from within an exception handler. Returns empty - * string if called outside an exception handler. + * context when called from within an exception handler. Returns NULL + * if called outside an exception handler.
🧹 Nitpick comments (3)
contrib/ivorysql_ora/src/builtin_functions/dbms_utility.c (2)
59-152: Minor cleanup intransform_and_append_lineearly-return pathsThe parsing logic and Oracle-style formatting look good. One small polish: in paths where
func_namehas been allocated but you bail out early (e.g.,func_end/line_markernot found), it isn’tpfree’d, while in the successful path you do free it. It’s not a correctness bug in the per-call memory context, but you can make the function more self‑contained by freeingfunc_namebefore thosereturn false;sites.For example:
- func_end = strstr(p, " line "); - if (!func_end) - return false; + func_end = strstr(p, " line "); + if (!func_end) + { + pfree(func_name); + return false; + } ... - line_marker = strstr(func_end, " line "); - if (!line_marker) - return false; + line_marker = strstr(func_end, " line "); + if (!line_marker) + { + pfree(func_name); + return false; + }
211-257: Optional: freepg_contexton empty-input fast path in_internal
ora_format_error_backtrace_internalis nicely self-contained and frees bothcontext_copyandpg_contextin the normal path. In thepg_context == NULL || pg_context[0] == '\0'case, you currentlyPG_RETURN_NULL()without freeingpg_context; harmless in the per-call context, but you can tighten it slightly:- if (pg_context == NULL || pg_context[0] == '\0') - PG_RETURN_NULL(); + if (pg_context == NULL || pg_context[0] == '\0') + { + if (pg_context) + pfree(pg_context); + PG_RETURN_NULL(); + }contrib/ivorysql_ora/sql/dbms_utility.sql (1)
5-233: Good coverage; consider clarifying Test 6 comment re NULL vs “empty” resultThe test suite nicely exercises
DBMS_UTILITY.FORMAT_ERROR_BACKTRACEacross nested procedures, functions, packages, anonymous blocks, and schema-qualified calls. For Test 6 (lines 133–140), the comment says “No exception (should return empty)”, but per the implementation and package docs it actually returnsNULLoutside an exception handler and will print as an empty placeholder in theRAISE INFOmessage.To avoid confusion for future readers, you might tweak the comment to something like “No exception (should return NULL, printed as empty)”.
📜 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 (8)
contrib/ivorysql_ora/Makefile(3 hunks)contrib/ivorysql_ora/ivorysql_ora_merge_sqls(1 hunks)contrib/ivorysql_ora/sql/dbms_utility.sql(1 hunks)contrib/ivorysql_ora/src/builtin_functions/builtin_functions--1.0.sql(1 hunks)contrib/ivorysql_ora/src/builtin_functions/dbms_utility--1.0.sql(1 hunks)contrib/ivorysql_ora/src/builtin_functions/dbms_utility.c(1 hunks)src/pl/plisql/src/pl_exec.c(4 hunks)src/pl/plisql/src/plisql.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pl/plisql/src/pl_exec.c
- src/pl/plisql/src/plisql.h
🧰 Additional context used
🧬 Code graph analysis (1)
contrib/ivorysql_ora/src/builtin_functions/dbms_utility.c (4)
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/pl/plisql/src/pl_exec.c (1)
plisql_get_current_exception_context(9941-9945)src/backend/utils/adt/varlena.c (2)
cstring_to_text(193-197)text_to_cstring(226-242)
🔇 Additional comments (3)
contrib/ivorysql_ora/src/builtin_functions/dbms_utility.c (1)
164-202:ora_format_error_backtracecontrol flow matches the intended semanticsThis function cleanly pulls the current PL/iSQL exception context, returns
NULLwhen absent, and line-splits into Oracle-style backtrace entries while preserving a trailing newline. The use of a private copy forstrtok_rand delegating per-line logic totransform_and_append_lineis straightforward and appropriate.contrib/ivorysql_ora/Makefile (1)
28-31: Wiring ofdbms_utilityobject, regress test, and plisql include path looks consistentAdding
src/builtin_functions/dbms_utility.otoOBJS, registering thedbms_utilityregress test, and extendingPG_CPPFLAGSwith the plisql include directory are all aligned with the new backend implementation and look correct for in-tree builds.Also applies to: 56-75, 78-79
contrib/ivorysql_ora/ivorysql_ora_merge_sqls (1)
1-5: Module registration forsrc/builtin_functions/dbms_utilityis appropriateIncluding
src/builtin_functions/dbms_utilityin the merge list correctly exposes the new package alongside the existing datatype and builtin modules.
rophy
force-pushed
the
feat/dbms_utility
branch
from
2b5e118 to
e0d1f54
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
src/backend/catalog/genbki.pl (1)
1121-1140: Zero‑OID warning behavior now depends ontype_name; confirm intended scopeThe new guard:
if (defined($bki_values->{type_name}) and $bki_values->{type_name} ne 'oracharchar' ... and $bki_values->{type_name} ne 'binary_double' ) { warn sprintf "invalid zero OID reference ..." if !$lookup_opt; }means we only emit the
"invalid zero OID reference"warning when:
type_nameexists and- it is not one of the Oracle-ish types in the allowlist.
For any row where
type_nameis absent/undefined,defined(...)is false, so we now silently accept'-'/'0'in lookup fields where we used to warn.If the goal was strictly to suppress this warning only for the listed Oracle pseudo-types and keep the previous behavior for all other catalogs, you may want to keep the old default and special‑case just those names, e.g.:
- if (defined($bki_values->{type_name}) - and $bki_values->{type_name} ne 'oracharchar' - ... - and $bki_values->{type_name} ne 'binary_double' - ) - { - warn sprintf "invalid zero OID reference ..." if !$lookup_opt; - } + my $tn = $bki_values->{type_name}; + my %zero_ok = map { $_ => 1 } qw( + oracharchar oracharbyte oravarcharchar oravarcharbyte + oradate oratimestamp oratimestamptz oratimestampltz + yminterval dsinterval number binary_float binary_double + ); + + # Keep existing warnings by default; only suppress for the + # explicit Oracle pseudo-types. + if (!defined($tn) || !$zero_ok{$tn}) + { + warn sprintf "invalid zero OID reference ..." if !$lookup_opt; + }If the broader suppression (for undefined
type_nameas well) is deliberate—for example, because all non‑Oracle rows should be allowed to use0/-without noise—then the current logic is fine, but it might still be worth extracting the allowlist into a%zero_okhash for readability and easier future extension.src/interfaces/libpq/ivy-exec.c (2)
3760-4047: Fix to cleanup-loop variable scope inIvyreplacenamebindtopositionIntroducing a function‑scope
int i = 0;and using separate block‑localjfor inner loops ensures theerror_handlecleanup loop (for (i = 0; i < stmtHandle->nParams; i++)) has a valid variable in scope. This avoids relying on a loop index declared inside a narrower block and looks correct.You might later consider factoring the duplicated get_parameter_description / CALL‑rewrite logic shared with
Ivyreplacenamebindtoposition2/3into a helper to reduce future bug surface.
4053-4319: Consistent fix inIvyreplacenamebindtoposition2Mirroring the change above, adding function‑scope
int i = 0;and usingjonly inside the get_parameter_description block keeps the cleanup loop aterror_handlewell‑defined and matches the intended control flow.Same as for
Ivyreplacenamebindtoposition, consider deduplicating the nearly identical error‑cleanup and parameter‑name building logic across the three replacename* helpers to simplify maintenance.src/pl/plisql/src/pl_scanner.c (1)
121-175: Globals commented out; consider cleaning upcur_line_numhandling in snapshot helpersCommenting out the unused
plisql_scannerandcur_line_numglobals is fine given that all active code now reads/writesyyextra->cur_line_num. However,PLiSQL_yylex_global_properstill contains acur_line_numfield thatplisql_get_yylex_global_proper()never populates andplisql_recover_yylex_global_proper()never restores (the assignment is commented). Either wire these helpers to copyyyextra->cur_line_numin and out, or drop the field entirely if no caller needs it, to avoid confusion about which line number state is authoritative.contrib/ivorysql_ora/Makefile (1)
5-32: Build and test wiring fordbms_utilityand plisql header dependency looks correct
- Adding
src/builtin_packages/dbms_utility/dbms_utility.otoOBJSanddbms_utilitytoORA_REGRESScleanly hooks the new package into the shared library and regression suite.- Including
ora_xml_functionsinORA_REGRESSkeeps the XML helper coverage together with the new Oracle‑side utilities.PG_CPPFLAGS += -I$(top_srcdir)/src/pl/plisql/srcis the right knob to pick upplisql.hforplisql_get_current_exception_context(); with make’s lazy expansion it will work both in-tree and via PGXS as long as the core sources are present.Given that ivorysql_ora is tightly coupled to the core tree, this direct include of the plisql source directory is acceptable here.
If you ever want ivorysql_ora to build against an installed server without full sources, consider installing a small public header for the exported plisql API instead of including from
src/pl/plisql/src.Also applies to: 58-75, 78-80
src/pl/plisql/src/plisql.h (1)
1461-1465: Public exception-context accessor looks correct; consider documenting pointer lifetimeThe new
plisql_get_current_exception_context()declaration matches the implementation and existing export patterns in this header, so exposing it for DBMS_UTILITY use looks fine. As a minor enhancement, you might add a brief note that the returnedconst char *is only valid while the current exception handler is active and must be copied by callers, to make the ownership/lifetime contract explicit to other extensions.src/pl/plisql/src/pl_comp.c (1)
1098-1117: Re-init and local index variable are sound; behavior unchangedZeroing
argtypenames/rettypenamebefore reusing them and switching to a block-local loop indexjinside thecheck_referenced_objectsblock keeps the logic clear and avoids interference with the function-scopei. The loop still correctly walks the argument-type-name array forpronargsentries and guards both null and empty strings before callingstringToNode, so this is functionally equivalent and safe.If you ever extend
get_func_typename_infoto allow a length different frompronargs, you might consider returning that length and iterating to it instead, but that's outside the scope of this change.design/dbms_utility/PROPOSAL.md (1)
15-18: Add language identifiers to fenced code blocks.For better Markdown rendering and to satisfy linting rules, add language identifiers to the fenced code blocks. Use
textfor structural diagrams.Apply this diff:
-``` +```text src/pl/plisql/src/ → plisql.so (PL/iSQL language runtime) contrib/ivorysql_ora/ → ivorysql_ora.so (Oracle compatibility extension)```diff -``` +```text contrib/ivorysql_ora/ ├── src/builtin_packages/ │ └── dbms_utility/Also applies to: 63-73
contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.c (1)
176-178: Hardcoded schema name limits accuracy.The hardcoded "PUBLIC" schema may not reflect the actual schema of the function. While the TODO indicates awareness, this could cause confusion when debugging functions in other schemas.
Consider looking up the actual schema from
pg_procusing the function OID if available in the context. Do you want me to generate an implementation that queriespg_procfor the schema name?
📜 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 (21)
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)contrib/uuid-ossp/uuid-ossp.c(2 hunks)design/dbms_utility/PROPOSAL.md(1 hunks)src/backend/catalog/genbki.pl(1 hunks)src/backend/utils/adt/arrayfuncs.c(1 hunks)src/backend/utils/adt/ruleutils.c(2 hunks)src/bin/initdb/initdb.c(1 hunks)src/bin/psql/common.c(5 hunks)src/fe_utils/string_utils.c(1 hunks)src/interfaces/libpq/ivy-exec.c(9 hunks)src/pl/plisql/src/meson.build(1 hunks)src/pl/plisql/src/pl_comp.c(1 hunks)src/pl/plisql/src/pl_exec.c(4 hunks)src/pl/plisql/src/pl_handler.c(1 hunks)src/pl/plisql/src/pl_scanner.c(2 hunks)src/pl/plisql/src/pl_subproc_function.c(1 hunks)src/pl/plisql/src/plisql.h(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/pl/plisql/src/pl_subproc_function.c
🧰 Additional context used
🧬 Code graph analysis (6)
src/pl/plisql/src/plisql.h (1)
src/pl/plisql/src/pl_exec.c (1)
plisql_get_current_exception_context(10034-10041)
src/pl/plisql/src/pl_handler.c (2)
src/backend/oracle_parser/liboracle_parser.c (1)
_PG_fini(84-91)contrib/ivorysql_ora/src/ivorysql_ora.c (1)
_PG_fini(107-119)
src/backend/utils/adt/ruleutils.c (1)
src/backend/utils/cache/lsyscache.c (2)
get_attnum(955-972)get_atttypetypmodcoll(1040-1059)
src/bin/psql/common.c (1)
src/bin/psql/variables.c (1)
BindVariableExist(1080-1108)
src/pl/plisql/src/pl_comp.c (1)
src/backend/utils/fmgr/funcapi.c (1)
get_func_typename_info(1889-1933)
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)
🔇 Additional comments (13)
src/pl/plisql/src/meson.build (1)
12-12: Addition ofpl_exception_type.cto build looks correctThe new source file is properly included in
plisql_sources, so it will be compiled and linked into theplisqlshared module as intended. No issues from the Meson side.src/backend/utils/adt/ruleutils.c (1)
14313-14456: Innerrelidshadowing fix looks correctThe new
Oid relid_1and corresponding updates inRangeVarGetRelid,get_attnum, andget_atttypetypmodcollcleanly remove the previous shadowing of the outerrelid. All inner uses have been updated consistently, outerrelidsemantics are preserved, and there is no behavior change — just improved clarity and fewer pitfalls for static analysis.src/fe_utils/string_utils.c (1)
107-115: LGTM! Good optimization for Oracle mode.Reordering the Oracle mode check before
getLocalPQExpBuffer()avoids unnecessary buffer allocation/reset when the function returns early viaora_fmtId(). This improves performance in the Oracle code path without affecting correctness.src/bin/psql/common.c (2)
3604-3619: LGTM! Variable renaming improves clarity.The loop index change from
itojis a good refactoring that avoids shadowing with theivariable declared later at line 3644. This makes the code clearer and prevents potential confusion.
3698-3698: LGTM! Explicit initialization and separate loop variable improve code quality.The changes here are good defensive programming practices:
- Explicit initialization of
iat line 3698 ensures the variable is properly initialized- Using a separate variable
jfor the inner loop (lines 3819-3830) avoids reusingiand improves clarity- The backward iteration logic with array access
[j-1]is correctly boundedAlso applies to: 3805-3830
contrib/uuid-ossp/uuid-ossp.c (1)
561-595: OSSP-only declarations moved to top of function are correct and behavior‑preservingLifting
uuid_t *uuid;anduuid_rc_t rc;under#ifdef HAVE_UUID_OSSPto the top ofora_sys_guid(before the first statement) resolves C90 “declaration after statement” issues while keeping them scoped to the OSSP build and used only in the OSSP branch. No functional changes are introduced, and all code paths still compile cleanly under the existing#ifdefmatrix.src/pl/plisql/src/pl_handler.c (1)
228-234: Forward declaration of_PG_finiis consistent and harmlessPrototype matches the definition below and doesn’t alter behavior; it just tightens declarations for compilers.
src/interfaces/libpq/ivy-exec.c (1)
2997-3044: IvyaddValueToList local variable reorg is safeDeclaring
pre,tmp, andfindat the top and using them in the traversal keeps semantics the same and correctly leavesprepointing at the last node when appending.src/bin/initdb/initdb.c (1)
2092-2098: Correctly scopesivorysql.compatible_modearoundplisqlextension creationTemporarily switching
ivorysql.compatible_modetooracleforCREATE EXTENSION plisqland then back topgis a good way to allow Oracle‑style constructs in the extension SQL without changing the long‑term default for template1. This is consistent with howload_ivorysql_orais handled.contrib/ivorysql_ora/ivorysql_ora_merge_sqls (1)
1-5: dbms_utility module correctly added to merge listIncluding
src/builtin_packages/dbms_utility/dbms_utilityhere wires the new package into the merged ivorysql_ora SQL as expected; path and naming look consistent with the rest of the tree.src/pl/plisql/src/pl_exec.c (1)
10034-10041:plisql_get_current_exception_contextAPI shape and usage look correctThe helper cleanly exposes the active handler’s
cur_error->contextas aconst char *without transferring ownership, and the NULL checks ensure callers getNULLwhen no handler is active. With the xact‑level reset suggested above, this interface should be safe and sufficient forDBMS_UTILITY.FORMAT_ERROR_BACKTRACE.contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility--1.0.sql (1)
9-26: DBMS_UTILITY wrapper and package definition look consistentThe SQL wrapper correctly exposes the C entry point
ora_format_error_backtraceassys.ora_format_error_backtrace()and then surfaces it viadbms_utility.FORMAT_ERROR_BACKTRACEwith matching signature. VOLATILE is appropriate given dependence on current exception state.contrib/ivorysql_ora/sql/dbms_utility.sql (1)
1-270: Comprehensive test coverage.The test suite provides excellent coverage of FORMAT_ERROR_BACKTRACE functionality:
- Basic and nested exception scenarios (Tests 1-3)
- Function and procedure calls (Test 4)
- Anonymous blocks (Test 5)
- NULL handling when no exception exists (Test 6)
- Multi-level exception propagation (Test 7)
- Package-scoped procedures (Test 8)
- Schema-qualified calls (Test 9)
- Context preservation across nested handlers (Test 10)
The tests are well-structured with proper setup/teardown, making them maintainable and reliable.
| #else | ||
| /* | ||
| * On Windows, we'd need to use GetProcAddress with the module handle. | ||
| * For now, just return NULL - this feature requires plisql. | ||
| */ | ||
| fn = NULL; | ||
| #endif |
There was a problem hiding this comment.
Windows support is incomplete.
The Windows implementation currently returns NULL, making FORMAT_ERROR_BACKTRACE unavailable on Windows platforms. While this is acceptable for an initial implementation, consider documenting this limitation in user-facing documentation.
🤖 Prompt for AI Agents
In contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.c around
lines 76 to 82, the Windows branch currently sets fn = NULL which makes
FORMAT_ERROR_BACKTRACE unavailable on Windows; update the project documentation
(README, platform support docs, and any user-facing changelog) to explicitly
state that FORMAT_ERROR_BACKTRACE is not supported on Windows and explain the
reason (requires plisql/GetProcAddress support), include a note about future
work to add Windows support and any workaround (e.g., use on POSIX systems or
build with plisql), and add a short comment in the source near these lines
pointing to the documentation entry.
| 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 data not freed.
The result.data allocated by initStringInfo is not freed after cstring_to_text copies it. This causes a memory leak on every invocation of FORMAT_ERROR_BACKTRACE.
Apply this diff to fix the leak:
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);
}📝 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); | |
| 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 236 to 254, the StringInfo buffer initialized by initStringInfo(&result)
is not freed after converting to a Text datum, causing a memory leak; fix by
assigning the result of cstring_to_text(result.data) to a Text *tmp (or
similar), free the result.data (or call freeStringInfo(&result)) before
returning, and then PG_RETURN_TEXT_P(tmp) so the buffer is released after the
copy is made.
| return false; | ||
| if (p == q) /* no digits? */ | ||
| return(escontext, false, | ||
| ereturn(escontext, false, |
There was a problem hiding this comment.
Unintended trailing whitespace added.
This change adds trailing whitespace after the comma, which appears unrelated to the PR's objective of implementing FORMAT_ERROR_BACKTRACE. This may be an accidental edit or formatting artifact.
Apply this diff to remove the trailing whitespace:
- ereturn(escontext, false,
+ ereturn(escontext, false,📝 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.
| ereturn(escontext, false, | |
| ereturn(escontext, false, |
🤖 Prompt for AI Agents
In src/backend/utils/adt/arrayfuncs.c around line 468, there's an unintended
trailing whitespace after the comma in the ereturn call; remove the extra space
character so the comma is immediately followed by the next token (i.e., delete
the stray whitespace), ensuring no other code changes are made.
| /* | ||
| * Pointer to the estate currently handling an exception. This is used by | ||
| * DBMS_UTILITY.FORMAT_ERROR_BACKTRACE to access the exception context from | ||
| * within the exception handler, even when nested function calls occur. | ||
| */ | ||
| static PLiSQL_execstate *exception_handling_estate = NULL; | ||
|
|
There was a problem hiding this comment.
Reset exception_handling_estate on transaction end to avoid dangling estate pointer
The global exception_handling_estate is correctly treated as a session‑wide “current handler” pointer and the save/restore logic in exec_stmt_block() looks stack‑like and fine for normal and nested handlers. However, its lifetime is tied to the stack‑allocated PLiSQL_execstate in plisql_exec_function / trigger entrypoints, and it is never cleared in plisql_xact_cb().
If an error is raised inside an exception handler and not caught by any inner PL/iSQL block, control unwinds out of the function without executing the restore, leaving exception_handling_estate pointing at an estate whose stack frame will be gone once the error reaches the top‑level caller. On transaction abort, plisql_xact_cb() currently clears only the simple‑expression state; exception_handling_estate remains non‑NULL and may still point at freed memory. A subsequent call to DBMS_UTILITY.FORMAT_ERROR_BACKTRACE in a fresh transaction but outside any handler would then call plisql_get_current_exception_context() and dereference a dangling estate pointer.
Recommend explicitly resetting exception_handling_estate in the xact callback for both commit and abort events so each new transaction starts with a clean state:
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;
+ }
+
+ /* In all cases, no handler should be considered active after xact end. */
+ exception_handling_estate = NULL;
}This keeps plisql_get_current_exception_context() safe to call in later transactions and preserves the intended “only valid while in a handler” contract.
Also applies to: 1948-2113, 9475-9507, 10034-10041
🤖 Prompt for AI Agents
In src/pl/plisql/src/pl_exec.c around lines 117 to 123, the global
exception_handling_estate can become a dangling pointer after errors unwind past
the stack frame of the estate; modify the transaction callback (plisql_xact_cb /
the xact callback that currently clears simple-expression state on commit/abort)
to also explicitly set exception_handling_estate = NULL for both commit and
abort paths so the global is cleared at transaction end and
FORMAT_ERROR_BACKTRACE cannot dereference freed memory.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/pl/plisql/src/pl_exec.c (1)
117-123: Clearexception_handling_estateon transaction end to avoid dangling estate pointer
exception_handling_estateis a global pointing at the currently active handler’sPLiSQL_execstateand is stack‑allocated in the various entrypoints. In the happy path you save and restore it around the handler body inexec_stmt_block()(Lines 1948, 2100–2113), so nested handlers behave stack‑like.If an error is raised from within a handler body and is not caught by an inner
EXCEPTIONblock, control exits theexec_stmts(estate, exception->action)call via error before the post‑handler restore executes. The transaction then unwinds, the correspondingPLiSQL_execstatestack frame is gone, butexception_handling_estatecontinues to point at the freed estate. On transaction abort,plisql_xact_cb()currently only clears simple‑expression state and never nullsexception_handling_estate, so a later call toplisql_get_current_exception_context/message/sqlerrcodeorplisql_get_call_stackin a fresh transaction can dereference a dangling pointer and crash or misbehave.This lifetime issue was already flagged in an earlier review; the global is still not reset at xact end. Please clear it unconditionally in
plisql_xact_cb()so each new transaction starts with no active handler:void plisql_xact_cb(XactEvent event, void *arg) { @@ - 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; - } + 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 handler should be considered active after transaction end. */ + exception_handling_estate = NULL; }This keeps the new public helpers safe to call in later transactions and enforces the intended “only valid while executing an exception handler” contract.
Also applies to: 1948-2113, 9475-9508
contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.c (1)
267-267: Memory leak: StringInfo data not freed.The past review comment correctly identified that
result.dataallocated byinitStringInfois not freed aftercstring_to_textcopies it, causing a memory leak.
🧹 Nitpick comments (1)
contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.c (1)
190-192: Consider implementing actual schema lookup.The code currently hardcodes "PUBLIC" as the schema name. While acceptable for initial implementation, looking up the actual schema from
pg_procwould improve Oracle compatibility and accuracy of backtraces.
📜 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 (5)
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)src/pl/plisql/src/pl_exec.c(4 hunks)src/pl/plisql/src/plisql.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pl/plisql/src/plisql.h
- contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility--1.0.sql
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-28T00:28:54.063Z
Learnt from: rophy
Repo: rophy/IvorySQL PR: 9
File: src/pl/plisql/src/sql/plisql_autonomous.sql:301-317
Timestamp: 2025-11-28T00:28:54.063Z
Learning: In PostgreSQL regression tests executed via pg_regress, errors do not abort script execution - they are captured in the expected output. The `\set ON_ERROR_STOP` directives are only needed for interactive psql sessions and should be commented out in regression test files like `plisql_autonomous.sql`.
Applied to files:
src/pl/plisql/src/pl_exec.c
🧬 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)
🔇 Additional comments (2)
src/pl/plisql/src/pl_exec.c (1)
10025-10126: New exception/call‑stack helper APIs look consistent with existing PL/iSQL stateThe four new helpers correctly:
- Guard against missing handler state by checking
exception_handling_estateandcur_errorbefore dereferencing.- Return
NULL/0when no handler is active, which aligns with the documented “no exception context exists” behavior.- Build the call stack by walking
error_context_stackand filtering onplisql_exec_error_callback, which matches how PL/iSQL frames are already identified elsewhere.Given the xact‑end reset noted above, these entry points look usable and low‑risk as the plumbing for DBMS_UTILITY.
contrib/ivorysql_ora/sql/dbms_utility.sql (1)
1-441: Comprehensive test coverage.The test suite is well-structured and thorough, covering:
- All three DBMS_UTILITY functions (FORMAT_ERROR_BACKTRACE, FORMAT_ERROR_STACK, FORMAT_CALL_STACK)
- Multiple scenarios: basic exceptions, nested calls, deep nesting, functions, anonymous blocks, packages, schema-qualified calls
- Edge cases: no-error scenarios, nested exception handlers with context preservation
- Integration test verifying all functions work together
The test patterns (create → call → drop) are clean and properly isolated.
| 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 data not freed.
Similar to the issue in ora_format_error_backtrace, the result.data allocated by initStringInfo is not freed after cstring_to_text copies it. This causes a memory leak on every invocation.
Apply this diff to fix the leak:
/* Format: ORA-XXXXX: <message> */
appendStringInfo(&result, "%s: %s\n", sqlstate_to_ora_errnum(sqlerrcode), message);
- 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);
}📝 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); | |
| /* Format: ORA-XXXXX: <message> */ | |
| appendStringInfo(&result, "%s: %s\n", sqlstate_to_ora_errnum(sqlerrcode), message); | |
| PG_RETURN_TEXT_P(cstring_to_text(result.data)); | |
| initStringInfo(&result); | |
| /* Format: ORA-XXXXX: <message> */ | |
| appendStringInfo(&result, "%s: %s\n", sqlstate_to_ora_errnum(sqlerrcode), message); | |
| 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 335-340, the StringInfo 'result' allocated by initStringInfo has its .data
copied into a new text datum by cstring_to_text but the original result.data is
never freed; change the code to first assign the copied text to a local variable
(e.g. text *res = cstring_to_text(result.data);), then free the original buffer
(pfree(result.data);), and finally return the text via PG_RETURN_TEXT_P(res);
ensuring result.data is released 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 data not freed in success path.
The function frees result.data when returning NULL (line 461) but not when returning a text value (line 465). This inconsistency causes a memory leak on successful invocations.
Apply this diff to fix the leak:
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));
+ text *result_text = cstring_to_text(result.data);
+ pfree(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(); | |
| } | |
| text *result_text = cstring_to_text(result.data); | |
| pfree(result.data); | |
| PG_RETURN_TEXT_P(result_text); |
🤖 Prompt for AI Agents
contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.c around
lines 383-465: the StringInfo result.data is pfree'd only on the NULL return
path, leaking memory when the function returns a text value; fix by converting
result.data to a text datum, pfree'ing result.data, then return the text datum
(i.e. call cstring_to_text on result.data, store the returned text pointer,
pfree result.data, and then PG_RETURN_TEXT_P the stored text).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contrib/ivorysql_ora/meson.buildsrc/pl/plisql/src/meson.build
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pl/plisql/src/meson.build
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: contrib_regression
- GitHub Check: pg_regression (ubuntu-latest)
- GitHub Check: oracle_regression (ubuntu-latest)
- GitHub Check: oracle_pg_regression (ubuntu-latest)
- GitHub Check: meson_build (ubuntu-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (2)
contrib/ivorysql_ora/meson.build (2)
29-29: LGTM! New DBMS_UTILITY source file added correctly.The addition of
dbms_utility.cto the source list follows the existing pattern and aligns with the PR objectives.
69-84: All regression test files are present and properly configured.Verification confirms that all 16 regression tests referenced in lines 69-84 have corresponding SQL files in
contrib/ivorysql_ora/sql/and expected output files incontrib/ivorysql_ora/expected/. Sincemeson.buildis a new file being added (not modifying an existing test configuration), no critical test coverage is being removed. The test list appropriately focuses on Oracle-compatible functionality with the newly addeddbms_utilitytest included.
|
|
||
| ivorysql_ora = shared_module('ivorysql_ora', | ||
| ivorysql_ora_sources, | ||
| include_directories: plisql_inc, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "plisql_inc" src/pl/plisql/src/meson.buildRepository: rophy/IvorySQL
Length of output: 136
🏁 Script executed:
# Check the contrib/ivorysql_ora/meson.build file context around line 48
head -60 contrib/ivorysql_ora/meson.build | tail -20Repository: rophy/IvorySQL
Length of output: 447
🏁 Script executed:
# Search for plisql_inc definition/export across the entire codebase
rg -n "plisql_inc\s*=" --type mesonRepository: rophy/IvorySQL
Length of output: 128
🏁 Script executed:
# Check if src/pl/plisql/src/meson.build is included in parent files
rg -n "subdir.*plisql" src/pl/meson.buildRepository: rophy/IvorySQL
Length of output: 76
🏁 Script executed:
# Check the top-level meson.build for subdirectory includes
rg -n "subdir" meson.build | head -20Repository: rophy/IvorySQL
Length of output: 431
🏁 Script executed:
# Look at src/pl/meson.build to understand the structure
cat -n src/pl/meson.buildRepository: rophy/IvorySQL
Length of output: 331
🏁 Script executed:
# Check if plisql_inc is exported from src/pl/plisql/src/meson.build
cat -n src/pl/plisql/src/meson.build | tail -30Repository: rophy/IvorySQL
Length of output: 791
🏁 Script executed:
# Check how contrib/ivorysql_ora/meson.build accesses plisql_inc
head -60 contrib/ivorysql_ora/meson.buildRepository: rophy/IvorySQL
Length of output: 1912
🏁 Script executed:
# Search for any declare_dependency or variable declarations involving plisql_inc
rg -B2 -A2 "plisql_inc" src/pl/plisql/src/meson.buildRepository: rophy/IvorySQL
Length of output: 262
🏁 Script executed:
# Check if plisql_inc is re-exported from parent meson.build files
rg -n "plisql_inc" src/pl/meson.buildRepository: rophy/IvorySQL
Length of output: 40
🏁 Script executed:
# Check src/meson.build for plisql_inc
rg -n "plisql_inc" src/meson.buildRepository: rophy/IvorySQL
Length of output: 40
🏁 Script executed:
# Check if contrib can access variables from src/pl subdirs
# Look for any mechanism to share plisql_inc at top level or contrib level
rg -n "plisql_inc" contrib/meson.build 2>/dev/null || echo "contrib/meson.build not found or no matches"Repository: rophy/IvorySQL
Length of output: 101
🏁 Script executed:
# Check the actual meson.build file hierarchy - look for where src/pl/plisql is included
cat -n src/pl/meson.build | grep -A5 -B5 "plisql"Repository: rophy/IvorySQL
Length of output: 331
🏁 Script executed:
# Check if plisql_inc is defined/exported at the top-level src directory
cat -n src/meson.build | grep -i "plisql_inc" || echo "No plisql_inc in src/meson.build"Repository: rophy/IvorySQL
Length of output: 90
🏁 Script executed:
# Check where plisql is included from contrib perspective
# Look for any imports or dependencies from contrib toward src/pl
cat contrib/ivorysql_ora/meson.build | head -100Repository: rophy/IvorySQL
Length of output: 2486
🏁 Script executed:
# Verify the exact scope: check where plisql_inc is first used before line 48
grep -n "plisql_inc" contrib/ivorysql_ora/meson.buildRepository: rophy/IvorySQL
Length of output: 95
Critical build error: plisql_inc is not accessible in contrib scope.
The variable plisql_inc is defined locally in src/pl/plisql/src/meson.build:55 but is never exported to the parent scope. In Meson, variables defined in subdirectories are not automatically visible to sibling directories. The reference to plisql_inc in contrib/ivorysql_ora/meson.build:48 will result in an undefined variable error at build time. The variable must be either re-exported from src/pl/plisql/src/meson.build to a shared scope accessible to contrib, or replaced with an absolute path to the include directory.
🤖 Prompt for AI Agents
In contrib/ivorysql_ora/meson.build around line 48, the include_directories:
plisql_inc reference fails because plisql_inc is defined only in
src/pl/plisql/src/meson.build and not visible in contrib; fix by either (A)
exporting the include dir from src/pl/plisql/src/meson.build to a variable in a
parent scope (e.g., define plisql_inc =
include_directories(meson.current_source_dir() / 'include') in a meson.build at
src/pl/plisql or the top-level meson.build and use that exported variable), or
(B) replace plisql_inc in contrib/ivorysql_ora/meson.build with an explicit
absolute or project-relative include_directories(path_to_plisql_include) such as
include_directories(meson.normalize_path('src/pl/plisql/include')) so the
contrib build can resolve the header path.
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
Move DBMS_UTILITY package from plisql to contrib/ivorysql_ora following the IvorySQL built-in package convention. Changes: - Add dbms_utility.c and dbms_utility--1.0.sql to contrib/ivorysql_ora/src/builtin_packages/dbms_utility/ - Use dynamic symbol lookup (dlsym) to call plisql API, avoiding link-time dependency between extensions - Keep minimal API in plisql: plisql_get_current_exception_context() exports exception context for cross-module access - Update Makefile and ivorysql_ora_merge_sqls for new package - Move regression tests to contrib/ivorysql_ora/sql and expected/ - Remove DBMS_UTILITY package definition from plisql--1.0.sql The dynamic lookup approach allows ivorysql_ora.so and plisql.so to be loaded in any order, resolving the symbol at runtime when the function is actually called from within a PL/iSQL exception handler.
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
- Add plisql_inc export in src/pl/plisql/src/meson.build - Add dbms_utility.c source to contrib/ivorysql_ora - Add plisql include directory for plisql.h header - Update regress test list to match Makefile ORA_REGRESS
rophy
force-pushed
the
feat/dbms_utility
branch
from
2043e1a to
8e9490c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
contrib/ivorysql_ora/meson.build (1)
48-48: Cross-module include dependency onplisql_inc.This references
plisql_incdefined insrc/pl/plisql/src/meson.build. As noted in a previous review, this requires that the plisql subdirectory is processed before contrib in Meson's build order. If the build fails with an undefined variable error, consider using an explicit path instead:include_directories: include_directories('../../src/pl/plisql/src')src/pl/plisql/src/pl_exec.c (1)
118-123: Globalexception_handling_estatecan dangle across transactions; clear it inplisql_xact_cb()
exception_handling_estateis a backend‑global pointer to thePLiSQL_execstatecurrently handling an exception. It is set in theexec_stmt_block()PG_CATCH path before running the handler body and restored afterward, but there are error paths where the restore never executes (e.g., an error raised inside the handler that is not caught by an inner PL/iSQL block and is rethrown out ofexec_stmt_block). In those cases, control unwinds out of the function whileexception_handling_estatestill points at the stack‑allocated estate.At transaction end,
plisql_xact_cb()currently only clears the simple expression EState and resowner; it does not touchexception_handling_estate. After such a failure/abort, the next transaction can callplisql_get_current_exception_context()/plisql_get_current_exception_message()/plisql_get_current_exception_sqlerrcode(), which will dereferenceexception_handling_estate->cur_errorthrough a stale estate pointer and hit freed memory. That’s the dangling‑pointer scenario previously flagged and is still present.You don’t need to redesign the handler logic to fix the cross‑transaction hazard; it’s sufficient to ensure that no handler is considered active once a transaction ends:
Proposed fix: reset `exception_handling_estate` on all xact end events
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; + } + + /* + * In all cases, no PL/iSQL exception handler should be considered active + * after transaction end; avoid leaving a dangling estate pointer for + * FORMAT_ERROR_* callers. + */ + exception_handling_estate = NULL; }This keeps the “current handler estate” notion strictly transaction‑local and makes the new public APIs safe to call at the start of a fresh transaction.
Also applies to: 1988-1989, 2140-2153, 9515-9548
contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.c (3)
249-267: Memory leak:result.datanot freed.The
initStringInfofunction allocates memory forresult.data. Aftercstring_to_textcopies it, the original buffer leaks.🔎 Proposed fix
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); }
335-340: Memory leak:result.datanot freed.Same issue as in
ora_format_error_backtrace— the StringInfo buffer is not freed aftercstring_to_textcopies it.🔎 Proposed fix
appendStringInfo(&result, "%s: %s\n", sqlstate_to_ora_errnum(sqlerrcode), message); - 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); }
458-465: Memory leak:result.datanot freed in success path.The function correctly frees
result.datawhen returning NULL (line 461) but leaks it when returning a text value (line 465).🔎 Proposed fix
if (!found_any) { pfree(result.data); PG_RETURN_NULL(); } - 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); }
🧹 Nitpick comments (3)
design/dbms_utility/PROPOSAL.md (3)
15-18: Add language specifier to fenced code block.The static analysis tool flagged this block as missing a language specifier. Consider adding
textfor plain text/ASCII diagrams:-``` +```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.** Similar to above, this file structure block should have a language specifier: ```diff -``` +```text contrib/ivorysql_ora/ ├── src/builtin_packages/ ...
144-145: Update author placeholder.The author placeholder should be filled in with the actual contributor name.
📜 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 (3)
- contrib/ivorysql_ora/ivorysql_ora_merge_sqls
- contrib/ivorysql_ora/Makefile
- contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility--1.0.sql
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-28T00:28:54.087Z
Learnt from: rophy
Repo: rophy/IvorySQL PR: 9
File: src/pl/plisql/src/sql/plisql_autonomous.sql:301-317
Timestamp: 2025-11-28T00:28:54.087Z
Learning: In PostgreSQL regression tests executed via pg_regress, errors do not abort script execution - they are captured in the expected output. The `\set ON_ERROR_STOP` directives are only needed for interactive psql sessions and should be commented out in regression test files like `plisql_autonomous.sql`.
Applied to files:
src/pl/plisql/src/pl_exec.c
🧬 Code graph analysis (1)
contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.c (3)
src/backend/utils/mmgr/mcxt.c (2)
pnstrdup(1713-1725)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). (5)
- GitHub Check: meson_build (ubuntu-latest)
- GitHub Check: oracle_pg_regression (ubuntu-latest)
- GitHub Check: pg_regression (ubuntu-latest)
- GitHub Check: contrib_regression
- GitHub Check: oracle_regression (ubuntu-latest)
🔇 Additional comments (18)
src/pl/plisql/src/Makefile (1)
66-66: LGTM!Adding
dbms_utilityto the regression test list properly integrates the new functionality into PL/iSQL's test suite.src/bin/initdb/initdb.c (1)
2094-2097: LGTM!The compatibility mode switching follows the same pattern as
load_ivorysql_ora()(lines 2113-2117) and correctly scopes the Oracle mode to just the extension creation where CREATE PACKAGE statements require it.contrib/ivorysql_ora/meson.build (2)
29-29: LGTM - new DBMS_UTILITY source file.Correctly adds the new C implementation file for DBMS_UTILITY package.
69-93: Test configuration looks correct.The regression test list properly includes
dbms_utilityalongside other ora_* tests, and the tap test structure follows the expected nested format.src/pl/plisql/src/plisql.h (1)
1464-1471: Well-designed minimal API for cross-module exception context access.The four new functions provide a clean interface for DBMS_UTILITY to access PL/iSQL exception state. The
PGDLLEXPORTannotations are correct for cross-module calls.Regarding
plisql_get_call_stack()returning non-constchar*: The implementation documentation at pl_exec.c:10121 explicitly states "The returned string is palloc'd in the current memory context," which properly documents that the caller owns the returned memory and must free it. The implementation correctly uses PostgreSQL'sStringInfoDataandpallocfor allocation, following standard conventions.src/pl/plisql/src/pl_exec.c (2)
10065-10111: New exception accessors correctly reflect current handler state; rely on caller to copy stringsThe three helpers:
plisql_get_current_exception_context()plisql_get_current_exception_message()plisql_get_current_exception_sqlerrcode()correctly guard on
exception_handling_estate != NULLandcur_error != NULLand then directly expose the underlyingErrorDatafields. That matches howGET STACKED DIAGNOSTICSusesestate->cur_errorand is enough for the DBMS_UTILITY wrappers, assuming they copy the returned strings into their own context.Once
exception_handling_estateis reliably cleared at xact end (see previous comment), these accessors look good.
10123-10166:plisql_get_call_stackimplementation is reasonable and PL/iSQL‑scopedWalking
error_context_stackand filtering oncallback == plisql_exec_error_callbackis an appropriate way to derive the active PL/iSQL frames. Emitting"<func_ptr>\t<lineno>\t<fn_signature>"per line gives the ORA‑side enough information to map back to function identity and source locations, and returning NULL when no PL/iSQL frame is present matches the expected “no call stack” semantics.No issues here from the PL/iSQL side; the remaining formatting/translation concerns belong in the DBMS_UTILITY wrapper layer.
contrib/ivorysql_ora/src/builtin_packages/dbms_utility/dbms_utility.c (5)
29-41: LGTM!The header includes and function export declarations are appropriate for PostgreSQL extension development.
43-60: LGTM!The function pointer typedefs and cached static pointers are appropriate for the dynamic lookup pattern. Since PostgreSQL uses process-based concurrency, the static caching is thread-safe in this context.
66-99: LGTM!The dynamic lookup pattern using
RTLD_DEFAULTis appropriate for resolving symbols from already-loaded shared objects. The one-time lookup with caching avoids repeateddlsymoverhead.
116-214: LGTM!The line transformation logic is well-structured with proper memory management. All
pnstrdup/pstrdupallocations are correctly freed before returning. The hardcoded "PUBLIC" schema (line 192) is acknowledged with a TODO comment for future improvement.
275-300: LGTM!The SQLSTATE to Oracle error number mapping covers the most common error scenarios. The default fallback to
ORA-06502is reasonable for unmapped cases.contrib/ivorysql_ora/sql/dbms_utility.sql (6)
1-131: LGTM!Tests 1-5 provide good coverage for
FORMAT_ERROR_BACKTRACEacross basic exceptions, nested procedure calls (3 and 5 levels deep), function calls, and anonymous blocks. Objects are properly cleaned up after each test.
133-144: LGTM!Test 6 correctly verifies the behavior when no exception context exists. The output with brackets will distinguish between NULL and empty values.
146-270: LGTM!Tests 7-10 provide excellent coverage for advanced scenarios including exception re-raising, package procedures, schema-qualified calls, and the important test for nested exception handler context preservation.
272-320: LGTM!Tests 11-13 adequately cover
FORMAT_ERROR_STACKincluding basic exceptions, division by zero (testing SQLSTATE mapping toORA-01476), and the no-exception case.
322-392: LGTM!Tests 14-16 verify
FORMAT_CALL_STACKfunctionality including the header structure, function name presence, nested call depth counting, and behavior within exception handlers. The string-based counting approach on line 353 is pragmatic for test validation.
394-441: LGTM!Test 17 is a valuable integration test that verifies all three
DBMS_UTILITYfunctions work correctly together within the same exception handler context. The pattern-based validation ensures the output format is correct without being overly strict about specific content.
| plisql_inc = include_directories('.') | ||
|
|
||
| plisql = shared_module('plisql', | ||
| plisql_sources, | ||
| c_pch: pch_postgres_h, | ||
| include_directories: include_directories('.'), | ||
| include_directories: plisql_inc, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the processing order of subdirectories in the top-level meson.build
echo "=== Checking top-level meson.build subdir order ==="
grep -n "subdir" meson.build | head -30
echo ""
echo "=== Checking detailed order of src and contrib ==="
awk '/subdir.*src/ {src_line=NR; print "src subdir at line:", NR}
/subdir.*contrib/ {print "contrib subdir at line:", NR; if(src_line && NR>src_line) print "✓ OK: src processed before contrib"; else print "✗ ERROR: contrib processed before src"}' meson.build
echo ""
echo "=== Verifying plisql_inc usage in contrib/ivorysql_ora/meson.build ==="
if [ -f "contrib/ivorysql_ora/meson.build" ]; then
grep -n "plisql_inc" contrib/ivorysql_ora/meson.build
else
echo "File not found: contrib/ivorysql_ora/meson.build"
fiRepository: rophy/IvorySQL
Length of output: 984
🏁 Script executed:
#!/bin/bash
# Check how plisql_inc is propagated through parent scopes
echo "=== Checking src/pl/plisql/src/meson.build (where plisql_inc is defined) ==="
wc -l src/pl/plisql/src/meson.build
echo ""
# Show the context around plisql_inc definition
grep -n "plisql_inc" src/pl/plisql/src/meson.build -B 2 -A 5
echo ""
echo "=== Checking src/pl/plisql/meson.build (parent directory) ==="
if [ -f "src/pl/plisql/meson.build" ]; then
wc -l src/pl/plisql/meson.build
grep -n "plisql_inc" src/pl/plisql/meson.build
grep -n "subdir" src/pl/plisql/meson.build
else
echo "File does not exist"
fi
echo ""
echo "=== Checking src/pl/meson.build ==="
if [ -f "src/pl/meson.build" ]; then
wc -l src/pl/meson.build
grep -n "plisql_inc" src/pl/meson.build
grep -n "subdir" src/pl/meson.build
else
echo "File does not exist"
fi
echo ""
echo "=== Checking src/meson.build for plisql_inc ==="
grep -n "plisql_inc" src/meson.build
echo ""
echo "=== Full context of contrib/ivorysql_ora/meson.build usage ==="
grep -n "plisql_inc" contrib/ivorysql_ora/meson.build -B 5 -A 2Repository: rophy/IvorySQL
Length of output: 1020
🏁 Script executed:
#!/bin/bash
# Check if plisql_inc is returned or exported from src/pl/plisql/src/meson.build
echo "=== Full src/pl/plisql/src/meson.build end section ==="
tail -20 src/pl/plisql/src/meson.build
echo ""
echo "=== Searching for set_variable or return in src/pl/plisql/src/meson.build ==="
grep -n "set_variable\|return\|plisql_inc" src/pl/plisql/src/meson.build | tail -10
echo ""
echo "=== Checking if plisql_inc is defined anywhere in top-level or parent meson files ==="
grep -r "plisql_inc" . --include="meson.build" | grep -v "src/pl/plisql/src" | grep -v "contrib/ivorysql_ora"
echo ""
echo "=== Check contrib/ivorysql_ora/meson.build for imports or variable setup ==="
head -50 contrib/ivorysql_ora/meson.buildRepository: rophy/IvorySQL
Length of output: 2507
Fix undefined plisql_inc variable scope.
The plisql_inc variable is defined locally in src/pl/plisql/src/meson.build:56 and cannot be accessed from contrib/ivorysql_ora/meson.build:48 due to Meson's variable scoping rules. Variables in subdirectories must be explicitly propagated to parent scopes via set_variable() to be accessible in sibling directories. Either define plisql_inc at a common parent level (e.g., src/meson.build or top-level meson.build) or propagate it through the scope hierarchy.
🤖 Prompt for AI Agents
In src/pl/plisql/src/meson.build around lines 56 to 61, plisql_inc is defined
locally and therefore not visible to contrib/ivorysql_ora/meson.build due to
Meson's scope rules; fix by moving the include_directories('.') assignment to a
shared parent meson.build (e.g., src/meson.build or top-level meson.build) or by
calling set_variable('plisql_inc', plisql_inc) after defining it so the variable
is exported to parent/sibling scopes; ensure any files that reference plisql_inc
use the shared name and update include_directories usage accordingly.
Implements Oracle-compatible
DBMS_UTILITY.FORMAT_ERROR_BACKTRACEfunction for retrieving formatted error stack traces.Changes:
Output format:
Returns ORA-06512 formatted backtrace or NULL when no exception context exists.
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.