Conversation
WalkthroughThe update primarily standardizes comments and formatting across many files. Functional changes include adding a function-execution hook in ExecSRF, initializing new default fields in FuncExpr creation, adding read/write macros for Void fields in node I/O, tightening an index-qual guard to pg_proc-backed functions, a small memory free in funcapi, and a struct member reordering in SPI internals. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Q as Query Executor
participant E as ExecInitTableFunctionResult
participant S as Security/ACL
participant H as InvokeFunctionExecuteHook
participant F as Function
Q->>E: Initialize table function result
E->>S: Check EXECUTE privileges
S-->>E: OK
note over E,H: New: invoke execution hook after ACL check
E->>H: InvokeFunctionExecuteHook(foid)
H-->>E: Return
E->>F: Prepare/execute (non-SRF subproc path)
F-->>E: Result/State
E-->>Q: Exec state initialized
sequenceDiagram
autonumber
participant P as Planner
participant M as match_funcclause_to_indexcol
participant FE as FuncExpr
P->>M: Try match function clause to index column
M->>FE: Inspect origin (pg_proc?)
alt Not from pg_proc
M-->>P: Error: function not from pg_proc
else From pg_proc
M-->>P: Proceed with arg/index qual checks
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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 |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/backend/parser/parse_relation.c (1)
1540-1564: Avoid using RelationGetRelationName() result after table_close
viewnamepoints into theRelationstruct. Aftertable_close(rel, lockmode);that memory is released, so the laterelog(ERROR, ...)dereferences freed storage. Copy the name before closing the relation.- viewname = RelationGetRelationName(rel); - table_close(rel, lockmode); + viewname = pstrdup(RelationGetRelationName(rel)); + table_close(rel, lockmode);src/backend/parser/parse_func.c (1)
938-951: Restore return-type package detectionInside the new
FUNC_FROM_PG_PROCbranch we initialise bothrettyp_ref_pkgtypeandargtype_ref_pkgtype, but in the return-type probe we accidentally setargtype_ref_pkgtypea second time. As writtenrettyp_ref_pkgtypecan never becometrue, sofuncexpr->ref_pkgtypestaysfalseeven when a function returns a package-defined type. That regresses any downstream logic that relies onref_pkgtypebeing accurate.Please flip the assignment so the return-type branch updates
rettyp_ref_pkgtype:- if (pkgtype != NULL) - argtype_ref_pkgtype = true; + if (pkgtype != NULL) + rettyp_ref_pkgtype = true;src/backend/utils/fmgr/funcapi.c (2)
1817-1823: Memory leak when parsing function_name
stringToNode()allocates a full node tree (list struct + list cells + Value nodes + embedded strings). Freeing just the list header withpfree(namelist);leaves the rest of those allocations live in the current memory context. Becauseget_internal_function_name()runs every time we resolve a package/internal function, this becomes an unbounded leak over a long-lived backend. Please free the entire structure (e.g.,list_free_deep(namelist);) instead of the single pointer.
739-759: Blocks with only EXCEPTION handlers never get annotated
set_blocks_oraparam_level()bails out whenblock->body == NIL, which is a normal shape forBEGIN … EXCEPTION … END. That short‑circuit leavesblock->ora_param_stack_*unset and we never descend into the statements inside the exception actions. Any logic that later relies on these annotations (e.g., matching runtime Oracle parameter stack levels during error handling) will see zeros for the block itself and for all nested blocks in the handlers. Please set the fields unconditionally, and also walk the exception action lists so their nested blocks inherit the correct (top, cur) metadata.src/pl/plisql/src/pl_exec.c (1)
9801-9838: Guard against NULL composite results before deconstructing
plisql_out_paramdereferences*op->resvalueunconditionally. When the top-level call returns NULL (e.g., all OUT params left unset or an anonymous block returning NULL),*op->resnullis true andvaluecontains junk. Passing that intodeconstruct_composite_datum()immediately segfaults. Please short-circuit onisNull(and assign NULL to each OUT param as needed) before deconstructing the composite Datum. Otherwise any NULL-returning caller blows up here.src/pl/plisql/src/pl_comp.c (1)
3391-3402: Potential memory leak:elemsarray not freed afterdeconstruct_array.The
deconstruct_arraycall at line 3396 allocates theelemsarray usingpalloc, but there's no correspondingpfree(elems)before the function returns. Based on the relevant code snippet fromsrc/backend/adt/arrayfuncs.c, the array is allocated in the current memory context and should be explicitly freed to avoid leaks.Apply this diff to free the allocated memory:
for (i = 0; i < nelems; i++) argtypenames[i] = TextDatumGetCString(elems[i]); + pfree(elems); }
🧹 Nitpick comments (2)
src/backend/utils/adt/ruleutils.c (2)
3383-3390: Pass a bool to generate_function_name; avoid EXPR_KIND_NONE
generate_function_nameexpects a boolinGroupBy. PassingEXPR_KIND_NONE(an enum) is misleading and brittle. Usefalse.[Suggest using the correct type]
- generate_function_name(proc->prosupport, 1, - NIL, argtypes, - false, NULL, EXPR_KIND_NONE)) + generate_function_name(proc->prosupport, 1, + NIL, argtypes, + false, NULL, false))
13528-13536: Minor: comment typo and const-correctness nit
- Typo: “Slove” → “Solve”.
quote_identifierreturnsconst char *; preferconst char *resultfor const-correctness.- /* - * Slove the issue of using the same global variable in static lib and - * dynamic lib - */ + /* + * Solve the issue of using the same global variable in static lib and + * dynamic lib + */ - char *result; + const char *result;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
README.md(1 hunks)contrib/postgres_fdw/deparse.c(3 hunks)src/backend/catalog/dependency.c(18 hunks)src/backend/commands/explain.c(6 hunks)src/backend/commands/functioncmds.c(38 hunks)src/backend/executor/execExpr.c(14 hunks)src/backend/executor/execSRF.c(4 hunks)src/backend/executor/spi.c(5 hunks)src/backend/nodes/makefuncs.c(1 hunks)src/backend/nodes/nodeFuncs.c(4 hunks)src/backend/nodes/outfuncs.c(1 hunks)src/backend/nodes/print.c(1 hunks)src/backend/nodes/readfuncs.c(1 hunks)src/backend/optimizer/path/clausesel.c(1 hunks)src/backend/optimizer/path/indxpath.c(1 hunks)src/backend/optimizer/util/clauses.c(12 hunks)src/backend/optimizer/util/plancat.c(1 hunks)src/backend/parser/analyze.c(13 hunks)src/backend/parser/parse_func.c(15 hunks)src/backend/parser/parse_relation.c(3 hunks)src/backend/utils/adt/ruleutils.c(37 hunks)src/backend/utils/fmgr/fmgr.c(3 hunks)src/backend/utils/fmgr/funcapi.c(27 hunks)src/include/executor/spi_priv.h(3 hunks)src/include/funcapi.h(3 hunks)src/pl/plisql/src/pl_comp.c(108 hunks)src/pl/plisql/src/pl_exec.c(156 hunks)src/pl/plisql/src/pl_funcs.c(50 hunks)src/pl/plisql/src/pl_gram.y(9 hunks)src/pl/plisql/src/pl_handler.c(18 hunks)src/pl/plisql/src/pl_reserved_kwlist.h(1 hunks)src/pl/plisql/src/pl_scanner.c(16 hunks)src/pl/plisql/src/pl_subproc_function.c(110 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(65 hunks)
🧰 Additional context used
🧬 Code graph analysis (22)
src/backend/parser/parse_relation.c (1)
src/backend/commands/view.c (1)
rel_is_force_view(789-808)
src/backend/commands/explain.c (1)
src/include/nodes/pg_list.h (1)
list_length(151-155)
src/backend/commands/functioncmds.c (9)
src/backend/utils/cache/lsyscache.c (4)
get_namespace_name(3848-3865)get_func_real_rettype(1822-1875)repl_func_real_argtype(1882-1987)get_func_real_allargtype(1992-2083)src/backend/commands/packagecmds.c (1)
get_package_namespace(1047-1066)src/backend/parser/parse_type.c (1)
TypeNameToString(511-519)src/backend/parser/parse_node.c (1)
parser_errposition(108-123)src/backend/nodes/makefuncs.c (1)
makeRangeVar(473-487)src/backend/catalog/namespace.c (1)
NameListToString(3835-3860)src/backend/utils/cache/syscache.c (1)
SysCacheGetAttr(599-622)src/backend/parser/parse_coerce.c (1)
IsBinaryCoercibleWithCast(3385-3464)src/backend/utils/fmgr/fmgr.c (1)
fmgr_info(129-133)
src/backend/parser/parse_func.c (6)
src/backend/parser/parse_param.c (1)
ParseVarParamState(492-518)src/backend/parser/parse_package.c (1)
LookupPkgFunc(439-483)src/backend/utils/cache/syscache.c (2)
ReleaseSysCache(268-272)SearchSysCache1(220-229)src/backend/utils/cache/lsyscache.c (1)
get_namespace_name(3848-3865)src/backend/commands/packagecmds.c (1)
get_package_namespace(1047-1066)src/backend/utils/fmgr/funcapi.c (1)
get_func_typename_info(1889-1933)
src/backend/utils/fmgr/funcapi.c (5)
src/backend/utils/cache/lsyscache.c (1)
get_func_real_rettype(1822-1875)src/backend/utils/cache/packagecache.c (2)
plisql_internel_funcs_init(393-404)set_pkginfo_from_funcexpr(410-451)src/backend/utils/adt/arrayfuncs.c (1)
deconstruct_array(3650-3709)src/include/postgres.h (1)
PointerGetDatum(326-330)src/backend/access/common/tupdesc.c (1)
TupleDescInitEntry(850-920)
src/pl/plisql/src/plisql.h (2)
src/pl/plisql/src/pl_comp.c (8)
plisql_compile(135-262)plisql_build_variable(2709-2768)plisql_build_record(2773-2807)plisql_adddatum(3204-3215)plisql_add_initdatums(3284-3334)plisql_start_datums(3187-3197)plisql_finish_datums(3221-3267)dynamic_build_func_vars(3789-3818)src/pl/plisql/src/pl_scanner.c (3)
void(645-674)plisql_yylex(206-424)plisql_scanner_errposition(615-629)
src/backend/optimizer/path/clausesel.c (1)
src/backend/optimizer/util/plancat.c (1)
function_selectivity(2048-2095)
src/backend/executor/spi.c (1)
src/backend/utils/mmgr/mcxt.c (1)
palloc(1320-1348)
src/backend/executor/execSRF.c (1)
src/include/utils/palloc.h (1)
MemoryContextSwitchTo(123-130)
src/backend/catalog/dependency.c (2)
src/backend/commands/view.c (1)
DeleteForceView(758-782)src/backend/utils/cache/packagecache.c (1)
get_top_function_info(515-528)
src/pl/plisql/src/pl_subproc_function.c (10)
src/backend/utils/mmgr/mcxt.c (5)
MemoryContextAlloc(1184-1209)repalloc(1547-1581)palloc(1320-1348)pstrdup(1702-1706)MemoryContextAllocZero(1218-1235)src/pl/plisql/src/pl_comp.c (7)
plisql_build_datatype(2928-2944)add_parameter_name(1519-1539)build_row_from_vars(2813-2873)plisql_build_variable(2709-2768)plisql_resolve_polymorphic_argtypes(3468-3537)plisql_start_datums(3187-3197)plisql_adddatum(3204-3215)src/pl/plisql/src/pl_funcs.c (3)
plisql_ns_lookup(139-197)plisql_ns_top(89-93)plisql_ns_additem(100-116)src/pl/plisql/src/pl_package.c (1)
is_const_datum(3022-3077)src/pl/plisql/src/pl_exec.c (1)
plisql_assign_in_global_var(6002-6042)src/backend/executor/spi.c (2)
SPI_get_connected(3444-3448)SPI_get_func(3463-3471)src/backend/utils/hash/dynahash.c (2)
hash_create(351-624)hash_search(955-966)src/backend/parser/parse_node.c (1)
parser_errposition(108-123)src/backend/parser/parse_expr.c (1)
transformExpr(141-157)src/pl/plisql/src/pl_scanner.c (1)
plisql_scanner_init(732-758)
src/pl/plisql/src/pl_funcs.c (2)
src/backend/utils/hash/dynahash.c (1)
hash_search(955-966)src/include/nodes/pg_list.h (1)
list_length(151-155)
src/backend/utils/adt/ruleutils.c (10)
src/common/stringinfo.c (2)
appendStringInfoString(229-233)appendStringInfo(144-166)src/backend/utils/adt/arrayfuncs.c (1)
array_ref(3165-3173)src/backend/utils/cache/syscache.c (1)
SysCacheGetAttr(599-622)src/backend/catalog/namespace.c (1)
NameListToString(3835-3860)src/backend/parser/parse_type.c (1)
TypeNameToQuoteString(563-571)src/backend/utils/fmgr/funcapi.c (1)
get_internal_function_name(1809-1831)src/backend/utils/sort/tuplestore.c (1)
tuplestore_begin_heap(329-350)src/backend/access/index/genam.c (2)
systable_beginscan(404-497)systable_getnext(530-574)src/backend/nodes/makefuncs.c (1)
makeRangeVar(473-487)src/backend/parser/parse_node.c (1)
parser_errposition(108-123)
src/include/funcapi.h (1)
src/backend/utils/fmgr/funcapi.c (12)
build_function_result_tupdesc_t(1734-1793)build_plisql_function_result_tupdesc_d(2118-2334)resolve_polymorphic_tupdesc(770-1075)get_type_func_class(1354-1390)build_internal_function_result_tupdesc_t(1798-1804)get_internal_function_name(1809-1831)get_internal_function_result_type(1836-1851)external_get_type_func_class(1856-1860)get_internal_function_outargs(1866-1872)get_internal_function_result_name(1877-1883)func_should_change_return_type(2589-2720)get_func_typename_info(1889-1933)
src/backend/optimizer/util/clauses.c (4)
src/backend/utils/cache/packagecache.c (1)
set_pkgtype_from_funcexpr(457-508)src/backend/nodes/nodeFuncs.c (1)
exprTypmod(323-566)src/include/postgres.h (1)
DatumGetBool(94-98)src/backend/utils/cache/lsyscache.c (1)
get_func_real_rettype(1822-1875)
src/backend/executor/execExpr.c (6)
src/backend/utils/init/miscinit.c (1)
GetUserId(519-524)src/backend/executor/execUtils.c (1)
ExecGetResultType(494-498)src/backend/utils/cache/syscache.c (1)
SearchSysCache1(220-229)src/include/postgres.h (1)
ObjectIdGetDatum(256-260)src/backend/executor/spi.c (1)
SPI_get_connected(3444-3448)src/backend/utils/error/elog.c (2)
errcode(853-864)errmsg(1070-1086)
src/pl/plisql/src/pl_handler.c (8)
src/backend/utils/fmgr/dfmgr.c (1)
find_rendezvous_variable(656-688)src/pl/plisql/src/pl_subproc_function.c (1)
plisql_function_from(1025-1039)src/pl/plisql/src/pl_exec.c (2)
plisql_exec_event_trigger(1295-1358)plisql_exec_function(508-928)src/backend/parser/parse_param.c (3)
get_doStmtCheckVar(2575-2579)push_oraparam_stack(562-589)get_oraparam_level(540-559)src/backend/access/transam/xact.c (1)
GetCurrentSubTransactionId(790-796)src/include/utils/palloc.h (1)
MemoryContextSwitchTo(123-130)src/backend/executor/spi.c (1)
Ora_spi_saved_memorycontext(3487-3491)src/backend/utils/cache/lsyscache.c (1)
get_typlenbyval(2668-2681)
src/pl/plisql/src/pl_scanner.c (1)
src/backend/oracle_parser/liboracle_parser.c (1)
internal_yylex(272-299)
src/backend/parser/analyze.c (4)
src/backend/parser/parse_target.c (4)
transformRowExpression(2054-2121)transformExpressionList(220-278)transformRowTargetList(2128-2191)transformTargetList(121-204)src/backend/parser/parse_node.c (1)
parser_errposition(108-123)src/include/nodes/pg_list.h (2)
list_length(151-155)list_head(127-131)src/backend/parser/parse_relation.c (1)
attnameAttNum(3637-3658)
src/pl/plisql/src/pl_exec.c (6)
src/pl/plisql/src/pl_subproc_function.c (1)
plisql_function_from(1025-1039)src/pl/plisql/src/pl_package.c (3)
plisql_get_relevantContext(2970-2982)package_datumTransfer(3147-3161)plisql_get_datum(2917-2929)src/backend/executor/spi.c (1)
SPI_datumTransfer(1387-1403)src/backend/utils/fmgr/funcapi.c (1)
get_call_result_type(283-293)src/backend/utils/cache/syscache.c (1)
ReleaseSysCache(268-272)src/backend/utils/hash/dynahash.c (1)
hash_search(955-966)
src/pl/plisql/src/pl_subproc_function.h (1)
src/pl/plisql/src/pl_subproc_function.c (26)
plisql_start_subproc_func(138-164)plisql_finish_subproc_func(169-193)plisql_push_subproc_func(199-213)plisql_pop_subproc_func(218-234)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_get_func_name(885-905)plisql_get_subprocfunc_result_type(911-938)plisql_get_subprocfunc_outargs(945-979)plisql_get_subprocfunc_result_name(986-1020)plisql_function_from(1025-1039)plisql_get_subproc_func(1045-1131)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/mmgr/mcxt.c (3)
palloc(1320-1348)pfree(1527-1541)palloc0(1350-1370)src/backend/parser/parse_type.c (2)
TypeNameToString(511-519)typeStringToTypeName(823-860)src/backend/utils/cache/syscache.c (1)
SysCacheGetAttr(599-622)src/backend/utils/fmgr/funcapi.c (1)
get_func_typename_info(1889-1933)src/backend/catalog/namespace.c (1)
NameListToString(3835-3860)src/backend/utils/hash/dynahash.c (1)
hash_search(955-966)
🔇 Additional comments (10)
src/backend/commands/functioncmds.c (3)
103-132: LGTM: Comment clarification and formatting improvements.The comment updates (lines 106-109) clarify the Oracle-specific constraint that schemas do not allow tables, types, and packages to share the same name. Variable declarations have been consistently aligned for better readability.
273-274: LGTM: Accurate language-specific comment.The comment correctly states that only PL/iSQL supports package types, which helps maintainers understand the language-specific behavior.
563-590: LGTM: Logic restructuring maintains original behavior.The code has been reorganized for better readability while preserving the original validation logic for parameter defaults. The behavior remains unchanged.
src/backend/optimizer/util/plancat.c (1)
2117-2128: LGTM: Improved code clarity for non-pg_proc functions.The changes improve readability by:
- Aligning the
FuncExprpointer variable declaration- Adding a clearer comment explaining the default cost path for functions not from pg_proc
- Maintaining the same functional behavior (apply default per-tuple cost and return early)
src/pl/plisql/src/pl_scanner.c (3)
111-177: LGTM: Consistent struct and variable declaration formatting.The changes improve code organization through:
- Consistent variable declaration alignment
- Proper struct member spacing in
PLiSQL_yylex_global_proper- Clearer visual separation between related declarations
All changes are cosmetic with no impact on scanner behavior.
212-403: LGTM: Improved function call parameter alignment.The parameter alignment in function calls like
plisql_parse_tripword,plisql_parse_dblword, andplisql_parse_wordenhances readability. The tokenization logic remains unchanged.
641-646: LGTM: Function signature formatting improvement.The empty line addition (641~) and signature alignment (646~) follow standard formatting conventions without affecting functionality.
src/backend/utils/adt/ruleutils.c (1)
11225-11236: get_internal_function_name declaration and inclusion verifiedsrc/backend/optimizer/path/clausesel.c (1)
867-879: LGTM! Proper guard for non-pg_proc function calls.The addition of the
FUNC_EXPR_FROM_PG_PROCcheck correctly distinguishes between pg_proc-backed functions and other function expressions. Using the heuristic selectivity of 0.3333333 for non-pg_proc calls is appropriate and consistent with the historical default used byfunction_selectivity(as shown in src/backend/optimizer/util/plancat.c lines 2047-2094).src/backend/nodes/outfuncs.c (1)
102-104: LGTM! Properly implemented Void field serialization macro.The new
WRITE_VOID_FIELDmacro follows the established pattern of other field-writing macros and correctly outputs a fixed value of 0 for Void fields during node serialization. This pairs well with the correspondingREAD_VOID_FIELDmacro for proper round-trip serialization.
| /* | ||
| * Only functions originating from pg_proc are eligible here. | ||
| * Subprocedure/package calls are not supported for index-qual matching | ||
| * and should never reach this path. | ||
| */ | ||
| if (!FUNC_EXPR_FROM_PG_PROC(clause->function_from)) | ||
| elog(ERROR, "invalid funcexpr, because it is a subproc"); | ||
| elog(ERROR, "invalid FuncExpr: subprocedure/package calls are not supported for index-qual matching"); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that this error path is unreachable by user queries.
The code uses elog(ERROR, ...) which will abort the transaction. The comment states "should never reach this path", suggesting this is a defensive sanity check for internal bugs rather than user-facing input validation.
Please confirm:
- Is it possible for user queries (e.g., involving extension functions, stored procedures, or packages) to trigger this error?
- If user queries can trigger it, consider returning
NULLinstead of raising an error, consistent with other early returns in this function that indicate "no match" rather than "invalid state".
If this is truly unreachable by normal query paths and only indicates a planner bug, then elog(ERROR) is appropriate. However, if extension functions or other valid SQL constructs can reach here, returning NULL would be more defensive and allow the query planner to continue considering other alternatives.
🏁 Script executed:
#!/bin/bash
# Description: Search for callers of match_funcclause_to_indexcol to understand the call path
# and whether subprocedure/package calls could reach this function.
# Find callers of match_funcclause_to_indexcol
rg -n "match_funcclause_to_indexcol" --type c
# Search for FUNC_EXPR_FROM_PG_PROC macro definition
ast-grep --pattern 'define FUNC_EXPR_FROM_PG_PROC'
# Look for function_from field usage to understand how it's set
rg -n "function_from\s*=" --type c -A 2 -B 2Length of output: 9639
Replace elog(ERROR) with return NULL in match_funcclause_to_indexcol (indxpath.c:3031-3037)
Avoid aborting queries when encountering package or subprocedure functions in index qualifiers by returning NULL to indicate no match and let the planner continue.
🤖 Prompt for AI Agents
In src/backend/optimizer/path/indxpath.c around lines 3031 to 3037, replace the
call to elog(ERROR, ...) that aborts on non-pg_proc functions with logic that
returns NULL from match_funcclause_to_indexcol to indicate no index-qual match;
specifically remove the elog(ERROR) and instead return NULL (optionally keep or
adjust the surrounding comment to note subprocedure/package calls are
unsupported and should be skipped), so the planner can continue rather than
aborting the query.
| /* | ||
| * Only fold functions that originate from pg_proc. | ||
| * Package/subprocedure invocations are engine-specific and | ||
| * are not simplified here. | ||
| */ | ||
| if (FUNC_EXPR_FROM_PG_PROC(expr->function_from)) | ||
| { | ||
| if (expr->ref_pkgtype) | ||
| set_pkgtype_from_funcexpr(expr); | ||
|
|
||
| simple = simplify_function(expr->funcid, | ||
| expr->funcresulttype, | ||
| exprTypmod(node), | ||
| expr->funccollid, | ||
| expr->inputcollid, | ||
| &args, | ||
| expr->funcvariadic, | ||
| true, | ||
| true, |
There was a problem hiding this comment.
Don’t skip argument simplification for package functions
Once FUNC_EXPR_FROM_PG_PROC is false we now bypass simplify_function(), but nothing else walks the argument list. That means child expressions are left untouched: we lose constant folding, default/named argument rewriting, and even basic recursion into the argument tree for package routines. Any constant or simplifiable subexpression stays in its original form and all planner-time optimisations are skipped. Please keep the recursive simplification in the non‑pg_proc path as well. For example:
if (FUNC_EXPR_FROM_PG_PROC(expr->function_from))
{
if (expr->ref_pkgtype)
set_pkgtype_from_funcexpr(expr);
simple = simplify_function(expr->funcid,
expr->funcresulttype,
exprTypmod(node),
expr->funccollid,
expr->inputcollid,
&args,
expr->funcvariadic,
true,
true,
context);
}
+ else
+ {
+ args = (List *)
+ expression_tree_mutator((Node *) args,
+ eval_const_expressions_mutator,
+ context);
+ }📝 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.
| /* | |
| * Only fold functions that originate from pg_proc. | |
| * Package/subprocedure invocations are engine-specific and | |
| * are not simplified here. | |
| */ | |
| if (FUNC_EXPR_FROM_PG_PROC(expr->function_from)) | |
| { | |
| if (expr->ref_pkgtype) | |
| set_pkgtype_from_funcexpr(expr); | |
| simple = simplify_function(expr->funcid, | |
| expr->funcresulttype, | |
| exprTypmod(node), | |
| expr->funccollid, | |
| expr->inputcollid, | |
| &args, | |
| expr->funcvariadic, | |
| true, | |
| true, | |
| /* | |
| * Only fold functions that originate from pg_proc. | |
| * Package/subprocedure invocations are engine-specific and | |
| * are not simplified here. | |
| */ | |
| if (FUNC_EXPR_FROM_PG_PROC(expr->function_from)) | |
| { | |
| if (expr->ref_pkgtype) | |
| set_pkgtype_from_funcexpr(expr); | |
| simple = simplify_function(expr->funcid, | |
| expr->funcresulttype, | |
| exprTypmod(node), | |
| expr->funccollid, | |
| expr->inputcollid, | |
| &args, | |
| expr->funcvariadic, | |
| true, | |
| true, | |
| context); | |
| } | |
| else | |
| { | |
| /* still recurse into args for constant‐folding */ | |
| args = (List *) | |
| expression_tree_mutator((Node *) args, | |
| eval_const_expressions_mutator, | |
| context); | |
| } |
🤖 Prompt for AI Agents
In src/backend/optimizer/util/clauses.c around lines 2619-2637, the current
branch skips calling simplify_function for non-PG_PROC functions and also fails
to walk/simplify the argument list, leaving child expressions and constants
unsimplified; update the non-pg_proc path to still recursively simplify each
argument (mirror what simplify_function does for args) by invoking the same
argument-walking/simplification routine used for pg_proc functions (or apply the
expression-tree mutator/simplifier to each arg), preserve handling of
default/named/variadic rewrites where applicable, and then continue with the
package-specific handling so that constant folding and planner-time
simplifications are not skipped.
| /* replace last ',' to )' */ | ||
| buf.data[buf.len] = ')'; | ||
| } |
There was a problem hiding this comment.
Off-by-one when replacing trailing comma corrupts buffer
buf.data[buf.len] = ')' overwrites the null terminator, leaving the trailing comma and producing a non-terminated string. Replace the last comma at len-1 (and guard in case there is no comma).
Apply this diff:
- /* replace last ',' to )' */
- buf.data[buf.len] = ')';
+ /* replace last ',' with ')' safely */
+ if (buf.len > 0 && buf.data[buf.len - 1] == ',')
+ buf.data[buf.len - 1] = ')';
+ else
+ appendStringInfoChar(&buf, ')');📝 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.
| /* replace last ',' to )' */ | |
| buf.data[buf.len] = ')'; | |
| } | |
| /* replace last ',' with ')' safely */ | |
| if (buf.len > 0 && buf.data[buf.len - 1] == ',') | |
| buf.data[buf.len - 1] = ')'; | |
| else | |
| appendStringInfoChar(&buf, ')'); | |
| } |
🤖 Prompt for AI Agents
In src/backend/utils/adt/ruleutils.c around lines 3585-3587, the code overwrites
the string terminator by writing ')' to buf.data[buf.len]; instead replace the
trailing comma at index buf.len-1 (but only if buf.len>0 and
buf.data[buf.len-1]==','), then ensure the buffer remains null-terminated (set
buf.data[buf.len] = '\0' after modification); add a guard for the no-comma case
so you don't modify out-of-bounds memory.
| /* tuplestore_donestoring(tupstore); */ | ||
| table_close(proc_rel, NoLock); | ||
|
|
There was a problem hiding this comment.
Lock leak: mismatch between open and close lock modes
table_open(..., AccessShareLock) must be paired with table_close(..., AccessShareLock). Using NoLock here retains the lock until transaction end.
Apply this diff:
- table_close(proc_rel, NoLock);
+ table_close(proc_rel, AccessShareLock);📝 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.
| /* tuplestore_donestoring(tupstore); */ | |
| table_close(proc_rel, NoLock); | |
| /* tuplestore_donestoring(tupstore); */ | |
| table_close(proc_rel, AccessShareLock); |
🤖 Prompt for AI Agents
In src/backend/utils/adt/ruleutils.c around lines 14336-14338,
table_close(proc_rel, NoLock) is releasing with the wrong lock mode; change the
call to table_close(proc_rel, AccessShareLock) so it matches the table_open(...,
AccessShareLock) used earlier and prevents a lock leak, ensuring the open and
close use the same lock mode.
| get_internal_function_result_type(FuncExpr *fexpr, | ||
| ReturnSetInfo *rsinfo, | ||
| Oid *resultTypeId, | ||
| TupleDesc *resultTupleDesc) | ||
| ReturnSetInfo *rsinfo, | ||
| Oid *resultTypeId, | ||
| TupleDesc *resultTupleDesc) | ||
| { | ||
| plisql_internel_funcs_init(); | ||
|
|
||
| if (FUNC_EXPR_FROM_PACKAGE(fexpr->function_from)) | ||
| set_pkginfo_from_funcexpr(fexpr); | ||
|
|
||
| return plisql_internal_funcs.get_internal_func_result_type(fexpr, | ||
| NULL, | ||
| resultTypeId, | ||
| resultTupleDesc); | ||
| NULL, | ||
| resultTypeId, | ||
| resultTupleDesc); | ||
| } |
There was a problem hiding this comment.
Do not drop rsinfo when delegating to PL/iSQL hook
internal_get_result_type() now hands off non-pg_proc functions via get_internal_function_result_type(), but that routine immediately calls plisql_internal_funcs.get_internal_func_result_type(..., NULL, ...). By discarding the caller’s ReturnSetInfo, any package/internal SRF that expects to see rsinfo->expectedDesc (or other state) loses the only clue about the required tuple layout. In practice this regresses previously working RECORD-returning internal functions: type resolution falls back to TYPEFUNC_RECORD, and execution raises errors when the executor can’t materialize the result. Please forward the actual rsinfo down to the PL/iSQL hook instead of NULL.
🤖 Prompt for AI Agents
In src/backend/utils/fmgr/funcapi.c around lines 1837-1851, the function
get_internal_function_result_type discards the caller's ReturnSetInfo by passing
NULL into plisql_internal_funcs.get_internal_func_result_type; modify the call
to forward the original rsinfo pointer instead of NULL so package/internal SRFs
can read rsinfo->expectedDesc and other state; ensure the PL/iSQL hook's
function signature accepts the rsinfo parameter (adjust invocation only if the
hook supports it) and keep existing initialization and package-info setup
unchanged.
| 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.
Fix prototype/definition mismatch for plisql_subprocfunc_ref.
You added the bool include_out_arguments parameter to the header, but the implementation in pl_subproc_function.c (and all call sites) still use the previous signature. This will trigger a conflicting declaration error at build time. Please update the implementation and every caller (or drop the extra param) so the signatures stay in sync.
🤖 Prompt for AI Agents
In src/pl/plisql/src/pl_subproc_function.h around lines 233 to 242, the
prototype for plisql_subprocfunc_ref was changed to add a bool
include_out_arguments parameter but the implementation in pl_subproc_function.c
and all callers still use the old signature; update the function definition in
pl_subproc_function.c to match this new prototype and then update every call
site to pass the appropriate include_out_arguments value (or remove the header
addition if you prefer the old behavior), ensuring the parameter order and types
match exactly so the declaration and definition are consistent.
Summary by CodeRabbit