package phase II#944
Conversation
1) nested function support out parameter` 2) package support out parameters 3) psql /? support \dk 4) supoort select * from func which include out paramster 5) change check variable assignable in pl_gram.y
WalkthroughThis PR extends PL/iSQL and executor infrastructure to support OUT parameters and subprocedures with improved function expression handling. Changes span the executor, parser, utility APIs, and PL/iSQL runtime, adding new subproc helper functions, branching logic for PG-proc versus internal function paths, and enhanced return-type resolution in Oracle compatibility mode. Changes
Sequence Diagram(s)sequenceDiagram
participant Executor as Executor (execExpr.c)
participant Cache as ProcCache
participant Subproc as Subproc Lookup
participant OutParam as OutParam Handler
rect rgb(200, 220, 255)
note over Executor,OutParam: PG-Proc Path (FuncExpr)
Executor->>Cache: ExecInitFuncWithOutParams (PG-proc)
Cache->>Cache: ReleaseSysCache (old)
Cache->>Executor: Validate prokind/prolang
alt Mismatch
Executor->>Cache: ReleaseSysCache
Executor->>Executor: return
end
Executor->>OutParam: get_func_arg_info + retrieve outparams
end
rect rgb(220, 255, 220)
note over Executor,OutParam: Subproc Path (FuncExpr)
Executor->>Subproc: ExecInitFuncWithOutParams (subproc)
Subproc->>Subproc: Skip if kind ≠ PROKIND_FUNCTION
Subproc->>OutParam: get_subproc_arg_info
Subproc->>OutParam: get_internal_function_name
end
rect rgb(255, 240, 200)
note over Executor,Subproc: Return-Type Resolution (Oracle compat)
Executor->>Executor: ExecMakeTableFunctionResult (SRF)
alt SPI not connected & Oracle mode
Executor->>Subproc: subproc_should_change_return_type
Executor->>Executor: Update funcrettype/funcresulttype
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pl/plisql/src/pl_gram.y (1)
4011-4027: Handle named arguments before validating OUT parametersWhen callers use named notation (
proc(arg_name => some_var)), each argument node infunc->argsis aNamedArgExprwrapping the underlyingParam. Because we pass that wrapper straight intoParseParamVariable, the helper returns theNamedArgExprunchanged (it only unwrapsRelabelType,FuncExpr,CoerceViaIO, etc.). The subsequentIsA(arg, Param)check therefore fails and we raise an error even though the caller supplied a variable. Please unwrapNamedArgExpr(e.g.if (IsA(arg, NamedArgExpr)) arg = ((NamedArgExpr *) arg)->arg;) before callingParseParamVariable.src/backend/executor/execTuples.c (1)
2192-2216: Add DB_ORACLE compatibility gate to match execSRF.c and prevent unintended behavior changeVerified: execSRF.c (line 140) wraps the return-type adjustment logic with
DB_ORACLE == compatible_db, but execTuples.c (line 2192) does not. This asymmetry allows return-type changes for functions with OUT parameters to apply unconditionally in all modes, not just Oracle compatibility mode.The fix is necessary: add the DB_ORACLE gate to execTuples.c (line 2192) to ensure consistency and prevent silent semantic changes in non-Oracle mode.
- if (nodeTag(tle->expr) == T_FuncExpr && SPI_get_connected() < 0) + if (nodeTag(tle->expr) == T_FuncExpr && + SPI_get_connected() < 0 && + DB_ORACLE == compatible_db) {
🧹 Nitpick comments (6)
src/oracle_test/regress/sql/ora_package.sql (1)
13-13: Configuration flag set redundantly. Theivorysql.allow_out_parameter_constflag is set to true at line 13 and again at line 2774, before the large test block. Since line 13 already enables the feature for the entire test file, the redundant set at line 2774 is unnecessary. Consider removing line 2774 unless the intermediate reset is intentional.Also applies to: 2774-2774, 6098-6098
src/pl/plisql/src/sql/plisql_nested_subproc.sql (1)
2184-2196: OUT-parameter function shouldn’t return an uninitialized local record
f1(... x OUT anyarray, y OUT anycompatiblearray) RETURN recorddeclaresr record;but never populates it;RETURN r;is redundant for OUT params and can confuse readers.- function f1(a anyelement, b anyarray, - c anycompatible, d anycompatible, - x OUT anyarray, y OUT anycompatiblearray) RETURN record - AS - r record; + function f1(a anyelement, b anyarray, + c anycompatible, d anycompatible, + x OUT anyarray, y OUT anycompatiblearray) RETURN record + AS begin x := a || b; y := array[c, d]; - RETURN r; + RETURN; -- OUT params carry the result end;Also applies to: 2198-2210
src/bin/psql/help.c (1)
247-247: Place “\dk … list packages” in sorted order and check options
- Keep commands alphabetically grouped: move “\dk…” before “\dl…” (k < l) to match surrounding order.
- Verify whether packages listing supports expanded mode (
x). Many entries use[Sx+]; ifxis supported for\\dk, adjust the option set.- \\dl[x+] list large objects, same as \\lo_list - \\dk[S+] [PATTERN] list packages + \\dk[S+] [PATTERN] list packages + \\dl[x+] list large objects, same as \\lo_listAlso remember the file header note: update ref/psql-ref.sgml accordingly.
src/bin/psql/po/zh_CN.po (1)
3008-3012: New translation for “\dk… list packages”: LGTM; sync catalogsThe string reads naturally (“列出包”). After merging, please run the usual PO update (msgmerge) so references/ordering are refreshed across locales.
src/include/funcapi.h (2)
281-290: Public prototypes for subproc helpers: add header docsExpose a short comment describing:
- what each accessor returns on failure,
- required includes (FuncExpr availability),
- ownership/lifetime of returned buffers.
This reduces misuse when called from non-PL/iSQL code.
205-214: Add documentation for new PLiSQL_funcs_call hooks describing memory ownership and lifetimesAll three new hooks are properly implemented by the plisql provider and correctly registered. However, their documentation is minimal and lacks critical details about memory contracts. Add comments to the hook declarations (src/include/funcapi.h lines 205-214) specifying:
- What each return value means and valid ranges
- Memory context for output parameters (palloc'd in which context? caller's responsibility to free?)
- Ownership of returned arrays/strings
- Preconditions (e.g., fexpr->parent_func must be set)
This matches the public wrapper functions' responsibility to document their behavior so external callers and future maintainers understand the memory management contract.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
src/oracle_test/regress/expected/ora_package.outis excluded by!**/*.outsrc/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!**/*.outsrc/pl/plisql/src/expected/plisql_out_parameter.outis excluded by!**/*.out
📒 Files selected for processing (20)
src/backend/commands/tablecmds.c(1 hunks)src/backend/executor/execExpr.c(2 hunks)src/backend/executor/execSRF.c(3 hunks)src/backend/executor/execTuples.c(1 hunks)src/backend/parser/parse_clause.c(1 hunks)src/backend/parser/parse_param.c(1 hunks)src/backend/utils/fmgr/funcapi.c(2 hunks)src/bin/psql/help.c(1 hunks)src/bin/psql/po/zh_CN.po(1 hunks)src/include/funcapi.h(2 hunks)src/oracle_test/regress/sql/ora_package.sql(9 hunks)src/oracle_test/regress/sql/ora_plisql.sql(1 hunks)src/pl/plisql/src/pl_exec.c(0 hunks)src/pl/plisql/src/pl_gram.y(2 hunks)src/pl/plisql/src/pl_package.c(1 hunks)src/pl/plisql/src/pl_package.h(1 hunks)src/pl/plisql/src/pl_subproc_function.c(16 hunks)src/pl/plisql/src/sql/plisql_nested_subproc.sql(14 hunks)src/pl/plisql/src/sql/plisql_nested_subproc2.sql(13 hunks)src/pl/plisql/src/sql/plisql_out_parameter.sql(0 hunks)
💤 Files with no reviewable changes (2)
- src/pl/plisql/src/sql/plisql_out_parameter.sql
- src/pl/plisql/src/pl_exec.c
🧰 Additional context used
🧬 Code graph analysis (9)
src/backend/executor/execTuples.c (1)
src/backend/utils/fmgr/funcapi.c (1)
subproc_should_change_return_type(1966-1978)
src/backend/parser/parse_clause.c (6)
src/backend/utils/cache/syscache.c (2)
SearchSysCache1(220-229)ReleaseSysCache(268-272)src/include/postgres.h (1)
ObjectIdGetDatum(256-260)src/include/access/htup_details.h (1)
GETSTRUCT(751-755)src/backend/utils/mmgr/mcxt.c (1)
pstrdup(1702-1706)src/backend/utils/fmgr/funcapi.c (3)
get_func_arg_info(1405-1495)get_subproc_arg_info(1949-1961)get_internal_function_name(1809-1831)src/backend/parser/parse_param.c (1)
ParseParamVariable(436-474)
src/pl/plisql/src/pl_package.h (1)
src/pl/plisql/src/pl_package.c (3)
plisql_get_subproc_arg_info(3495-3556)plisql_get_subproc_prokind(3562-3581)plisql_subproc_should_change_return_type(3587-3653)
src/pl/plisql/src/pl_package.c (2)
src/include/nodes/pg_list.h (1)
list_length(151-155)src/backend/utils/mmgr/mcxt.c (3)
palloc(1320-1348)palloc0(1350-1370)pstrdup(1702-1706)
src/include/funcapi.h (1)
src/backend/utils/fmgr/funcapi.c (3)
get_subproc_kind(1938-1944)get_subproc_arg_info(1949-1961)subproc_should_change_return_type(1966-1978)
src/backend/executor/execExpr.c (5)
src/backend/utils/cache/syscache.c (2)
SearchSysCache1(220-229)ReleaseSysCache(268-272)src/include/postgres.h (1)
ObjectIdGetDatum(256-260)src/include/access/htup_details.h (1)
GETSTRUCT(751-755)src/backend/utils/fmgr/funcapi.c (4)
get_func_arg_info(1405-1495)get_subproc_kind(1938-1944)get_subproc_arg_info(1949-1961)get_internal_function_name(1809-1831)src/backend/utils/cache/lsyscache.c (1)
get_func_real_rettype(1822-1875)
src/pl/plisql/src/pl_subproc_function.c (3)
src/backend/utils/fmgr/fmgr.c (1)
get_fn_expr_rettype(1956-1971)src/pl/plisql/src/pl_comp.c (3)
plisql_build_datatype(2926-2942)plisql_build_variable(2707-2766)add_parameter_name(1518-1538)src/pl/plisql/src/pl_package.c (3)
plisql_get_subproc_arg_info(3495-3556)plisql_get_subproc_prokind(3562-3581)plisql_subproc_should_change_return_type(3587-3653)
src/backend/executor/execSRF.c (3)
src/backend/executor/spi.c (1)
SPI_get_connected(3444-3448)src/backend/nodes/nodeFuncs.c (1)
exprType(44-316)src/backend/utils/fmgr/funcapi.c (2)
func_should_change_return_type(2635-2766)subproc_should_change_return_type(1966-1978)
src/backend/utils/fmgr/funcapi.c (1)
src/backend/utils/cache/packagecache.c (1)
plisql_internel_funcs_init(393-404)
🔇 Additional comments (15)
src/oracle_test/regress/sql/ora_plisql.sql (1)
908-930: Nested function OUT parameter changes align with PR objectives. The test correctly demonstrates OUT parameter propagation in nested subprocedures. The function properly assigns to both the nested function's OUT parameter and the outer function's IN OUT parameter, showing the expected semantics.src/pl/plisql/src/sql/plisql_nested_subproc2.sql (3)
2049-2078: Return type ambiguity with OUT parameters. Lines 2054-2062 show a function with OUT parameters (x OUT anyarray, y OUT anycompatiblearray) returningRECORD, but the test at lines 2065-2076 appears to expect array outputs. Verify whether the RECORD return is intentional when OUT parameters are already providing output semantics, or if the return type should be adjusted to match the test expectations (e.g.,INTEGER).
2303-2359: OUT parameter semantics correctly demonstrated. The test cases properly show that OUT parameters can be assigned within functions, propagated to callers, and coexist with independent return values. The patternvar1 := test_f(var1); raise info '%-%', var1, var2correctly validates both the return value and OUT parameter assignment.
2006-2042: OUT parameters in package functions work correctly. The test demonstrates that package functions can use OUT parameters (e.g.,var2 out mds%rowtype) alongside standard return types. The implementation correctly assigns output values both to the OUT parameter and return value, which are properly captured in test invocations at lines 2050-2055.src/oracle_test/regress/sql/ora_package.sql (3)
5077-5118: OUT parameters in procedures work correctly, including mutual recursion. The test demonstrates procedures with OUT parameters (lines 5080-5083) that call each other in a chain (proc2→proc1, proc4→proc3), with proper value assignment and propagation. The test correctly validates both output values at line 5116, confirming the feature works across procedure call chains.
5629-6095: Comprehensive OUT parameter test coverage. The large test block (lines 5629-6095) demonstrates extensive OUT parameter functionality including: single/multiple OUT parameters, mixed parameter ordering (IN + OUT), INOUT parameters, complex types (records, arrays), nested/recursive procedures, and coverage across procedures, functions, and packages. All tests properly validate that OUT parameters receive values from called routines and propagate correctly to callers. The test structure is well-organized by feature and correctly exercises the implementation.
2782-2821: Package-level OUT parameter function signatures are correct. The package functions properly combine OUT parameters with return values (e.g., line 2782:function f_out(id integer, name out varchar) return varchar), with implementations at lines 2804-2821 correctly assigning to OUT parameters and returning values. The pattern is consistent across package, schema-level, and nested function scopes.Also applies to: 3043-3048
src/pl/plisql/src/sql/plisql_nested_subproc.sql (4)
30-31: IN OUT signature for nested square(): LGTMChanging
squaretoin outmatches the test flow (mutates actual arg, returns original square). No issues spotted.
1817-1831: nocopy OUT test: LGTM
out nocopysemantics and returning a scalar are coherent; the RAISE shows OUT effects before return. No changes requested.Please confirm if you want to assert
ret = 23(returned value) alongside the OUT prints in this block.
2216-2229: Simple OUT param test f1(): LGTMOUT
jand scalar return are exercised both via call and SELECT INTO. Looks consistent with the new OUT behavior.
2548-2555: Consistent scalar returns after OUT/NOCOPY: LGTMExplicit
RETURNvalues (24/return id) are fine with OUT/INOUT patterns tested here.Also applies to: 2560-2566, 2586-2593
src/backend/executor/execExpr.c (3)
5125-5156: Good split between PG-proc and subproc pathsThe prokind/prolang checks and using get_func_arg_info vs get_subproc_arg_info are correct, preventing misuse of proc cache for subprocs.
5171-5236: Strict variable requirement path is correct; guard error message lifetimeThe SPI-connected path correctly insists on variables for OUT/INOUT unless
allow_out_parameter_constis true and wires the callback viaoutparamSetup. Also,funcnameoriginates from syscache only used beforeReleaseSysCache, so lifetime is safe.
5208-5221: Need to verify both code locations mentioned in the review. Let me examine the second location (lines 5249-5261) to ensure consistency:Now let me verify the second location (5249-5261) to ensure both code paths are consistent:
The code is correct; the paramid-to-index conversion is properly implemented.
The subtraction of 1 correctly converts from one-based Param IDs to zero-based array indices. The ParamListInfo uses
params[paramno - 1]where paramno (the parameter ID) is 1-based, establishing the pattern. The storedparamidsarray is consumed byestate->datums[paramids[i]]in the callback, which requires zero-based indices, confirming the conversion is necessary and correct.Adding an explanatory comment would improve maintainability, but the implementation itself is sound:
// Store as 0-based index for estate->datums lookup in callback paramids[nout++] = ((Param *) arg)->paramid - 1;src/backend/executor/execSRF.c (1)
138-170: Based on the code inspection, I can now refine the review comment. The duplication is confirmed but differs from the original assessment in a key way: the guard conditions are not identical.Update to Review Comment
The inner if/else logic for return-type adjustment is indeed duplicated between execSRF.c and execTuples.c, but the guard conditions differ: execSRF.c checks
DB_ORACLE == compatible_dbbefore applying the adjustment, while execTuples.c does not. This suggests either:
- The DB_ORACLE check should also apply to execTuples.c, or
- The two sites intentionally have different gating logic.
Before extracting a shared helper, clarify whether both sites should have identical guard conditions. If so, a unified helper could enforce consistent semantics across both files.
DRY the return-type adjustment into a shared helper, but verify guard consistency first.
The func_should_change_return_type/subproc_should_change_return_type calling pattern duplicates across execSRF.c (line 150, 157) and execTuples.c (line 2200, 2208). However, their guard conditions differ: execSRF.c grates with DB_ORACLE check; execTuples.c does not. Confirm whether the DB_ORACLE gate should also apply to execTuples.c, then extract both the guard and adjustment logic into a shared helper (e.g., in funcapi.c) to prevent future drift.
| case AT_ForceViewCompile: | ||
| return NULL; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that returning NULL is the intended behavior for AT_ForceViewCompile.
Unlike other cases in this switch that return descriptive strings (e.g., "ALTER TABLE ... SET WITH ROWID"), this new case explicitly returns NULL. Please confirm:
- Is
NULLthe correct return value, or should this return a descriptive string? - If
NULLis intentional (e.g., because this operation is internal-only and not user-facing), consider adding a comment explaining why. - Ensure all other switch statements handling
AlterTableTypeproperly handle this new enum value.
Run the following script to verify how AT_ForceViewCompile is defined and used:
🏁 Script executed:
#!/bin/bash
# Description: Find the definition and usage of AT_ForceViewCompile to verify proper handling
# Find the enum definition
echo "=== Enum definition ==="
rg -n "AT_ForceViewCompile" --type=c --type=h -C 2
# Find all switch statements on AlterTableType to check if they handle this case
echo -e "\n=== Switch statements that may need updating ==="
rg -n "switch.*AlterTableType|case AT_" --type=c --type=h -A 2 | head -100Length of output: 10212
Now let me run a script to verify the function context and check for issues:Based on the script output already provided and my analysis, I can now generate the rewritten review comment. The evidence shows:
- Function context: Lines 6960-6961 are in a switch statement that converts
AlterTableTypeenum values to strings for error/debug messages - Inconsistency found: The test module (
test_ddl_deparse.c) has a descriptive string"COMPILE FOIRCE VIEW"for this enum, but the main function returnsNULL - Typo detected: The test module has
"FOIRCE"instead of the correct"FORCE" - Oracle feature: This is an Oracle-specific operation (Oracle uses "COMPILE FORCE VIEW" for invalid views), being added to PostgreSQL for compatibility
Key findings:
- Lines 4962-4963, 5454-5456, and 5884 all handle
AT_ForceViewCompilewith actual logic - Only the string conversion function at 6960-6961 returns
NULL - Test module shows it should return a descriptive string
- Test module has a typo that needs fixing
Fix the NULL return value and correct the typo in the test module.
The AT_ForceViewCompile case at lines 6960-6961 returns NULL, but the test module (src/test/modules/test_ddl_deparse/test_ddl_deparse.c:296-297) indicates this should return a descriptive string. Additionally, the test module contains a typo: "COMPILE FOIRCE VIEW" should be "COMPILE FORCE VIEW".
Update lines 6960-6961 to return a descriptive string matching the pattern of other cases (e.g., "ALTER TABLE ... COMPILE FORCE VIEW"), and fix the typo in the test module.
🤖 Prompt for AI Agents
In src/backend/commands/tablecmds.c around lines 6960-6961, the switch case for
AT_ForceViewCompile returns NULL but should return a descriptive string like the
other AlterTableType cases (e.g., "ALTER TABLE ... COMPILE FORCE VIEW"); change
the NULL to an appropriate string literal following existing patterns. Also fix
the typo in the test module src/test/modules/test_ddl_deparse/test_ddl_deparse.c
at lines ~296-297 where "COMPILE FOIRCE VIEW" should be corrected to "COMPILE
FORCE VIEW" so the test and stringify function match.
| get_subproc_arg_info(func, &argtypes, | ||
| &argnames, &argmodes); | ||
| proname = get_internal_function_name(func); | ||
| } | ||
| i = 0; | ||
| foreach(lc, func->args) | ||
|
|
||
| foreach(lc1, func->args) | ||
| { | ||
|
|
||
| if (argmodes[i] == PROARGMODE_OUT || | ||
| argmodes[i] == PROARGMODE_INOUT) | ||
| { | ||
| Node *arg = (Node *) lfirst(lc); | ||
| Node *arg = (Node *) lfirst(lc1); | ||
|
|
||
| arg = ParseParamVariable(arg); | ||
| if (!IsA(arg, Param)) | ||
| { | ||
| ReleaseSysCache(procTup); | ||
| ereport(ERROR, | ||
| (errcode(ERRCODE_DATA_EXCEPTION), | ||
| errmsg("OUT or IN OUT arguments of the function %s must be variables ", | ||
| errmsg("OUT or IN OUT arguments of the function %s must be variables.", | ||
| proname))); | ||
| } | ||
| } | ||
| i++; | ||
| } | ||
| } |
There was a problem hiding this comment.
Incorrect argmode indexing rejects valid calls
When an OUT (or TABLE) parameter appears before an IN/INOUT parameter, argmodes[0] is PROARGMODE_OUT while the first actual argument in func->args is really for the later IN parameter. Incrementing i per actual argument therefore misclassifies that IN argument as OUT/INOUT and raises the “must be variables” error. A simple test case is CREATE FUNCTION pkg.foo(OUT o int, IN i int) ... and SELECT * FROM pkg.foo(1); — this now fails even though the call is legal.
Please keep the mode index aligned with the formal parameter list: capture the nargs returned by get_func_arg_info/get_subproc_arg_info, advance an argpos counter over the formal arguments (skipping PROARGMODE_OUT/PROARGMODE_TABLE) before looking at each actual argument, and only enforce the Param requirement when argmodes[argpos] == PROARGMODE_INOUT.
- i = 0;
- foreach (lc1, func->args)
- {
- if (argmodes[i] == PROARGMODE_OUT ||
- argmodes[i] == PROARGMODE_INOUT)
- {
- Node *arg = (Node *) lfirst(lc1);
- arg = ParseParamVariable(arg);
- if (!IsA(arg, Param))
- ereport(ERROR, ...);
- }
- i++;
- }
+ nargs = ...
+ int argpos = 0;
+ foreach (lc1, func->args)
+ {
+ while (argpos < nargs &&
+ (argmodes[argpos] == PROARGMODE_OUT ||
+ argmodes[argpos] == PROARGMODE_TABLE))
+ argpos++;
+ if (argpos >= nargs)
+ break;
+ if (argmodes[argpos] == PROARGMODE_INOUT)
+ {
+ Node *arg = ParseParamVariable((Node *) lfirst(lc1));
+ if (!IsA(arg, Param))
+ ereport(ERROR, ...);
+ }
+ argpos++;
+ }This keeps OUT-only slots out of the actual-argument walk and preserves correct behaviour for mixed-mode signatures.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/backend/parser/parse_clause.c around lines 4005 to 4030, the code indexes
argmodes using the actual-argument counter i which gets out of sync when there
are OUT/TABLE-only formals before IN/INOUT formals; capture the formal-argument
count (nargs) from get_func_arg_info/get_subproc_arg_info and introduce a
separate argpos pointer over the formal parameter list; before examining each
actual argument, advance argpos until it points at the next non-OUT/non-TABLE
formal (i.e. skip PROARGMODE_OUT and PROARGMODE_TABLE slots), then check
argmodes[argpos] and only enforce the "must be variables" Param requirement when
the matching formal is INOUT (PROARGMODE_INOUT); ensure argpos is incremented to
the next formal each time an actual argument is consumed so indexing stays
aligned with the formal list.
| if (nargs == 0) | ||
| return nargs; | ||
|
|
||
| *p_argtypes = (Oid *) palloc(nargs * sizeof(Oid)); | ||
| *p_argnames = (char **) palloc(nargs * sizeof(char *)); | ||
| *p_argmodes = (char *) palloc0((nargs + 1) * sizeof(char)); | ||
|
|
There was a problem hiding this comment.
Initialize outputs when there are no arguments
This early return leaves *p_argtypes, *p_argnames, and *p_argmodes untouched when nargs == 0. Callers expect these outputs to be initialized (at least to NULL), and we expose this through the public wrappers in funcapi.c. Returning with uninitialized pointers will surface as random memory reads or crashes for zero-arg subprocs. Please set the outputs to NULL before returning zero to keep the API contract and avoid undefined behaviour.
nargs = list_length(subprocfunc->arg);
if (nargs == 0)
- return nargs;
+{
+ if (p_argtypes)
+ *p_argtypes = NULL;
+ if (p_argnames)
+ *p_argnames = NULL;
+ if (p_argmodes)
+ *p_argmodes = NULL;
+ return 0;
+}🤖 Prompt for AI Agents
In src/pl/plisql/src/pl_package.c around lines 3521 to 3527, the early return
when nargs == 0 leaves *p_argtypes, *p_argnames and *p_argmodes uninitialized;
set *p_argtypes = NULL, *p_argnames = NULL and *p_argmodes = NULL before
returning 0 so callers receive initialized outputs (no random reads/crashes),
then return 0.
| declare | ||
| salary integer; | ||
| var2 record; | ||
| var2 integer; | ||
| id integer; | ||
| function test_out(id in out integer, salary out integer) return record | ||
| function test_out(id in out integer, salary out integer) return integer | ||
| IS | ||
| declare | ||
| var1 record; | ||
| function test_out(id in out integer,salary out integer) return record | ||
| var1 integer; | ||
| function test_out(id in out integer,salary out integer) return integer | ||
| IS |
There was a problem hiding this comment.
🧩 Analysis chain
Fix test expectations and initialize IN OUT argument in test_out()
- Comments expect tuple-like output (id,salary), but the function returns an integer; the RAISE prints only the scalar. Also,
idis passed as IN OUT without initialization, which risks NULL arithmetic. - Initialize
idand print OUT values explicitly to match expectations.
@@
declare
salary integer;
- var2 integer;
+ var2 integer;
id integer;
function test_out(id in out integer, salary out integer) return integer
@@
- var1 := test_out(id, salary);
- raise info 'var=%', var1;
+ var1 := test_out(id, salary);
+ -- Print OUT params as per comment (var = (id,salary))
+ raise info 'var=(%,%)', id, salary;
@@
-begin
- var2 := test_out(id, salary);
- raise info 'var2 = %', var2;
+begin
+ id := 0; -- ensure deterministic IN OUT behavior
+ var2 := test_out(id, salary);
+ -- Print OUT params after outer call
+ raise info 'var2 = (%,%)', id, salary;Also applies to: 815-821, 823-825
🏁 Script executed:
#!/bin/bash
# Examine the SQL file around lines 797-826 to understand the test_out function
# and verify the review comment concerns
sed -n '790,830p' src/pl/plisql/src/sql/plisql_nested_subproc.sqlLength of output: 779
🏁 Script executed:
#!/bin/bash
# Also examine expected output comments around line 190/207
sed -n '185,215p' src/pl/plisql/src/sql/plisql_nested_subproc.sqlLength of output: 766
🏁 Script executed:
#!/bin/bash
# Search for any expected output pattern in comments to understand what format is expected
grep -n "var=" src/pl/plisql/src/sql/plisql_nested_subproc.sql | head -20Length of output: 240
🏁 Script executed:
#!/bin/bash
# Check the function signature at the start to understand IN OUT parameters
sed -n '1,50p' src/pl/plisql/src/sql/plisql_nested_subproc.sqlLength of output: 1127
🏁 Script executed:
#!/bin/bash
# Look for the outer test_out function definition and its expected output
sed -n '797,850p' src/pl/plisql/src/sql/plisql_nested_subproc.sqlLength of output: 1155
🏁 Script executed:
#!/bin/bash
# Search for RAISE INFO statements related to var, var1, var2
grep -n "raise info.*var" src/pl/plisql/src/sql/plisql_nested_subproc.sqlLength of output: 2908
🏁 Script executed:
#!/bin/bash
# Search for expected output comments that might mention the format
grep -n -i "expected\|output\|var\s*=\s*(" src/pl/plisql/src/sql/plisql_nested_subproc.sql | head -30Length of output: 85
🏁 Script executed:
#!/bin/bash
# Get line count to understand file scope
wc -l src/pl/plisql/src/sql/plisql_nested_subproc.sqlLength of output: 52
🏁 Script executed:
#!/bin/bash
# Look at the context around lines 30-31 mentioned in review (IN OUT)
sed -n '25,35p' src/pl/plisql/src/sql/plisql_nested_subproc.sqlLength of output: 296
🏁 Script executed:
#!/bin/bash
# Look at lines 2184-2210 mentioned in review (OUT anyarray)
sed -n '2180,2215p' src/pl/plisql/src/sql/plisql_nested_subproc.sqlLength of output: 926
🏁 Script executed:
#!/bin/bash
# Look at lines 2216-2229 mentioned (simple OUT)
sed -n '2210,2235p' src/pl/plisql/src/sql/plisql_nested_subproc.sqlLength of output: 499
🏁 Script executed:
#!/bin/bash
# Search for all expected output annotations in the file
grep -n "^--.*expected\|^--.*Expected" src/pl/plisql/src/sql/plisql_nested_subproc.sqlLength of output: 0
🏁 Script executed:
#!/bin/bash
# Get more context around line 190 and 207
sed -n '180,220p' src/pl/plisql/src/sql/plisql_nested_subproc.sql | cat -nLength of output: 1234
🏁 Script executed:
#!/bin/bash
# Look for actual test case that might show the expected tuple output format
sed -n '800,830p' src/pl/plisql/src/sql/plisql_nested_subproc.sql | cat -nLength of output: 824
🏁 Script executed:
#!/bin/bash
# Search for initialization of id variable anywhere to see current pattern
grep -n "id\s*:=\|id integer" src/pl/plisql/src/sql/plisql_nested_subproc.sql | head -20Length of output: 696
🏁 Script executed:
#!/bin/bash
# Look at the complete declaration and implementation block
sed -n '797,826p' src/pl/plisql/src/sql/plisql_nested_subproc.sqlLength of output: 609
🏁 Script executed:
#!/bin/bash
# Check if there are any expected output comments before the test block
sed -n '790,835p' src/pl/plisql/src/sql/plisql_nested_subproc.sqlLength of output: 885
🏁 Script executed:
#!/bin/bash
# Search for the pattern that shows expected output format mentioned in review (id,salary)
grep -n "(.*id.*salary\|id.*salary" src/pl/plisql/src/sql/plisql_nested_subproc.sql | head -10Length of output: 236
🏁 Script executed:
#!/bin/bash
# Get a comprehensive view of the test block structure
sed -n '797,825p' src/pl/plisql/src/sql/plisql_nested_subproc.sqlLength of output: 608
🏁 Script executed:
#!/bin/bash
# Search for any comments that reference the expected format
sed -n '790,830p' src/pl/plisql/src/sql/plisql_nested_subproc.sql | grep -i "expected\|var\s*=\|comment\|note"Length of output: 63
🏁 Script executed:
#!/bin/bash
# Look for RAISE PRINT or output statements in the test region
sed -n '797,826p' src/pl/plisql/src/sql/plisql_nested_subproc.sql | grep -i "raise\|print"Length of output: 60
🏁 Script executed:
#!/bin/bash
# Try to understand the overall structure better with a wider view
sed -n '795,830p' src/pl/plisql/src/sql/plisql_nested_subproc.sqlLength of output: 691
🏁 Script executed:
#!/bin/bash
# Search for any comments about output format in the file
grep -n "^[[:space:]]*--[[:space:]]*.*var.*=.*(" src/pl/plisql/src/sql/plisql_nested_subproc.sqlLength of output: 390
🏁 Script executed:
#!/bin/bash
# Look for comments that show expected output with the tuple format
grep -B2 -A2 "id.*salary" src/pl/plisql/src/sql/plisql_nested_subproc.sql | head -40Length of output: 449
🏁 Script executed:
#!/bin/bash
# Check the area around the outer call to understand the expectation
sed -n '810,830p' src/pl/plisql/src/sql/plisql_nested_subproc.sqlLength of output: 344
🏁 Script executed:
#!/bin/bash
# Look for test result expectations or assertions
grep -n "EXPECTED\|Expected\|expected\|assert" src/pl/plisql/src/sql/plisql_nested_subproc.sql | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Get exact lines 797-826 with explicit numbering
awk 'NR>=797 && NR<=826 {print NR": "$0}' src/pl/plisql/src/sql/plisql_nested_subproc.sqlLength of output: 759
🏁 Script executed:
#!/bin/bash
# Also check lines 823-825 specifically mentioned
awk 'NR>=820 && NR<=828 {print NR": "$0}' src/pl/plisql/src/sql/plisql_nested_subproc.sqlLength of output: 177
🏁 Script executed:
#!/bin/bash
# Look at the full function test_out to understand return type
sed -n '797,850p' src/pl/plisql/src/sql/plisql_nested_subproc.sqlLength of output: 1155
🏁 Script executed:
#!/bin/bash
# Check for any expected output specification before this test
sed -n '790,800p' src/pl/plisql/src/sql/plisql_nested_subproc.sqlLength of output: 185
🏁 Script executed:
#!/bin/bash
# Get the immediate context of lines referenced in the review
awk 'NR==30 || NR==31 || NR==190 || NR==207 || NR>=797 && NR<=826' src/pl/plisql/src/sql/plisql_nested_subproc.sqlLength of output: 798
🏁 Script executed:
#!/bin/bash
# Final comprehensive check: show the exact state of the test code block
sed -n '797,825p' src/pl/plisql/src/sql/plisql_nested_subproc.sql | nl -v 797Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check around line 815-821 as mentioned
sed -n '815,821p' src/pl/plisql/src/sql/plisql_nested_subproc.sql | nl -v 815Length of output: 40
🏁 Script executed:
#!/bin/bash
# Final context check - see what's before and after the entire test block
sed -n '785,840p' src/pl/plisql/src/sql/plisql_nested_subproc.sql | nl -v 785Length of output: 40
Fix RAISE statements to print OUT parameters and initialize IN OUT argument
The expected output format var = (1,101) var2=(2,102) (line 795 comment) requires printing the OUT parameter values (id, salary), not just the function return values. Additionally, id is used uninitialized in the arithmetic operation var2 := id + 2 (line 811), which results in NULL propagation.
Changes needed:
- Initialize
idbefore the outertest_out()call to ensure deterministic behavior - Print OUT parameters explicitly in both RAISE statements
begin
+ id := 0; -- initialize IN OUT parameter for deterministic behavior
var1 := test_out(id, salary);
- raise info 'var=%', var1;
+ raise info 'var=(%,%)', id, salary;
salary := 102;
id := 2;
return var1;
end;
begin
+ id := 0; -- ensure deterministic outer call
var2 := test_out(id, salary);
- raise info 'var2 = %', var2;
+ raise info 'var2 = (%,%)', id, salary;Also applies to: 815–821, 823–825
| ReleaseSysCache(procTup); | ||
|
|
||
| if (argmodes == NULL) | ||
| continue; |
There was a problem hiding this comment.
do we need free proname manually ?
4 participants
Summary by CodeRabbit
New Features
\dk[S+] [PATTERN]command in psql to list packages.Enhancements