No actionable comments were generated in the recent review. 🎉

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 help to get the list of available commands and usage tips.}

Oracle compatibility verification

Verified behavior against Oracle 23.26 Free:

-- Setup
CREATE TABLE test_nested_commit (id NUMBER);

-- Inner procedure with COMMIT
CREATE OR REPLACE PROCEDURE nested_inner_commit AS
BEGIN
    INSERT INTO test_nested_commit VALUES (1);
    COMMIT;
    INSERT INTO test_nested_commit VALUES (2);
END;
/

-- Outer procedure calling inner
CREATE OR REPLACE PROCEDURE nested_outer_commit AS
BEGIN
    INSERT INTO test_nested_commit VALUES (0);
    nested_inner_commit();
    INSERT INTO test_nested_commit VALUES (3);
END;
/

-- Test 1: Basic nested call with COMMIT
TRUNCATE TABLE test_nested_commit;
EXEC nested_outer_commit;
SELECT * FROM test_nested_commit ORDER BY id;
-- Result: 0, 1, 2, 3

-- Test 2: Deeply nested calls (3 levels)
CREATE OR REPLACE PROCEDURE nested_level3 AS
BEGIN
    INSERT INTO test_nested_commit VALUES (103);
    COMMIT;
END;
/

CREATE OR REPLACE PROCEDURE nested_level2 AS
BEGIN
    INSERT INTO test_nested_commit VALUES (102);
    nested_level3();
    INSERT INTO test_nested_commit VALUES (104);
END;
/

CREATE OR REPLACE PROCEDURE nested_level1 AS
BEGIN
    INSERT INTO test_nested_commit VALUES (101);
    nested_level2();
    INSERT INTO test_nested_commit VALUES (105);
END;
/

TRUNCATE TABLE test_nested_commit;
EXEC nested_level1;
SELECT * FROM test_nested_commit ORDER BY id;
-- Result: 101, 102, 103, 104, 105

-- Test 3: ROLLBACK in nested procedure
CREATE OR REPLACE PROCEDURE nested_inner_rollback AS
BEGIN
    INSERT INTO test_nested_commit VALUES (201);
    ROLLBACK;
    INSERT INTO test_nested_commit VALUES (202);
END;
/

CREATE OR REPLACE PROCEDURE nested_outer_rollback AS
BEGIN
    INSERT INTO test_nested_commit VALUES (200);
    nested_inner_rollback();
    INSERT INTO test_nested_commit VALUES (203);
END;
/

TRUNCATE TABLE test_nested_commit;
EXEC nested_outer_rollback;
SELECT * FROM test_nested_commit ORDER BY id;
-- Result: 202, 203 (200 and 201 were rolled back)

Known limitations
Procedures must use SECURITY INVOKER (PL/iSQL defaults to SECURITY DEFINER which forces atomic mode)

I tried to understand this statement, and found the followings:

1. SECURITY INVOKER indicates that the procedure is to be executed with the privileges of the user that calls it. That is the default. SECURITY DEFINER specifies that the procedure is to be executed with the privileges of the user that owns it.
   A SECURITY DEFINER procedure cannot execute transaction control statements (for example, COMMIT and ROLLBACK, depending on the language).

commit ee895a6
Author: Tom Lane [email protected]
Date: Mon Jan 25 22:28:29 2021 -0500

Improve performance of repeated CALLs within plpgsql procedures.

This patch essentially is cleaning up technical debt left behind
by the original implementation of plpgsql procedures, particularly
commit d92bc83c4.  That patch (or more precisely, follow-on patches
fixing its worst bugs) forced us to re-plan CALL and DO statements
each time through, if we're in a non-atomic context.  That wasn't
for any fundamental reason, but just because use of a saved plan
requires having a ResourceOwner to hold a reference count for the
plan, and we had no suitable resowner at hand, nor would the
available APIs support using one if we did.  While it's not that
expensive to create a "plan" for CALL/DO, the cycles do add up
in repeated executions.

...

Now, when running a non-atomic procedure or DO block that contains
any CALL or DO commands, plpgsql creates a ResourceOwner that
will be used to pin the plans of the CALL/DO commands.  (In an
atomic context, we just use CurrentResourceOwner, as before.)
Having done this, we can just save CALL/DO plans normally,
whether or not they are used across transaction boundaries.
This seems to be good for something like 2X speedup of a CALL
of a trivial procedure with a few simple argument expressions.
By restricting the creation of an extra ResourceOwner like this,
there's essentially zero penalty in cases that can't benefit.

Pavel Stehule, with some further hacking by me

Discussion: https://postgr.es/m/CAFj8pRCLPdDAETvR7Po7gC5y_ibkn_-bOzbeJb39WHms01194Q@mail.gmail.com

For commit 307b066 (fix: allow COMMIT in nested procedure calls for Oracle compatibility)

1. src/pl/plisql/src/pl_gram.y changes - LGTM
   The changes to set requires_procedure_resowner = true for Oracle-style procedure calls (T_WORD/T_CWORD) look correct and should be kept.
2. src/backend/tcop/utility.c changes - Should be removed
   I don't think the changes to utility.c are necessary. Looking at the expected output changes:
   The case START TRANSACTION; DO LANGUAGE plisql $$ BEGIN COMMIT; END $$; - This is explicitly calling DO within a transaction block. The "invalid transaction termination" error is expected and correct behavior here - you cannot commit inside an explicit transaction block. START TRANSACTION is also a PG behavior. Oracle doesn't have similiar statement.
   The case with data_source(i int) function and DO block - The current error is expected because the function definition and the DO block are parsed together as a single SQL statement due to missing terminator.
   Suggested alternative: Instead of modifying utility.c to change atomicity behavior globally for Oracle mode, we can fix the test file directly. Please add /; after the CREATE FUNCTION data_source(i int) definition in plisql_transaction.sql (around line 396) to properly separate it from the following DO block:
   CREATE FUNCTION data_source(i int) RETURNS TEXT LANGUAGE sql
   AS 'select f1 from test4' IMMUTABLE;
   /; -- Add this line to separate statements

DO $$
declare x text;
...
This way "invalid transaction termination" error can be removed, and we don't need to modify the core utility processing logic.

For commit 20c3898 (fix: handle econtext stack in multi-statement queries with COMMIT/ROLLBACK), what crash scenario does this fix address?
The commit message mentions that in multi-statement queries, stale entries on simple_econtext_stack cause crashes. However I tested locally without applying this commit (20c3898), and ran all the test cases in plisql_transaction.sql - I did not observe any crashes.

Could you please provide:
1.The specific test case or scenario that reproduces the crash
2.The error message or stack trace when the crash occurs
3.Steps to reproduce the issue
Without being able to reproduce the crash, it's difficult to verify that this fix is both necessary and correct. The changes to pl_exec.c (clearing simple_econtext_stack, shared_simple_eval_estate, and shared_simple_eval_resowner in exec_stmt_commit() and exec_stmt_rollback()) could have side effects, so we need to understand the exact failure case this addresses.

Hi @rophy, thanks again for the PR. Since I haven’t heard back for a while, would you mind if I push a small follow-up commit to your branch? I’d like to keep the pl_gram.y change and adjust/drop the parts we discussed (e.g. the utility.c change and/or test tweaks), then help get this merged.
If you prefer, I can also open a separate PR from my branch instead—please let me know which you’d rather have.

Hi @rophy, thanks again for the PR. Since I haven’t heard back for a while, would you mind if I push a small follow-up commit to your branch? I’d like to keep the pl_gram.y change and adjust/drop the parts we discussed (e.g. the utility.c change and/or test tweaks), then help get this merged. If you prefer, I can also open a separate PR from my branch instead—please let me know which you’d rather have.

@yuanyl630 feel free to push things as necessary. Sorry that I'll have to wait till this weekend to continue working on PRs.

Commit 2ad34f3 removed utility.c changes as suggested in #1125 (comment), and tests pass.

Still working on #1125 (comment) - honestly I don't understand and remember why I (ok, Calude) changed pl_exec.c. I vaguely remember I ased Claude to add more tests and see if this indeed fixed "COMMIT/ROLLBACK in nested PL/iSQL procedure calls", and I think the changes resulted in Claude trying to fix something in pl_exec.c.

Updated based on review feedback:

• Removed utility.c and pl_exec.c changes as suggested by @yuanyl630. The crash scenario is unreachable without utility.c, so pl_exec.c cleanup is unnecessary.
• Cleaned up tests: removed misleading multi-statement tests, added issue PL/iSQL: COMMIT in nested procedure call fails unlike Oracle (plancache leak in packages) #1007 reproduction cases.

Net changes (3 files): pl_gram.y (requires_procedure_resowner for Oracle-style calls), plus test SQL and expected output.

Re @OreoYang's SECURITY INVOKER question: The PR description was inaccurate. Test procedures use SECURITY INVOKER because Oracle-syntax CREATE PROCEDURE ... IS ends up with prosecdef=true, forcing atomic mode. I'm investigating where this gets set — grammar and catalog defaults are both false, so it may be a separate bug.

The original issue's Test 1 (standalone procs without explicit SECURITY INVOKER) is still broken due to this. Test 2 (packages) works because packages skip the prosecdef check.

In reply to #1125 (comment)):

Why test procedures use SECURITY INVOKER:

Oracle-syntax CREATE PROCEDURE ... IS in IvorySQL defaults to prosecdef=true (SECURITY DEFINER). This is intentional — Oracle's default is AUTHID DEFINER. The code is in functioncmds.c:

if (ORA_PARSER == compatible_db &&
    language_item &&
    strcmp(*language, "plisql") == 0)
    *security_definer = true;   /* default is DEFINER in oracle */

PostgreSQL blocks COMMIT/ROLLBACK in SECURITY DEFINER procedures (forces atomic=true). So without explicit SECURITY INVOKER, the tests fail with "invalid transaction termination".

However, Oracle allows COMMIT in AUTHID DEFINER procedures. I verified against Oracle 23.26 Free — COMMIT works identically in both AUTHID DEFINER and AUTHID CURRENT_USER. The partial commit behavior is the same.

So the real issue is: IvorySQL applies Oracle's AUTHID default but PostgreSQL's atomic restriction, which is incompatible. Oracle-mode should either:

1. Not force atomic=true for prosecdef procedures, or
2. Change the default

This should probably be discussed in another issue

Final update:

• Switched tests to AUTHID CURRENT_USER (Oracle-native syntax) instead of SECURITY INVOKER
• Added package COMMIT test to ora_package.sql — directly tests the plancache resource leak fix

What this PR fixes:
This PR fixes Error 2 (plancache resource leak) from issue #1007 by setting requires_procedure_resowner for Oracle-style calls in pl_gram.y.

• For packages: fully fixed — packages skip the prosecdef check, so only the plancache fix was needed
• For standalone procedures: also fixes the plancache leak, but AUTHID CURRENT_USER is needed as a workaround for Error 1 ("invalid transaction termination"), since Oracle-syntax procedures default to AUTHID DEFINER which forces atomic mode

Error 1 is a separate issue: IvorySQL applies Oracle's AUTHID DEFINER default but PostgreSQL's restriction that blocks COMMIT in SECURITY DEFINER procedures. Oracle allows COMMIT regardless of AUTHID.

Net changes (5 files): pl_gram.y, plisql_transaction.sql/.out, ora_package.sql/.out

Ready for squash/merge. @yuanyl630 feel free to adjust.