feat: support return values in autonomous transaction functions#9
feat: support return values in autonomous transaction functions#9
Conversation
- Detect function vs procedure (VOID vs non-VOID return type) - Functions use SELECT via dblink() with SPI to capture return value - Procedures use CALL via dblink_exec() (no return value) - Add test cases for function return values (Test 10, 11) - Document COMMIT/ROLLBACK limitation (issue IvorySQL#985) All 17 tests pass. Matches Oracle behavior for function returns.
WalkthroughDistinguishes autonomous functions from procedures: adds a dblink OID cache and invalidation, changes call generation to use SELECT for functions and CALL for procedures, and adds execute_autonomous_function to run dblink SELECT via SPI and return a copied Datum with proper null semantics. Changes
Sequence DiagramsequenceDiagram
participant Client
participant PLiSQL as "PL/pgSQL Runtime"
participant Cache
participant dblink
participant SPI
Client->>PLiSQL: invoke autonomous call
PLiSQL->>Cache: check `dblink_oid` / `dblink_exec_oid`
alt cache miss
PLiSQL->>dblink: lookup dblink OIDs
dblink-->>PLiSQL: return OIDs
PLiSQL->>Cache: update cache
end
alt detected as function
PLiSQL->>SPI: open SPI session
PLiSQL->>dblink: run dblink SELECT (SQL)
dblink-->>SPI: return result row
SPI-->>PLiSQL: return Datum (copied)
PLiSQL-->>Client: return Datum (fcinfo->isnull = false/true accordingly)
else detected as procedure
PLiSQL->>dblink: call dblink_exec with CALL statement
dblink-->>PLiSQL: execution status
PLiSQL-->>Client: return NULL (fcinfo->isnull = true)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/pl/plisql/src/pl_autonomous.c (1)
431-440: Consider avoiding duplicate syscache lookup.The function looks up
proctuphere to getrettype, butbuild_autonomous_call()performs the same lookup again internally (lines 222-225). This is a minor inefficiency.You could refactor to pass
rettypetobuild_autonomous_call()or have it return bothis_functionandrettype:-static char * -build_autonomous_call(PLiSQL_function *func, FunctionCallInfo fcinfo, bool *is_function) +static char * +build_autonomous_call(PLiSQL_function *func, FunctionCallInfo fcinfo, bool *is_function, Oid *rettype)This would eliminate the duplicate syscache lookup in the caller.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/pl/plisql/src/expected/plisql_autonomous.outis excluded by!**/*.out
📒 Files selected for processing (2)
src/pl/plisql/src/pl_autonomous.c(9 hunks)src/pl/plisql/src/sql/plisql_autonomous.sql(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pl/plisql/src/pl_autonomous.c (5)
src/include/postgres.h (1)
ObjectIdGetDatum(256-260)src/include/access/htup_details.h (1)
GETSTRUCT(751-755)src/backend/parser/parse_func.c (1)
LookupFuncName(2414-2459)src/backend/executor/spi.c (4)
SPI_connect(96-100)SPI_execute(605-636)SPI_getbinval(1278-1292)SPI_finish(186-220)src/backend/utils/cache/lsyscache.c (1)
get_typlenbyval(2668-2681)
🔇 Additional comments (8)
src/pl/plisql/src/sql/plisql_autonomous.sql (3)
197-207: LGTM! Good test coverage for autonomous function return values.The test correctly validates that:
- The function returns the computed value (doubled input)
- The side effect (INSERT) persists in the autonomous transaction
Note: The PRAGMA placement inside
DECLAREblock differs from procedure tests (where it's directly in the procedure body). Both placements are valid in PL/iSQL, but consistency across tests would improve readability.
220-233: Good coverage for NULL return handling.This test ensures that NULL returns from autonomous functions are handled correctly, which is an important edge case for the new
execute_autonomous_function()implementation.
235-296: Appreciated: Documented known limitations with issue references.The commented-out tests for COMMIT/ROLLBACK inside autonomous blocks are valuable documentation. They clearly reference issue IvorySQL#985 and explain the expected behavior vs. actual limitation.
src/pl/plisql/src/pl_autonomous.c (5)
33-52: LGTM! Cache invalidation properly handles both OIDs.The invalidation callback correctly resets both
dblink_exec_oidanddblink_oidwhen pg_proc changes, ensuring stale OIDs won't be used after extension drop/recreate.
227-228: LGTM! Correct detection of function vs procedure.Using
prorettype != VOIDOIDis the standard PostgreSQL pattern to distinguish functions that return values from procedures.
367-374: LGTM! Correct datum handling before SPI cleanup.The code correctly:
- Retrieves type length/byval info
- Copies pass-by-reference datums before
SPI_finish()frees SPI memory- Handles NULL returns without unnecessary copying
This ensures the returned datum remains valid after SPI context is destroyed.
498-520: LGTM! Procedure path has proper error handling.The PG_TRY/PG_CATCH block correctly handles cleanup on error. The allocated datums (
connstr_datum,sql_datum) will be freed when the memory context resets, which is acceptable for this short-lived operation.
470-494: LGTM! Function execution path properly handles cleanup.The PG_TRY/PG_CATCH ensures resources are freed on both success and error paths. The
resultis already safely copied insideexecute_autonomous_function()before SPI cleanup.
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 (1)
src/pl/plisql/src/pl_autonomous.c(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pl/plisql/src/pl_autonomous.c (4)
src/common/stringinfo.c (1)
appendStringInfo(144-166)src/backend/parser/parse_func.c (1)
LookupFuncName(2414-2459)src/backend/executor/spi.c (4)
SPI_connect(96-100)SPI_execute(605-636)SPI_getbinval(1278-1292)SPI_finish(186-220)src/backend/utils/cache/lsyscache.c (1)
get_typlenbyval(2668-2681)
🔇 Additional comments (8)
src/pl/plisql/src/pl_autonomous.c (8)
25-25: LGTM!The addition of
utils/datum.hfordatumCopysupport and the newdblink_oidcache variable follow the established pattern fordblink_exec_oid.Also applies to: 35-35
50-52: LGTM!Correctly invalidates both
dblink_exec_oidanddblink_oidwhen the catalog changes, ensuring no stale function references.
207-230: LGTM!The function/procedure detection logic using
prorettype != VOIDOIDis correct and follows PostgreSQL conventions. The output parameteris_functionproperly communicates the result to the caller.
280-300: LGTM!The SQL generation correctly uses
SELECTfor functions to capture return values andCALLfor procedures. The session settings are properly included in both paths.
340-344: LGTM!The query construction correctly uses
quote_literal_cstrfor string arguments andformat_type_befor the return type. Thedblink()call with typed output columnsAS t(result rettype)follows the proper syntax for returning SETOF record.
371-382: LGTM!The result extraction and type handling is correct.
datumCopyis appropriately called only for non-byval types beforeSPI_finish()frees the SPI memory context. Byval types are safely returned as-is since they're values, not pointers.
451-545: LGTM!The dual execution paths correctly handle functions (using
execute_autonomous_functionto capture return values) and procedures (usingdblink_execwith no return). Both paths have proper error handling withPG_TRY/PG_CATCHblocks and consistent resource cleanup.
328-338: LGTM!The
dblink()function lookup correctly specifies the two-argument signature(text, text)and provides a helpful error message with installation hint if the function is not found.
| PG_TRY(); | ||
| { | ||
| /* Connect to SPI */ | ||
| ret = SPI_connect(); | ||
| if (ret < 0) | ||
| ereport(ERROR, | ||
| (errcode(ERRCODE_INTERNAL_ERROR), | ||
| errmsg("could not connect to SPI for autonomous function execution"), | ||
| errdetail("SPI_connect returned %d", ret))); | ||
|
|
||
| /* Execute the query */ | ||
| ret = SPI_execute(query, true, 1); | ||
| if (ret != SPI_OK_SELECT) | ||
| ereport(ERROR, | ||
| (errcode(ERRCODE_INTERNAL_ERROR), | ||
| errmsg("autonomous function execution failed"), | ||
| errdetail("SPI_execute returned %d", ret))); | ||
|
|
||
| if (SPI_processed != 1) | ||
| ereport(ERROR, | ||
| (errcode(ERRCODE_INTERNAL_ERROR), | ||
| errmsg("autonomous function returned unexpected number of rows: %lu", | ||
| (unsigned long) SPI_processed))); | ||
|
|
||
| /* Extract return value from result */ | ||
| result = SPI_getbinval(SPI_tuptable->vals[0], | ||
| SPI_tuptable->tupdesc, | ||
| 1, | ||
| &isnull); | ||
|
|
||
| /* Get type information for datumCopy */ | ||
| get_typlenbyval(rettype, &typlen, &typbyval); | ||
|
|
||
| /* Copy result to upper context before SPI_finish frees SPI memory */ | ||
| if (!isnull && !typbyval) | ||
| result = datumCopy(result, typbyval, typlen); | ||
|
|
||
| SPI_finish(); | ||
| } | ||
| PG_CATCH(); | ||
| { | ||
| /* Clean up on error */ | ||
| SPI_finish(); | ||
| pfree(query); | ||
| PG_RE_THROW(); | ||
| } | ||
| PG_END_TRY(); |
There was a problem hiding this comment.
SPI_finish called when SPI not connected.
In the PG_CATCH block (line 389), SPI_finish() is called unconditionally, even if SPI_connect() failed at line 350. While SPI_finish() returns an error code rather than crashing when called without an active connection, this is still incorrect practice.
Consider tracking the connection state or restructuring with a flag:
/* Execute via SPI with proper error handling */
+ bool spi_connected = false;
+
PG_TRY();
{
/* Connect to SPI */
ret = SPI_connect();
if (ret < 0)
ereport(ERROR,
(errcode(ERRCODE_INTERNAL_ERROR),
errmsg("could not connect to SPI for autonomous function execution"),
errdetail("SPI_connect returned %d", ret)));
+ spi_connected = true;
/* Execute the query */
ret = SPI_execute(query, true, 1);
if (ret != SPI_OK_SELECT)
ereport(ERROR,
(errcode(ERRCODE_INTERNAL_ERROR),
errmsg("autonomous function execution failed"),
errdetail("SPI_execute returned %d", ret)));
if (SPI_processed != 1)
ereport(ERROR,
(errcode(ERRCODE_INTERNAL_ERROR),
errmsg("autonomous function returned unexpected number of rows: %lu",
(unsigned long) SPI_processed)));
/* Extract return value from result */
result = SPI_getbinval(SPI_tuptable->vals[0],
SPI_tuptable->tupdesc,
1,
&isnull);
/* Get type information for datumCopy */
get_typlenbyval(rettype, &typlen, &typbyval);
/* Copy result to upper context before SPI_finish frees SPI memory */
if (!isnull && !typbyval)
result = datumCopy(result, typbyval, typlen);
SPI_finish();
+ spi_connected = false;
}
PG_CATCH();
{
/* Clean up on error */
- SPI_finish();
+ if (spi_connected)
+ SPI_finish();
pfree(query);
PG_RE_THROW();
}
PG_END_TRY();📝 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.
| PG_TRY(); | |
| { | |
| /* Connect to SPI */ | |
| ret = SPI_connect(); | |
| if (ret < 0) | |
| ereport(ERROR, | |
| (errcode(ERRCODE_INTERNAL_ERROR), | |
| errmsg("could not connect to SPI for autonomous function execution"), | |
| errdetail("SPI_connect returned %d", ret))); | |
| /* Execute the query */ | |
| ret = SPI_execute(query, true, 1); | |
| if (ret != SPI_OK_SELECT) | |
| ereport(ERROR, | |
| (errcode(ERRCODE_INTERNAL_ERROR), | |
| errmsg("autonomous function execution failed"), | |
| errdetail("SPI_execute returned %d", ret))); | |
| if (SPI_processed != 1) | |
| ereport(ERROR, | |
| (errcode(ERRCODE_INTERNAL_ERROR), | |
| errmsg("autonomous function returned unexpected number of rows: %lu", | |
| (unsigned long) SPI_processed))); | |
| /* Extract return value from result */ | |
| result = SPI_getbinval(SPI_tuptable->vals[0], | |
| SPI_tuptable->tupdesc, | |
| 1, | |
| &isnull); | |
| /* Get type information for datumCopy */ | |
| get_typlenbyval(rettype, &typlen, &typbyval); | |
| /* Copy result to upper context before SPI_finish frees SPI memory */ | |
| if (!isnull && !typbyval) | |
| result = datumCopy(result, typbyval, typlen); | |
| SPI_finish(); | |
| } | |
| PG_CATCH(); | |
| { | |
| /* Clean up on error */ | |
| SPI_finish(); | |
| pfree(query); | |
| PG_RE_THROW(); | |
| } | |
| PG_END_TRY(); | |
| bool spi_connected = false; | |
| PG_TRY(); | |
| { | |
| /* Connect to SPI */ | |
| ret = SPI_connect(); | |
| if (ret < 0) | |
| ereport(ERROR, | |
| (errcode(ERRCODE_INTERNAL_ERROR), | |
| errmsg("could not connect to SPI for autonomous function execution"), | |
| errdetail("SPI_connect returned %d", ret))); | |
| spi_connected = true; | |
| /* Execute the query */ | |
| ret = SPI_execute(query, true, 1); | |
| if (ret != SPI_OK_SELECT) | |
| ereport(ERROR, | |
| (errcode(ERRCODE_INTERNAL_ERROR), | |
| errmsg("autonomous function execution failed"), | |
| errdetail("SPI_execute returned %d", ret))); | |
| if (SPI_processed != 1) | |
| ereport(ERROR, | |
| (errcode(ERRCODE_INTERNAL_ERROR), | |
| errmsg("autonomous function returned unexpected number of rows: %lu", | |
| (unsigned long) SPI_processed))); | |
| /* Extract return value from result */ | |
| result = SPI_getbinval(SPI_tuptable->vals[0], | |
| SPI_tuptable->tupdesc, | |
| 1, | |
| &isnull); | |
| /* Get type information for datumCopy */ | |
| get_typlenbyval(rettype, &typlen, &typbyval); | |
| /* Copy result to upper context before SPI_finish frees SPI memory */ | |
| if (!isnull && !typbyval) | |
| result = datumCopy(result, typbyval, typlen); | |
| SPI_finish(); | |
| spi_connected = false; | |
| } | |
| PG_CATCH(); | |
| { | |
| /* Clean up on error */ | |
| if (spi_connected) | |
| SPI_finish(); | |
| pfree(query); | |
| PG_RE_THROW(); | |
| } | |
| PG_END_TRY(); |
🤖 Prompt for AI Agents
In src/pl/plisql/src/pl_autonomous.c around lines 347 to 393, the PG_CATCH block
unconditionally calls SPI_finish even if SPI_connect failed; add a local boolean
(e.g., bool spi_connected = false) initialized false, set it true immediately
after successful SPI_connect, and replace unconditional SPI_finish() calls with
conditional calls (if (spi_connected) SPI_finish()); ensure both normal and
error paths respect this flag so SPI_finish is only invoked when a connection
was actually established.
- Check SPI_connect() return value before proceeding - Wrap SPI operations in PG_TRY/PG_CATCH to prevent memory leaks - Add utils/datum.h include for datumCopy() function - Fix memory context for pass-by-reference types (TEXT, NUMERIC) - Switch to function's memory context before datumCopy() Test coverage additions: - Test 15: Error handling (PG_CATCH cleanup verification) - Test 16: TEXT return (pass-by-reference, found and fixed bug) - Test 17: Nested autonomous calls (multiple SPI contexts) - Test 18: NUMERIC return (variable-length pass-by-reference) - Test 19: DATE return (pass-by-value temporal type) - Test 20: BOOLEAN return (1-byte pass-by-value type) All 20 tests pass.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/pl/plisql/src/pl_autonomous.c (1)
395-401: SPI_finish called when SPI not connected.The
PG_CATCHblock (line 398) callsSPI_finish()unconditionally, even ifSPI_connect()failed at line 351. WhileSPI_finish()returns an error code rather than crashing when called without an active connection, this is still incorrect practice.The previous review suggested tracking the connection state with a boolean flag, and this was marked as addressed in commit a591a48, but the current code does not implement the flag.
Consider tracking the connection state:
/* Execute via SPI with proper error handling */ + bool spi_connected = false; + PG_TRY(); { /* Connect to SPI */ ret = SPI_connect(); if (ret < 0) ereport(ERROR, (errcode(ERRCODE_INTERNAL_ERROR), errmsg("could not connect to SPI for autonomous function execution"), errdetail("SPI_connect returned %d", ret))); + spi_connected = true; /* Execute the query */ ret = SPI_execute(query, true, 1); // ... rest of try block ... SPI_finish(); + spi_connected = false; } PG_CATCH(); { /* Clean up on error */ - SPI_finish(); + if (spi_connected) + SPI_finish(); pfree(query); PG_RE_THROW(); } PG_END_TRY();
🧹 Nitpick comments (1)
src/pl/plisql/src/pl_autonomous.c (1)
460-466: LGTM: Proper function/procedure branching with resource cleanup.The code correctly routes functions through
execute_autonomous_function(returning a value) and procedures throughdblink_exec(returning NULL). Both paths have properPG_TRY/CATCHblocks ensuring cleanup of allocated resources.Minor optimization opportunity:
rettypeis looked up twice (here at line 461 and insidebuild_autonomous_callat line 223). This could be consolidated by extendingbuild_autonomous_callto returnrettype, avoiding the second syscache lookup. However, this is a performance optimization, not a correctness issue.Also applies to: 500-554
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/pl/plisql/src/expected/plisql_autonomous.outis excluded by!**/*.out
📒 Files selected for processing (2)
src/pl/plisql/src/pl_autonomous.c(10 hunks)src/pl/plisql/src/sql/plisql_autonomous.sql(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pl/plisql/src/pl_autonomous.c (4)
src/backend/utils/cache/syscache.c (2)
SearchSysCache1(220-229)ReleaseSysCache(268-272)src/include/postgres.h (1)
ObjectIdGetDatum(256-260)src/backend/executor/spi.c (4)
SPI_connect(96-100)SPI_execute(605-636)SPI_getbinval(1278-1292)SPI_finish(186-220)src/include/utils/palloc.h (1)
MemoryContextSwitchTo(123-130)
🔇 Additional comments (7)
src/pl/plisql/src/sql/plisql_autonomous.sql (3)
194-429: Verify test count aligns with PR objectives.PR objectives state "five new tests added," but the code implements 11 new autonomous-function tests (Tests 10–20 at lines 194–429), including 3 commented-out tests referencing issue IvorySQL#985. Confirm whether this count discrepancy is intentional or a documentation issue.
343-360: Nested autonomous function test is well-structured.Test 17 correctly validates multiple SPI contexts: outer_function calls inner_function, both insert autonomously, and return values are chained correctly (160 + 10 + 5 = 175). The test includes verification queries for both inserts.
220-227: NULL and typed return values are comprehensive.Tests 11 and 16–20 properly exercise NULL handling and various return types (TEXT, NUMERIC, DATE, BOOLEAN), covering both pass-by-value and pass-by-reference semantics. Verification queries confirm both the returned values and side effects (inserts).
Also applies to: 323-331, 373-381, 393-401, 414-422
src/pl/plisql/src/pl_autonomous.c (4)
25-25: LGTM: Include and global variable for dblink() caching.The new include and cached OID follow the existing pattern for dblink_exec_oid and are correctly invalidated together.
Also applies to: 35-35
50-52: LGTM: Invalidation callback updated for both cached OIDs.Correctly resets both
dblink_exec_oidanddblink_oidwhen pg_proc changes.
207-207: LGTM: Function vs procedure detection and SQL generation.The distinction based on
prorettype != VOIDOIDcorrectly identifies functions (SELECT) vs procedures (CALL), enabling return value capture for functions.Also applies to: 228-229, 280-300
386-391: LGTM: Correct memory context handling for pass-by-reference types.The code correctly switches to the function's memory context (
fn_mcxt) before copying pass-by-reference return values, ensuring the data survivesSPI_finish(). Pass-by-value types are correctly skipped.
Summary
Adds return value support for autonomous transaction functions, achieving full Oracle compatibility for the
PRAGMA AUTONOMOUS_TRANSACTIONfeature.What changed:
Implementation Details
Function vs Procedure Detection
Return Value Capture
SELECT * FROM dblink(...) AS t(result rettype)datumCopy()beforeSPI_finish()Test Coverage
Added 5 new test cases:
Test 10: Function with return value
Test 11: Function with NULL return
Test 12-14: Commented test cases for COMMIT/ROLLBACK (references issue IvorySQL#985)
Oracle Compatibility
Testing
All 17 regression tests pass:
Verified against Oracle Database Free 23.26:
84for input42→ IvorySQL now matches ✅$dollar quoting needed)Known Limitations
COMMIT/ROLLBACK not supported (tracked separately in issue IvorySQL#985)
Code Changes
Files modified:
src/pl/plisql/src/pl_autonomous.c(+217/-39 lines)execute_autonomous_function()helpersrc/pl/plisql/src/sql/plisql_autonomous.sql(+106 lines)src/pl/plisql/src/expected/plisql_autonomous.out(+118 lines)Total: 402 insertions, 39 deletions
Related Issues
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.