support function and procedure in WITH clause#1322
support function and procedure in WITH clause#1322bigplaice wants to merge 3 commits intoIvorySQL:masterfrom
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughImplements Oracle-compatible WITH FUNCTION/PROCEDURE: grammar and lexer support, parse-time registration and lookup (overloads/defaults), propagation into Query/PlannedStmt, executor initialization and SRF/composite guards, PL/iSQL lazy compilation/runtime dispatch, EXPLAIN emission of signatures/bodies, build updates, and regression tests. ChangesWITH Clause Inline Functions and Procedures
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.20.0)OpenGrep fatal error (exit code 2): [00.14][ERROR]: Error: exception Unix_error: No such file or directory stat src/include/nodes/parsenodes.h 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/backend/parser/analyze.c (1)
380-382:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
withFuncDefsis not propagated for MERGE statements.The default
transformMergeStmtimplementation insrc/backend/parser/parse_merge.c(lines 124–132) callstransformWithClauseand setsqry->hasModifyingCTE, but omits the assignmentqry->withFuncDefs = stmt->withClause->plsql_defs. All other DML transformers (INSERT, UPDATE, DELETE, VALUES, set-ops) perform this assignment. Without it,WITH FUNCTION … MERGE …will silently discard inline function definitions. The Oracle hook implementation correctly includes this assignment (seecontrib/ivorysql_ora/src/merge/ora_merge.cline 701), indicating it should be in the standard implementation as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/parser/analyze.c` around lines 380 - 382, The MERGE transformer is not propagating WITH FUNCTION definitions: update the default transformMergeStmt implementation to call transformWithClause (as it already does) and then assign qry->withFuncDefs = stmt->withClause->plsql_defs (and preserve existing qry->hasModifyingCTE behavior) so that inline function defs from stmt->withClause are propagated into the resulting Query; modify transformMergeStmt to mirror the withFuncDefs assignment used by other DML transformers (and by the Oracle hook implementation) to ensure WITH FUNCTION … MERGE … retains the definitions.src/include/nodes/parsenodes.h (1)
1662-1663:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the
WithClausepropagation comment to match current behavior.The comment says
WithClausedoes not propagate intoQuery, butQuery.withFuncDefsnow carries data derived from it. This is misleading for future maintainers.Also applies to: 1668-1668
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/include/nodes/parsenodes.h` around lines 1662 - 1663, The existing comment claiming "WithClause does not propagate into the Query" is outdated; change the comment(s) around WithClause (both occurrences) to state that WithClause-derived information is now carried into Query via Query.withFuncDefs (and that CommonTableExpr still propagates as before), so future readers understand that Query.withFuncDefs holds data derived from WithClause.
🧹 Nitpick comments (5)
src/backend/executor/execSRF.c (1)
156-157: 💤 Low valueSilent skip at line 157 is consistent with the
init_sexprerror barrier — but consider an explicitAssert.The
funcexpr->function_from != FUNC_FROM_WITH_CLAUSEguard preventssubproc_should_change_return_typefrom being called on a WITH-clause function. In practice this code path is unreachable becauseinit_sexpr(called earlier, duringExecInitTableFunctionResult) would have already raised an error. The current behaviour — silently falling through to the outerfuncrettype = exprType(...)— is harmless given that guarantee.However, adding
Assert(funcexpr->function_from != FUNC_FROM_WITH_CLAUSE)immediately before line 158 would make the invariant explicit and catch any future regression during development.🛡️ Optional: make the invariant explicit
else if (!FUNC_EXPR_FROM_PG_PROC(funcexpr->function_from) && + /* WITH-clause functions must have been rejected by init_sexpr already */ + Assert(funcexpr->function_from != FUNC_FROM_WITH_CLAUSE), funcexpr->function_from != FUNC_FROM_WITH_CLAUSE && subproc_should_change_return_type(funcexpr, &resulttype, &chtypmod, &chcollationoid))(Alternatively, a standalone
Assertstatement placed before theelse ifwould read more clearly.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/executor/execSRF.c` around lines 156 - 157, Add an explicit Assert to document the invariant that a WITH-clause function cannot reach this branch: before the else-if that checks funcexpr->function_from (the block that currently skips calling subproc_should_change_return_type and falls back to computing funcrettype via exprType), insert Assert(funcexpr->function_from != FUNC_FROM_WITH_CLAUSE). This makes the guarantee from init_sexpr (invoked during ExecInitTableFunctionResult) explicit and will catch regressions where a WITH-clause function could reach this code path.src/oracle_test/regress/sql/with_function.sql (2)
22-28: ⚡ Quick winAdd a sibling-call case from inside a WITH subprogram body.
Current coverage proves top-level composition and self-recursion, but not the new path where one inline subprogram resolves a sibling WITH function/procedure from inside its own body. That is the exact scope-propagation path added in this PR, so it should have a regression. As per coding guidelines,
**/sql/*.sql: Test SQL files. Ensure comprehensive coverage of features.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/oracle_test/regress/sql/with_function.sql` around lines 22 - 28, Add a new test in the WITH-function regression file that exercises a sibling-call from inside a WITH subprogram body: create two WITH FUNCTIONS (e.g., add1 and mul2 as in the diff) but modify one function body so it calls its sibling (for example, have mul2 call add1 internally or vice versa) and then SELECT the top-level call to exercise the new scope propagation path; ensure the test lives alongside the existing WITH FUNCTION examples in src/oracle_test/regress/sql/with_function.sql and uses the same function names (add1, mul2) so the test covers resolving a sibling WITH function from inside a subprogram body.
8-13: ⚡ Quick winAdd a regression that actually invokes a WITH PROCEDURE.
This only proves the declaration parses. It never exercises the procedure execution path, argument binding, or side effects, so a break in the new runtime support could still pass this file. As per coding guidelines,
**/sql/*.sql: Test SQL files. Ensure comprehensive coverage of features.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/oracle_test/regress/sql/with_function.sql` around lines 8 - 13, The file only declares the WITH PROCEDURE do_nothing without invoking it; update the test to actually execute the procedure (e.g., add a CALL do_nothing; or a BEGIN do_nothing; END; block) and then assert a side-effect or result (keep the existing SELECT 'procedure ok' AS result or replace with a SELECT that verifies the side-effect) so the runtime execution, argument binding and side-effect path for the WITH PROCEDURE named do_nothing is exercised.src/oracle_test/regress/expected/with_function.out (2)
11-20: ⚡ Quick winT02 does not validate actual WITH PROCEDURE invocation.
This case currently proves parsing/acceptance, but not procedure execution semantics. Please add one case that invokes the procedure (directly or via a WITH function wrapper) and asserts an observable effect.
As per coding guidelines,
**/expected/*.outfiles must cover Oracle-compatibility behavior and edge-case coverage, and the referenced contract requires side-effect/void-style WITH PROCEDURE semantics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/oracle_test/regress/expected/with_function.out` around lines 11 - 20, The test defines WITH PROCEDURE do_nothing but never invokes it; update the test to actually call the procedure and assert a side-effect-observable result by adding a WITH-function wrapper that invokes do_nothing and returns a marker (e.g., a function call_wrapper that calls do_nothing then returns 'procedure ok'), then SELECT the wrapper result so the output verifies execution semantics of WITH PROCEDURE (reference symbols: WITH PROCEDURE do_nothing and the new wrapper function name you add).
351-363: ⚡ Quick winAdd EXPLAIN VERBOSE coverage for WITH PROCEDURE body text.
You already verify function body rendering in VERBOSE mode; add the equivalent procedure-body assertion to lock that part of the EXPLAIN contract too.
As per coding guidelines,
**/expected/*.outfiles must ensure edge-case coverage, and the referenced contract requires EXPLAIN output coverage for WITH Function/Procedure definitions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/oracle_test/regress/expected/with_function.out` around lines 351 - 363, Add a second EXPLAIN VERBOSE case mirroring the existing FUNCTION block: after the EXPLAIN (COSTS OFF, VERBOSE) test that exercises WITH FUNCTION shows_body, add an analogous EXPLAIN that defines a WITH PROCEDURE (e.g., WITH PROCEDURE shows_proc(...) IS ... END;) and selects/calls it so the verbose output includes a "WITH Procedure: shows_proc" line and a "Body: ..." line; update the expected output block to assert those Procedure lines just like the Function ones so EXPLAIN VERBOSE covers WITH PROCEDURE body text.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/parser/parse_cte.c`:
- Around line 129-132: The errhint currently references the C enum constant
ORA_PARSER which users cannot type; update the errhint in the ereport call (the
block using ereport/errcode/errmsg/errhint) to instruct users to set the GUC to
the user-typable string value by changing the hint text to: "Set
ivorysql.compatible_mode = 'oracle' to enable this feature." Keep the rest of
the ereport invocation unchanged.
In `@src/backend/parser/parse_with_plsql.c`:
- Around line 249-309: The current check uses (nentryargs - nargs) >
entry->ndefaults which only counts defaults, not whether the omitted suffix is
fully defaulted; change the validation to ensure every omitted parameter
(positions from nargs to nentryargs-1) is a defaulted trailing parameter by
scanning entry->argtypes/entry->analyzeddefaults from the end and counting
contiguous trailing defaults and rejecting the call if omitted_count >
trailing_defaults; also only append analyzeddefaults into *fargs when the
expansion flag (expand_defaults) is true (and remove/avoid hitting Assert by
ensuring we never iterate into non-default params).
In `@src/pl/plisql/src/pl_handler.c`:
- Around line 349-385: buildWithFuncEntries creates WithFuncEntry objects but
never initializes their default-argument metadata, causing garbage reads later;
update buildWithFuncEntries to initialize the default-related fields on each new
WithFuncEntry (match how transformWithFuncDefs sets up defaults) — at minimum
set entry->ndefaults = 0 and entry->analyzeddefaults = NULL (or the appropriate
zero/empty values for those fields) and ensure any flag or mechanism that
triggers default expansion for runtime calls is disabled unless defaults are
properly analyzed; reference buildWithFuncEntries, WithFuncEntry,
p_with_func_list and transformWithFuncDefs to copy the same
initialization/behavior for default handling.
- Around line 287-337: buildParamListForFunc() currently omits FUNC_PARAM_OUT
entries (and returns NULL if nparams==0), which means OUT-only parameters never
get a variable in the compiled WITH subprogram; to fix, count and allocate
ParamListInfo entries for OUT parameters as well (i.e., include fp->mode ==
FUNC_PARAM_OUT when computing nparams), and in the second loop do not skip OUT
params—resolve their type with typenameTypeIdAndMod and populate paramnames and
params[*] (ptype, ptypmod, pmode as (char)fp->mode, isnull/value as appropriate)
so OUT params exist in the resulting ParamListInfo used by
plisql_compile_inline().
---
Outside diff comments:
In `@src/backend/parser/analyze.c`:
- Around line 380-382: The MERGE transformer is not propagating WITH FUNCTION
definitions: update the default transformMergeStmt implementation to call
transformWithClause (as it already does) and then assign qry->withFuncDefs =
stmt->withClause->plsql_defs (and preserve existing qry->hasModifyingCTE
behavior) so that inline function defs from stmt->withClause are propagated into
the resulting Query; modify transformMergeStmt to mirror the withFuncDefs
assignment used by other DML transformers (and by the Oracle hook
implementation) to ensure WITH FUNCTION … MERGE … retains the definitions.
In `@src/include/nodes/parsenodes.h`:
- Around line 1662-1663: The existing comment claiming "WithClause does not
propagate into the Query" is outdated; change the comment(s) around WithClause
(both occurrences) to state that WithClause-derived information is now carried
into Query via Query.withFuncDefs (and that CommonTableExpr still propagates as
before), so future readers understand that Query.withFuncDefs holds data derived
from WithClause.
---
Nitpick comments:
In `@src/backend/executor/execSRF.c`:
- Around line 156-157: Add an explicit Assert to document the invariant that a
WITH-clause function cannot reach this branch: before the else-if that checks
funcexpr->function_from (the block that currently skips calling
subproc_should_change_return_type and falls back to computing funcrettype via
exprType), insert Assert(funcexpr->function_from != FUNC_FROM_WITH_CLAUSE). This
makes the guarantee from init_sexpr (invoked during ExecInitTableFunctionResult)
explicit and will catch regressions where a WITH-clause function could reach
this code path.
In `@src/oracle_test/regress/expected/with_function.out`:
- Around line 11-20: The test defines WITH PROCEDURE do_nothing but never
invokes it; update the test to actually call the procedure and assert a
side-effect-observable result by adding a WITH-function wrapper that invokes
do_nothing and returns a marker (e.g., a function call_wrapper that calls
do_nothing then returns 'procedure ok'), then SELECT the wrapper result so the
output verifies execution semantics of WITH PROCEDURE (reference symbols: WITH
PROCEDURE do_nothing and the new wrapper function name you add).
- Around line 351-363: Add a second EXPLAIN VERBOSE case mirroring the existing
FUNCTION block: after the EXPLAIN (COSTS OFF, VERBOSE) test that exercises WITH
FUNCTION shows_body, add an analogous EXPLAIN that defines a WITH PROCEDURE
(e.g., WITH PROCEDURE shows_proc(...) IS ... END;) and selects/calls it so the
verbose output includes a "WITH Procedure: shows_proc" line and a "Body: ..."
line; update the expected output block to assert those Procedure lines just like
the Function ones so EXPLAIN VERBOSE covers WITH PROCEDURE body text.
In `@src/oracle_test/regress/sql/with_function.sql`:
- Around line 22-28: Add a new test in the WITH-function regression file that
exercises a sibling-call from inside a WITH subprogram body: create two WITH
FUNCTIONS (e.g., add1 and mul2 as in the diff) but modify one function body so
it calls its sibling (for example, have mul2 call add1 internally or vice versa)
and then SELECT the top-level call to exercise the new scope propagation path;
ensure the test lives alongside the existing WITH FUNCTION examples in
src/oracle_test/regress/sql/with_function.sql and uses the same function names
(add1, mul2) so the test covers resolving a sibling WITH function from inside a
subprogram body.
- Around line 8-13: The file only declares the WITH PROCEDURE do_nothing without
invoking it; update the test to actually execute the procedure (e.g., add a CALL
do_nothing; or a BEGIN do_nothing; END; block) and then assert a side-effect or
result (keep the existing SELECT 'procedure ok' AS result or replace with a
SELECT that verifies the side-effect) so the runtime execution, argument binding
and side-effect path for the WITH PROCEDURE named do_nothing is exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 255887cf-561f-450c-9f33-cf6a3814d96f
📒 Files selected for processing (28)
contrib/ivorysql_ora/src/merge/ora_merge.csrc/backend/commands/explain.csrc/backend/executor/execExpr.csrc/backend/executor/execSRF.csrc/backend/executor/execTuples.csrc/backend/optimizer/plan/planner.csrc/backend/oracle_parser/ora_gram.ysrc/backend/parser/Makefilesrc/backend/parser/analyze.csrc/backend/parser/meson.buildsrc/backend/parser/parse_clause.csrc/backend/parser/parse_cte.csrc/backend/parser/parse_func.csrc/backend/parser/parse_with_plsql.csrc/backend/utils/fmgr/funcapi.csrc/include/nodes/execnodes.hsrc/include/nodes/parsenodes.hsrc/include/nodes/plannodes.hsrc/include/nodes/primnodes.hsrc/include/oracle_parser/ora_with_function.hsrc/include/parser/parse_node.hsrc/oracle_fe_utils/ora_psqlscan.lsrc/oracle_test/regress/expected/with_function.outsrc/oracle_test/regress/parallel_schedulesrc/oracle_test/regress/sql/with_function.sqlsrc/pl/plisql/src/pl_comp.csrc/pl/plisql/src/pl_handler.csrc/pl/plisql/src/plisql.h
1 participant
fix #1067
Summary by CodeRabbit
New Features
Behavior Changes
Tests