plisql code for function and procedure #921
Conversation
refine the comments.
refine the comments.
WalkthroughWide-ranging comment, documentation, and formatting edits across backend, PL/PLiSQL, and tests. Functional changes: added a memory-context switch when updating package variables in PL/ISQL execution, removed the core yyscanner linkage from the PL/ISQL scanner global state, and expanded PLiSQL subproc-function header/API (new constant, ARGMODE variants, new/extended prototypes). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Exec as PL/ISQL Executor
participant FuncMC as Function MemoryContext
participant OldMC as Previous MemoryContext
participant PV as PackageVar
Caller->>Exec: exec_stmt_case(...)
Exec->>OldMC: save current context (oldctx)
Exec->>FuncMC: switch to function memory context
Exec->>PV: update package variable (value / datatype / typmod)
Exec->>OldMC: restore oldctx
Exec-->>Caller: continue execution
note right of Exec: New explicit context switch for package var update
sequenceDiagram
autonumber
participant Parser
participant PLScan as PL/ISQL Scanner
participant CoreLex as Core Lexer
Parser->>PLScan: save yylex global state
note right of PLScan: core `yyscanner` pointer is no longer saved/restored
Parser->>CoreLex: continue lexing (core-managed scanner)
Parser->>PLScan: restore yylex global state (without core scanner pointer)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pl/plisql/src/pl_exec.c (1)
5098-5102: Holdable flag must be applied before opening the cursorThis new block only ORs
CURSOR_OPT_HOLDintoportal->cursorOptionsafterSPI_cursor_open_with_paramlistreturns. By that pointSPI_cursor_open_with_paramlisthas already decided whether to callPersistHoldablePortal, so the portal remains bound to the current resource owner and will still be closed at subtransaction/transaction end. In other words, package-level cursors are still non-holdable despite this change.To actually make these cursors holdable, we need to ensure the
CURSOR_OPT_HOLDbit is present in the options passed intoSPIbefore the portal is created (e.g. by OR-ing it intocurvar->cursor_options/stmt->cursor_optionsand ultimately thecursorOptionsargument ofSPI_cursor_open_with_paramlist). That allows SPI to persist the portal correctly. Applying the flag afterwards is too late and leaves the functionality broken.
🧹 Nitpick comments (1)
src/pl/plisql/src/pl_subproc_function.h (1)
224-258: Deduplicate the repeated prototypes.
plisql_assign_out_subprocfunc_globalvarandplisql_subprocfunc_ref(plus their argument lists) are declared twice back-to-back. Keeping two copies risks the signatures drifting in future edits and confuses readers. Trim the duplicates so each API is declared once.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
src/oracle_test/regress/expected/ora_plisql.outis excluded by!**/*.outsrc/pl/plisql/src/expected/plisql_nested_subproc.outis excluded by!**/*.outsrc/pl/plisql/src/expected/plisql_nested_subproc2.outis excluded by!**/*.out
📒 Files selected for processing (18)
src/backend/commands/functioncmds.c(1 hunks)src/backend/oracle_parser/ora_gram.y(2 hunks)src/include/nodes/parsenodes.h(1 hunks)src/include/pg_config.h.in(1 hunks)src/oracle_fe_utils/ora_psqlscan.l(1 hunks)src/oracle_test/regress/sql/ora_plisql.sql(3 hunks)src/pl/plisql/src/pl_comp.c(67 hunks)src/pl/plisql/src/pl_exec.c(12 hunks)src/pl/plisql/src/pl_funcs.c(2 hunks)src/pl/plisql/src/pl_gram.y(7 hunks)src/pl/plisql/src/pl_handler.c(3 hunks)src/pl/plisql/src/pl_reserved_kwlist.h(1 hunks)src/pl/plisql/src/pl_scanner.c(3 hunks)src/pl/plisql/src/pl_subproc_function.h(5 hunks)src/pl/plisql/src/pl_unreserved_kwlist.h(1 hunks)src/pl/plisql/src/plisql.h(7 hunks)src/pl/plisql/src/sql/plisql_nested_subproc.sql(23 hunks)src/pl/plisql/src/sql/plisql_nested_subproc2.sql(19 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/pl/plisql/src/pl_subproc_function.h (1)
src/pl/plisql/src/pl_subproc_function.c (16)
plisql_build_variable_from_funcargs(240-583)plisql_set_subprocfunc_action(591-612)plisql_check_subprocfunc_properties(617-718)plisql_build_subproc_function(728-771)plisql_add_subproc_function(776-788)plisql_register_internal_func(795-816)plisql_unregister_internal_func(822-826)plisql_get_func_result_tupdesc(832-852)get_subprocfunc_arg_info(857-880)plisql_init_subprocfunc_globalvar(1136-1192)plisql_assign_out_subprocfunc_globalvar(1198-1251)plisql_subprocfunc_ref(2960-3039)plisql_get_subprocfunc_detail(1280-1423)get_subprocfunc_arg_info_from_arguments(2534-2571)plisql_subprocfunc_HashTableLookup(2576-2590)plisql_dynamic_compile_subproc(2695-2837)
src/pl/plisql/src/pl_comp.c (6)
src/backend/utils/fmgr/funcapi.c (2)
get_func_typename_info(1889-1933)resolve_polymorphic_argtypes(1090-1343)src/pl/plisql/src/pl_funcs.c (2)
plisql_ns_lookup(139-197)plisql_ns_top(89-93)src/include/postgres.h (1)
ObjectIdGetDatum(256-260)src/backend/utils/cache/typcache.c (1)
lookup_type_cache(385-963)src/backend/utils/cache/syscache.c (1)
SysCacheGetAttr(599-622)src/backend/utils/fmgr/fmgr.c (1)
get_call_expr_argtype(1997-2035)
⏰ 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: oracle_regression (ubuntu-latest)
- GitHub Check: oracle_pg_regression (ubuntu-latest)
- GitHub Check: meson_build (ubuntu-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: contrib_regression
- GitHub Check: pg_regression (ubuntu-latest)
🔇 Additional comments (5)
src/pl/plisql/src/pl_comp.c (1)
54-55: LGTM – All changes are cosmetic.The modifications throughout this file consist entirely of:
- Whitespace and indentation normalization
- Comment wording improvements (e.g., "Build an execution tree" → "Build", "compiling for validation" → "compiling")
- Parameter alignment in function calls
- Minor formatting adjustments
No functional logic, control flow, or behavioral changes detected.
Also applies to: 83-83, 100-121, 126-262, 347-1479, 1799-1800, 1855-1913, 2032-2033, 2074-2317, 2338-2547, 2727-2871, 2876-2912, 2928-3093, 3105-3535, 3573-3630
src/oracle_test/regress/sql/ora_plisql.sql (1)
5-5: LGTM – Documentation improvements.Three minor comment corrections:
- Line 5: Grammar fix ("Can" → "can")
- Line 937: Clarity improvement ("is only using get" → "is only used to get")
- Line 1059: Formatting fix ("---" → " --") and terminology ("recursive" → "recursion")
No functional changes to test logic.
Also applies to: 937-937, 1059-1059
src/pl/plisql/src/plisql.h (1)
218-220: LGTM – Comment and formatting refinements.All changes are cosmetic improvements to inline documentation:
- Clarified field descriptions (notnull, pctrowtypname, info fields)
- Improved comment grammar and wording
- Adjusted formatting for consistency
No changes to type definitions, function signatures, or data structure layouts.
Also applies to: 357-358, 442-443, 973-973, 978-978, 1069-1074, 1168-1168, 1242-1242
src/pl/plisql/src/pl_scanner.c (2)
109-109: Minor grammar fix.Comment improved from "The stuff the core lexer needs" to "The stuff that the core lexer needs".
148-174: Removal of yyscanner from saved state is safe
The Flex scanner handle is held in the staticplisql_scanner(set once byora_scanner_init) and is never reassigned during nested parses, so preserving onlycore_yy_extrafully restores lexer state.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pl/plisql/src/pl_subproc_function.h (1)
240-258: Remove the duplicated prototypes.
plisql_assign_out_subprocfunc_globalvarandplisql_subprocfunc_refare declared twice (Lines 224-238 and again here). The duplication adds noise and invites future drift between the copies. Drop the second block and keep a single declaration set.Apply this diff to remove the redundant block:
-extern void plisql_assign_out_subprocfunc_globalvar(PLiSQL_execstate *estate, - FunctionCallInfo fcinfo); -extern int plisql_subprocfunc_ref(ParseState *pstate, List *funcname, - List **fargs, /* return value */ - List *fargnames, - int nargs, - Oid *argtypes, - bool expand_variadic, - bool expand_defaults, - bool include_out_arguments, - Oid *funcid, /* return value */ - Oid *rettype, /* return value */ - bool *retset, /* return value */ - int *nvargs, /* return value */ - Oid *vatype, /* return value */ - Oid **true_typeids, /* return value */ - List **argdefaults, /* return value */ - void **pfunc); /* return value */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pl/plisql/src/pl_subproc_function.h(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pl/plisql/src/pl_subproc_function.h (1)
src/pl/plisql/src/pl_subproc_function.c (16)
plisql_build_variable_from_funcargs(240-583)plisql_set_subprocfunc_action(591-612)plisql_check_subprocfunc_properties(617-718)plisql_build_subproc_function(728-771)plisql_add_subproc_function(776-788)plisql_register_internal_func(795-816)plisql_unregister_internal_func(822-826)plisql_get_func_result_tupdesc(832-852)get_subprocfunc_arg_info(857-880)plisql_init_subprocfunc_globalvar(1136-1192)plisql_assign_out_subprocfunc_globalvar(1198-1251)plisql_subprocfunc_ref(2960-3039)plisql_get_subprocfunc_detail(1280-1423)get_subprocfunc_arg_info_from_arguments(2534-2571)plisql_subprocfunc_HashTableLookup(2576-2590)plisql_dynamic_compile_subproc(2695-2837)
⏰ 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: meson_build (ubuntu-latest)
- GitHub Check: pg_regression (ubuntu-latest)
- GitHub Check: oracle_pg_regression (ubuntu-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: oracle_regression (ubuntu-latest)
- GitHub Check: contrib_regression
|
from @coderabbitai comments |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1616 @OreoYang, you're correct! There are duplicate declarations of both functions. Here's the corrected version with the duplicates removed: extern void plisql_init_subprocfunc_globalvar(PLiSQL_execstate *estate,
FunctionCallInfo fcinfo);
extern void plisql_assign_out_subprocfunc_globalvar(PLiSQL_execstate *estate,
FunctionCallInfo fcinfo);
extern int plisql_subprocfunc_ref(ParseState *pstate, List *funcname,
List **fargs, /* return value */
List *fargnames,
int nargs,
Oid *argtypes,
bool expand_variadic,
bool expand_defaults,
bool include_out_arguments,
Oid *funcid, /* return value */
Oid *rettype, /* return value */
bool *retset, /* return value */
int *nvargs, /* return value */
Oid *vatype, /* return value */
Oid **true_typeids, /* return value */
List **argdefaults, /* return value */
void **pfunc); /* return value */
extern FuncDetailCode plisql_get_subprocfunc_detail(ParseState *pstate,
PLiSQL_function *pfunc,I've kept the second version of each declaration since it has better formatting with arguments on separate lines, making it more readable. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/pl/plisql/src/pl_subproc_function.h (1)
38-38: Consider removing extra blank lines for consistency.The blank lines at lines 38 and 41 add unnecessary whitespace. C headers typically group related declarations without extra spacing unless separating major sections.
Also applies to: 41-41
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pl/plisql/src/pl_subproc_function.h(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pl/plisql/src/pl_subproc_function.h (1)
src/pl/plisql/src/pl_subproc_function.c (16)
plisql_build_variable_from_funcargs(240-583)plisql_set_subprocfunc_action(591-612)plisql_check_subprocfunc_properties(617-718)plisql_build_subproc_function(728-771)plisql_add_subproc_function(776-788)plisql_register_internal_func(795-816)plisql_unregister_internal_func(822-826)plisql_get_func_result_tupdesc(832-852)get_subprocfunc_arg_info(857-880)plisql_init_subprocfunc_globalvar(1136-1192)plisql_assign_out_subprocfunc_globalvar(1198-1251)plisql_subprocfunc_ref(2960-3039)plisql_get_subprocfunc_detail(1280-1423)get_subprocfunc_arg_info_from_arguments(2534-2571)plisql_subprocfunc_HashTableLookup(2576-2590)plisql_dynamic_compile_subproc(2695-2837)
⏰ 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: oracle_regression (ubuntu-latest)
- GitHub Check: pg_regression (ubuntu-latest)
- GitHub Check: oracle_pg_regression (ubuntu-latest)
- GitHub Check: contrib_regression
- GitHub Check: meson_build (ubuntu-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (5)
src/pl/plisql/src/pl_subproc_function.h (5)
42-47: LGTM!The ARGMODE enum expansion to include OUT and INOUT parameter modes is correct and necessary for handling different parameter passing semantics in PL/ISQL.
142-162: LGTM!The inline comment updates and comprehensive documentation block (lines 89-140) significantly improve the clarity of the struct's purpose and field semantics.
193-202: LGTM!The formatting and alignment improvements enhance readability while preserving correct function signatures that match the implementations.
Also applies to: 206-207, 221-225
243-264: LGTM!All function prototypes are correctly declared and match their implementations in
pl_subproc_function.c. The expanded parameter lists forplisql_get_subprocfunc_detailproperly reflect the enhanced API.
266-266: LGTM!The include guard is correctly closed with a single
#endif, as confirmed by the past review feedback being addressed in commit 6f12501.
| extern int plisql_subprocfunc_ref(ParseState *pstate, List *funcname, | ||
| List **fargs, /* return value */ | ||
| List *fargnames, | ||
| int nargs, | ||
| Oid *argtypes, | ||
| bool expand_variadic, | ||
| bool expand_defaults, | ||
| bool include_out_arguments, | ||
| Oid *funcid, /* return value */ | ||
| Oid *rettype, /* return value */ | ||
| bool *retset, /* return value */ | ||
| int *nvargs, /* return value */ | ||
| Oid *vatype, /* return value */ | ||
| Oid **true_typeids, /* return value */ | ||
| List **argdefaults, /* return value */ | ||
| void **pfunc); /* return value */ |
There was a problem hiding this comment.
Critical: Parameter name mismatch between header and implementation.
The header declares the 9th parameter as include_out_arguments (line 233), but the implementation in pl_subproc_function.c (lines 2959-3038) names it proc_call. This mismatch creates confusion about the parameter's purpose and can lead to incorrect usage.
Apply this diff to align the parameter name with the implementation:
extern int plisql_subprocfunc_ref(ParseState *pstate, List *funcname,
List **fargs, /* return value */
List *fargnames,
int nargs,
Oid *argtypes,
bool expand_variadic,
bool expand_defaults,
- bool include_out_arguments,
+ bool proc_call,
Oid *funcid, /* return value */
Oid *rettype, /* return value */
bool *retset, /* return value */
int *nvargs, /* return value */
Oid *vatype, /* return value */
Oid **true_typeids, /* return value */
List **argdefaults, /* return value */
void **pfunc); /* return value */📝 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.
| extern int plisql_subprocfunc_ref(ParseState *pstate, List *funcname, | |
| List **fargs, /* return value */ | |
| List *fargnames, | |
| int nargs, | |
| Oid *argtypes, | |
| bool expand_variadic, | |
| bool expand_defaults, | |
| bool include_out_arguments, | |
| Oid *funcid, /* return value */ | |
| Oid *rettype, /* return value */ | |
| bool *retset, /* return value */ | |
| int *nvargs, /* return value */ | |
| Oid *vatype, /* return value */ | |
| Oid **true_typeids, /* return value */ | |
| List **argdefaults, /* return value */ | |
| void **pfunc); /* return value */ | |
| extern int plisql_subprocfunc_ref(ParseState *pstate, List *funcname, | |
| List **fargs, /* return value */ | |
| List *fargnames, | |
| int nargs, | |
| Oid *argtypes, | |
| bool expand_variadic, | |
| bool expand_defaults, | |
| bool proc_call, | |
| Oid *funcid, /* return value */ | |
| Oid *rettype, /* return value */ | |
| bool *retset, /* return value */ | |
| int *nvargs, /* return value */ | |
| Oid *vatype, /* return value */ | |
| Oid **true_typeids, /* return value */ | |
| List **argdefaults, /* return value */ | |
| void **pfunc); /* return value */ |
🤖 Prompt for AI Agents
In src/pl/plisql/src/pl_subproc_function.h around lines 226 to 241, the 9th
parameter is declared as include_out_arguments but the implementation in
pl_subproc_function.c uses the name proc_call; update the header to match the
implementation by renaming the 9th parameter in this function prototype to
proc_call (and ensure any related comments/usage nearby reflect the same name)
so the declaration and definition use the same parameter identifier.
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Style / Chores