fix(plisql): resolve variable reset bug in nested function expressions (#1124)#1133
fix(plisql): resolve variable reset bug in nested function expressions (#1124)#1133gaoxueyu merged 2 commits intoIvorySQL:masterfrom
Conversation
📝 WalkthroughWalkthroughConditional datum-copy logic was added to PL/pgSQL parameter evaluation to avoid dangling pointers during nested calls; a regression test file and Makefile entry were added to validate fixes for Bug Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/pl/plisql/src/pl_subproc_function.c (1)
2810-2813: Preserving parent datum dno during subproc compilation is the right fixSwitching to
plisql_adddatum_keep_dnohere avoids rewritingdatum->dnofor datums inherited from the parent function, which would corrupt the parent’s view and explains the nested-expression bug. Because you add them sequentially fromi = 0tolastoutvardno - 1, the localplisql_Datumsindex still matches the preserveddno, so existing execstate lookups remain valid.As a small safety net, you could assert the expected mapping when copying:
- for (i = 0; i < subprocfunc->lastoutvardno; i++) - plisql_adddatum_keep_dno(subprocfunc->function->datums[i]); + for (i = 0; i < subprocfunc->lastoutvardno; i++) + { + Assert(subprocfunc->function->datums[i]->dno == i); + plisql_adddatum_keep_dno(subprocfunc->function->datums[i]); + }
📜 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_bug1124.outis excluded by!**/*.out
📒 Files selected for processing (6)
src/pl/plisql/src/Makefile(1 hunks)src/pl/plisql/src/pl_comp.c(1 hunks)src/pl/plisql/src/pl_exec.c(2 hunks)src/pl/plisql/src/pl_subproc_function.c(1 hunks)src/pl/plisql/src/plisql.h(1 hunks)src/pl/plisql/src/sql/plisql_bug1124.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T10:43:25.257Z
Learnt from: syasirs
Repo: IvorySQL/IvorySQL PR: 858
File: src/Makefile.global.in:513-514
Timestamp: 2025-09-02T10:43:25.257Z
Learning: In IvorySQL Makefile.global.in, $(wildcard t/oracle/*.pl) is used instead of t/oracle/*.pl in Oracle TAP test discovery because the t/oracle directory is optional and may not exist in most builds. The wildcard function safely returns empty string when the directory is missing, preventing shell expansion errors.
Applied to files:
src/pl/plisql/src/Makefile
🧬 Code graph analysis (3)
src/pl/plisql/src/plisql.h (1)
src/pl/plisql/src/pl_comp.c (1)
plisql_adddatum_keep_dno(3242-3253)
src/pl/plisql/src/pl_subproc_function.c (1)
src/pl/plisql/src/pl_comp.c (1)
plisql_adddatum_keep_dno(3242-3253)
src/pl/plisql/src/pl_exec.c (2)
src/include/postgres.h (1)
DatumGetPointer(316-320)src/include/utils/palloc.h (1)
MemoryContextSwitchTo(123-130)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: pg_regression (ubuntu-latest)
- GitHub Check: contrib_regression
- GitHub Check: oracle_regression (ubuntu-latest)
- GitHub Check: meson_build (ubuntu-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: oracle_pg_regression (ubuntu-latest)
🔇 Additional comments (5)
src/pl/plisql/src/Makefile (1)
57-61: Add plisql_bug1124 to REGRESS suite looks correctIncluding
plisql_bug1124alongside the existing PL/iSQL tests ensures the new regression coverage is exercised by bothoracle-checkandoracle-installcheck. No issues spotted with ordering or options.src/pl/plisql/src/plisql.h (1)
1350-1352: Header declaration for plisql_adddatum_keep_dno is consistentThe new
extern void plisql_adddatum_keep_dno(PLiSQL_datum *datum);declaration is placed appropriately next toplisql_adddatumand matches the implementation signature, so other modules (likepl_subproc_function.c) can safely use it.src/pl/plisql/src/pl_comp.c (1)
3232-3253: Implementation of plisql_adddatum_keep_dno correctly avoids dno mutationThe new helper mirrors
plisql_adddatum’s allocation/growth logic but deliberately skips assigningdatum->dno, which is exactly what you want when reusing parent datums for nested compilations. Given it’s only used from subproc init, and the comment clearly documents the constraint, this looks sound.src/pl/plisql/src/pl_exec.c (1)
7340-7371: New varlena copy inplisql_param_eval_var_rocorrectly avoids dangling pointersThe updated
plisql_param_eval_var_ronow:
- Detects non‑null,
typlen == -1(varlena) vars that are not external expanded objects.- Copies those values into
get_eval_mcontext(estate)viadatumCopy(...), instead of returningvar->valuedirectly.- Retains the old
MakeExpandedObjectReadOnlypath for expanded objects (and non‑varlena / null cases).This aligns with the Bug #1124 description: nested PL/iSQL execution that reassigns a var can free or overwrite the original varlena storage, so returning a direct pointer was unsafe. Copying into
eval_mcontextmakes the param value independent for the duration of expression evaluation and is cleaned up byexec_eval_cleanup, which is exactly what we want.Implementation details look sound: correct memory context, correct
datumCopyusage (typByVal = false,typlen = -1), and unchanged behavior for non‑varlena types.src/pl/plisql/src/sql/plisql_bug1124.sql (1)
1-131: Comprehensive and well-designed regression test suite. ✓This test file effectively validates the fix for bug #1124 with clear scenarios and correct expected values:
- Test 1 (basic): Core bug scenario—variable modification followed by nested function call in expression.
- Test 2 (multiple): Ensures consistent behavior across repeated invocations.
- Test 3 (proc_and_func): Validates that the fix works when a nested procedure modifies the outer variable (not just direct assignment).
- Test 4 (integer): Confirms the fix applies to fixed-length types (not just varlena NUMBER).
- Test 5 (order): Tests expression evaluation is order-independent.
All expected results (139 = 109 + 30) are mathematically correct. Comments clearly document the bug and each test's purpose. Proper test isolation via function creation/cleanup.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/pl/plisql/src/pl_exec.c (2)
7096-7108:paramidselection split (package:datum->dno + 1, non-package:param->paramid) looks right for Bug #1124 and package datum addressing.
Optional: consider adding a smallAssert(OidIsValid(datum->pkgoid) == OidIsValid(scratch.d.cparam.pkgoid))-style sanity check nearby to lock the intended invariant.
7345-7384: Fix is correct: copy non-expanded varlena intoeval_mcontextto avoid dangling pointers across nested PL/iSQL calls.
Minor polish: since this path is only selected for varlena vars, the comment branch “pass-by-value” is a bit misleading—consider tightening wording to “expanded object or NULL” (or assertvar->datatype->typlen == -1here for clarity).
📜 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_bug1124.outis excluded by!**/*.out
📒 Files selected for processing (6)
src/pl/plisql/src/Makefile(1 hunks)src/pl/plisql/src/pl_comp.c(1 hunks)src/pl/plisql/src/pl_exec.c(3 hunks)src/pl/plisql/src/pl_subproc_function.c(1 hunks)src/pl/plisql/src/plisql.h(1 hunks)src/pl/plisql/src/sql/plisql_bug1124.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-02T10:43:25.257Z
Learnt from: syasirs
Repo: IvorySQL/IvorySQL PR: 858
File: src/Makefile.global.in:513-514
Timestamp: 2025-09-02T10:43:25.257Z
Learning: In IvorySQL Makefile.global.in, $(wildcard t/oracle/*.pl) is used instead of t/oracle/*.pl in Oracle TAP test discovery because the t/oracle directory is optional and may not exist in most builds. The wildcard function safely returns empty string when the directory is missing, preventing shell expansion errors.
Applied to files:
src/pl/plisql/src/Makefile
📚 Learning: 2025-12-03T06:50:27.755Z
Learnt from: rophy
Repo: IvorySQL/IvorySQL PR: 998
File: contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c:82-146
Timestamp: 2025-12-03T06:50:27.755Z
Learning: In contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c, the behavior where ENABLE() clears the existing buffer (rather than preserving it like Oracle does) is an intentional known difference tracked in issue #26 at https://github.com/rophy/IvorySQL/issues/26 (medium impact, open).
Applied to files:
src/pl/plisql/src/pl_exec.c
🧬 Code graph analysis (3)
src/pl/plisql/src/plisql.h (1)
src/pl/plisql/src/pl_comp.c (1)
plisql_adddatum_keep_dno(3242-3253)
src/pl/plisql/src/pl_subproc_function.c (1)
src/pl/plisql/src/pl_comp.c (1)
plisql_adddatum_keep_dno(3242-3253)
src/pl/plisql/src/pl_exec.c (2)
src/include/postgres.h (1)
DatumGetPointer(316-320)src/include/utils/palloc.h (1)
MemoryContextSwitchTo(123-130)
🔇 Additional comments (4)
src/pl/plisql/src/pl_exec.c (1)
7055-7064: RW-optimization gating for package variables is correct and preventsexec_check_rw_parameter()invariant violations.
This matches theparamid == target_param + 1assumption and avoids mixing package datum numbering with function-local datum numbering.src/pl/plisql/src/Makefile (1)
57-62: LGTM: regression suite includesplisql_bug1124.Good to have this covered by default regress runs.
src/pl/plisql/src/sql/plisql_bug1124.sql (1)
1-131: Verify the DROP FUNCTION syntax compatibility with your test harness.The test file uses
DROP FUNCTION test_name;(without signature) across all 5 test functions. While this syntax is valid in Oracle PL/SQL, PostgreSQL typically requires the function signature:DROP FUNCTION test_name();.Check whether the IvorySQL test harness expects Oracle-compatible syntax or PostgreSQL-standard syntax. If the harness runs in a stricter PostgreSQL mode, these DROP statements may fail. If Oracle compatibility is guaranteed, the current form is acceptable; otherwise, adding
()to each DROP statement would improve robustness across different execution contexts.src/pl/plisql/src/plisql.h (1)
1315-1352: No action required. The function correctly lacksPGDLLEXPORTas it is an internal compiler helper, not part of the public plugin API. The header contains both public (markedPGDLLEXPORT) and internal declarations; the presence in an installed header does not determine API exposure—only thePGDLLEXPORTmarker does. This design pattern is consistent throughout the header (16 of 20 functions in this section intentionally lack the marker).
|
Hi, @rophy I'm totally agree with your point about Root Cause part 2, during call
The plisql_param_compile function is responsible for preparing the evaluation of a parameter within a PL/iSQL expression. It is called during expression compilation to set up the correct evaluation logic for each parameter reference. when compiling The first condition checks if the variable is not null, is of varlena type (typlen == -1), and is not already an "expanded" object (a special in-memory representation for complex data). If all these are true, it means the value is a plain varlena datum, so the code makes a copy of it in the evaluation memory context (eval_mcontext). Before this PR, will At this point, the issue can be resolved without modifying the dno-related code. |
|
Per my understanding, is enough to handle the bug. |
29439c1 to
cb474b2
Compare
…ySQL#1124) For varlena types that are NOT expanded objects, copy the value into eval_mcontext rather than returning a pointer to variable storage. Nested function calls can happen during expression evaluation, and when they return, they copy back their global variables to the parent. This copy-back operation frees the parent's old variable storage, which would leave a dangling pointer if we just returned var->value directly. Fixes IvorySQL#1124
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pl/plisql/src/sql/plisql_bugs.sql (1)
13-13: Optional: Consider rounder test values for clarity.The initial value of 99 across all tests is functional but slightly arbitrary. Using 100 instead would make mental arithmetic marginally easier without affecting test validity. This is a very minor nitpick and entirely optional.
Example adjustment
- v_sum NUMBER := 99; + v_sum NUMBER := 100;Then update comments accordingly (e.g., "v_sum = 110" and final result "110 + 30 = 140").
Also applies to: 36-36, 64-64, 92-92, 113-113
📜 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_bugs.outis excluded by!**/*.out
📒 Files selected for processing (2)
src/pl/plisql/src/Makefilesrc/pl/plisql/src/sql/plisql_bugs.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pl/plisql/src/Makefile
🔇 Additional comments (6)
src/pl/plisql/src/sql/plisql_bugs.sql (6)
1-7: Well-documented test file header.The comments clearly describe the bug scenario and provide helpful context for understanding the test cases.
8-32: LGTM - Core bug scenario is well-tested.Test 1 directly addresses the primary bug pattern where a modified variable's value was lost during expression evaluation with a nested function call. The arithmetic (99 + 10 = 109, then 109 + 30 = 139) is correct, and the test structure is clear.
34-60: LGTM - Consistency testing across multiple invocations.Test 2 validates that the fix works reliably across multiple evaluations of the same expression pattern. The intentional repetition of the expression on lines 47, 50, and 53 effectively tests for consistency and prevents regressions in repeated calls.
62-88: LGTM - Important nested procedure + function interaction.Test 3 covers a critical scenario where a nested procedure modifies a parent variable before a nested function reads it in an expression. This validates that the fix correctly handles parameter state preservation across both procedure and function nesting.
90-109: LGTM - Fixed-length type coverage.Test 4 validates the fix with integer (fixed-length) type, complementing the NUMBER (variable-length varlena) tests. Since the PR fix addresses varlena copying (typlen == -1) for types like NUMBER, this test confirms that fixed-length types remain unaffected and continue to work correctly.
111-131: LGTM - Operand ordering variation covered.Test 5 validates that the fix works regardless of operand order by placing the nested function call on the left side of the expression. This ensures the parameter state preservation doesn't depend on evaluation order.
Add test cases to verify the fix for variable reset bug in nested functions. Tests cover NUMBER and integer types, multiple expressions, nested procedures modifying variables, and different operand orders.
Both done! Your minimal version worked perfectly. |
Summary
Fix a bug where PL/iSQL variables showed original values instead of modified values when used in expressions containing nested function calls.
Problem
When evaluating expressions like
v_result := v_sum + nested_func(), ifv_sumwas modified before this statement, it would incorrectly use the original value (or zero) instead of the modified value.Example that failed before this fix:
Root Cause
Two issues in the parameter evaluation mechanism for nested functions:
Parameter state not preserved: When
plisql_param_fetchwas called withop == PLISQL_DTYPE_SUBPROC, it clearedparams->paramValids[dno]causing subsequent reads of the same variable to re-fetch stale values.Varlena values not copied: For variable-length types (NUMBER/numeric with
typlen == -1), the value pointer referenced memory ineval_mcontextthat was reset during nested function execution, causing data corruption.Fix
Preserve parameter validity: Only clear
paramValids[dno]for regular datum types, not for subproc parameters.Copy varlena values: For types with
typlen == -1, copy the value usingdatumCopy()to ensure it survives memory context resets.Use stable datum numbers: Added
plisql_adddatum_keep_dno()to preserve original datum numbers when setting up nested function parameter lists.Testing
Added regression test
plisql_bug1124with 5 test cases covering:Verified behavior matches Oracle Database 23c
All existing plisql regression tests pass
Files Changed
src/pl/plisql/src/pl_exec.c- Main fix for parameter handling and varlena copyingsrc/pl/plisql/src/pl_comp.c- Addedplisql_adddatum_keep_dno()functionsrc/pl/plisql/src/plisql.h- Added function declarationsrc/pl/plisql/src/pl_subproc_function.c- Use new function for nested functionssrc/pl/plisql/src/sql/plisql_bug1124.sql- Test casessrc/pl/plisql/src/expected/plisql_bug1124.out- Expected outputsrc/pl/plisql/src/Makefile- Added test to REGRESS listFixes #1124
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.