Callinto 5stable#937
Conversation
… psql and handle the reference of bind variables at the sql and plsql.
WalkthroughThis PR adds comprehensive Oracle SQL*PLUS style CALL statements with host variables (CALL INTO), enabling prepared statement execution via EXECUTE_RUN_PREPARE setting, extending psql with Oracle command parsing, and propagating call-context metadata through parser, backend, PL/iSQL, and libpq layers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as psql Client
participant Parser as Oracle Parser
participant Backend as Backend
participant PL as PL/iSQL
participant libpq as libpq/Ivy
Client->>Parser: Parse CALL func INTO :host_var
Parser->>Parser: Recognize CallStmt with<br/>hostvariable, do_from_call=true
Parser->>Backend: Send CallStmt with metadata
Backend->>Backend: Extract host variables<br/>via get_hostvariables()
Backend->>PL: Compile as inline block<br/>with do_from_call=true
PL->>PL: Propagate do_from_call<br/>to parse state
PL->>PL: Use explicit coercion<br/>for Oracle types
PL->>Backend: Execute with parameter typmod
Backend->>Backend: Bind OUT params using typmod
libpq->>Client: Return results via<br/>extended protocol
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 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 |
There was a problem hiding this comment.
Actionable comments posted: 28
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/backend/utils/fmgr/funcapi.c (1)
2074-2090: Clarify column numbering rationale and consider consistency.The column naming uses
i + 1(position in the full argument list, including IN parameters) whenout_parameter_column_positionis true, but usesnumoutargs + 1(sequential output parameter number) for the default "columnN" naming. This creates an inconsistency:For arguments
[IN, OUT, IN, OUT]:
- With GUC enabled:
_column_2,_column_4- With GUC disabled (no argnames):
column1,column2While using
i + 1may be intentional to preserve the original argument positions, this could confuse users when toggling the GUC or correlating column names with argument positions.Additionally, when
out_parameter_column_positionis true, user-provided argument names (argnames) are completely ignored. This might surprise users who expect their named parameters to be respected regardless of the GUC setting.Consider:
- Adding a comment explaining why
i + 1is used instead ofnumoutargs + 1- Documenting that argnames is ignored when the GUC is enabled
- Evaluating whether sequential numbering (
numoutargs + 1) would be more consistentAlso note that identical logic appears in
build_plisql_function_result_tupdesc_d(lines 2269-2283), suggesting potential for refactoring into a shared helper function to reduce duplication.src/interfaces/libpq/ivytest/testlibpq.c (3)
139-144: Build breaker: wrong exit_nicely signatureexit_nicely() takes no args, but called with one.
- exit_nicely(conn); + exit_nicely();
341-346: Potential NULL deref: thread_data.tconn2 is never initializedDefault branch uses tconn2 which is never assigned. This will crash if executed.
Suggested quick fixes (pick one):
- Initialize conn2 in main and set thread_data.tconn2, and Ivyfinish it; or
- Remove/guard default branch paths that use tconn2.
114-114: Typo in error message"dorp" → "drop".
- fprintf(stderr, "dorp table Ivyexec failed\n"); + fprintf(stderr, "drop table Ivyexec failed\n");src/interfaces/libpq/ivytest/testlibpq_prepare_plsql.c (1)
127-139: Fix buffer size for string bind; avoid truncation/incorrect OUT handlingYou pass strlen(z) as val_size for :z. That’s the current content length, not the buffer capacity. OUT values may be longer and will be reported as truncated. Pass the full buffer size (sizeof z) instead.
Apply for both name and positional binds:
- z, - strlen(z), + z, + sizeof(z),Repeat the same change in the positional bind of parameter 4.
Also applies to: 197-209
src/bin/psql/psqlplusparse.y (1)
285-289: Fix memory leak from get_guc_settings() in type rules.get_guc_settings() allocates; current comparisons don’t free the returned string.
Proposed pattern:
- if (pg_strcasecmp(get_guc_settings("nls_length_semantics"), "byte") == 0) - $$ = make_bindvartype(ORACHARBYTEOID, $3, $1); + { + char *len_sem = get_guc_settings("nls_length_semantics"); + bool is_byte = (len_sem && pg_strcasecmp(len_sem, "byte") == 0); + if (len_sem) pg_free(len_sem); + $$ = make_bindvartype(is_byte ? ORACHARBYTEOID : ORACHARCHAROID, $3, $1); + }Apply similarly to the VARCHAR2 branch.
Also applies to: 312-316
src/bin/psql/mainloop.c (1)
985-1060: Fix callback structure inconsistency; use proper Ora_psqlscan_callbacks instead of empty placeholder.Line 1003 creates scanner state with
&psqlplus_callbacks, but this is a locally-defined structure with NULL placeholders. The main oracle scanner at line 87 correctly uses&Ora_psqlscan_callbacks. Use the same callback structure here for consistency and proper initialization.Additionally:
- Lines 1010/1018: Consider using
psqlplus_yylex()wrapper instead of callingorapsql_yylex()directly for consistency with the wrapper pattern defined in psqlplusparse.y.- Haserror logic is functional but could be simplified: initialize
success = SendQuery(query_buf->data)in the else branch and only override tofalseif haserror is set, eliminating the conditional.src/backend/utils/adt/rowtypes.c (1)
2125-2129: Remove-1from pop_oraparam_stack() call arguments; current call violates Assert preconditionThe concern is valid. After
push_oraparam_stack()creates a new level,get_oraparam_level()captures matching top and current levels. Whenpop_oraparam_stack(oraparam_top_level - 1, oraparam_cur_level)is called, this subtracts 1 from the first argument only, producing arguments like(0, 1), which violatesAssert(top >= cur)in the common case where both start equal.Correct pattern per
pl_exec.c: call without the-1adjustment to preserve the captured relationship, or adjust both arguments consistently. Lines 2126, 2135, 2226, and 2242 require fixes.
🧹 Nitpick comments (40)
src/interfaces/libpq/ivytest/testlibpq.c (6)
349-351: Standardize integer endianness handling for OUT valuesYou print name3_out with ntohl but print prestmt3_* ints directly. Confirm the Ivy OUT/INOUT contract (host vs network order). If host-order, drop ntohl for consistency.
Option if host-order:
- printf("plsql_function_out: %s,%s,%d\n", name2_out, name_out,ntohl(name3_out)); + printf("plsql_function_out: %s,%s,%d\n", name2_out, name_out, name3_out);If network-order, apply ntohl to prestmt3_* too.
67-81: Optional: fix variable name typoanonymous_paramete_str → anonymous_parameter_str for clarity.
-const char *anonymous_paramete_str = +const char *anonymous_parameter_str = ... - const char *stmt3 = anonymous_paramete_str; + const char *stmt3 = anonymous_parameter_str;
242-246: Optional: zero indicator arrays before bindsDefensive init of prestmt3_indp mirrors string buffers init.
memset(name2_out, 0x00,256); +memset(prestmt3_indp, 0x00, sizeof(prestmt3_indp));Also applies to: 302-309
46-46: Consider int32_t for name3_out tooFor consistency with OID 23 and printing.
-int name3_out; +int32_t name3_out;And adjust sizeof in its bind if changed:
- sizeof(int), + sizeof(int32_t),
385-406: Minor: length init consistencyparam_lengths[0] is hardcoded to 1; others use strlen. Consider using strlen(custid) for consistency.
230-233: Leak/cleanup completenessIf you add conn2, also Ivyfinish(thread_data.tconn2) and set pointers to NULL in exit_nicely.
contrib/file_fdw/sql/ivy_file_fdw.sql (1)
5-5: Reset session toggle to avoid cross-test leakageEnable at start is fine. Add a matching reset at the end of the script to prevent state bleed into subsequent tests.
@@ -\set EXECUTE_RUN_PREPARE on +\set EXECUTE_RUN_PREPARE on @@ DROP ROLE regress_file_fdw_superuser, regress_file_fdw_user, regress_no_priv_user; +-- reset prepared-exec test mode +\set EXECUTE_RUN_PREPARE offsrc/include/fe_utils/psqlscan_int.h (2)
174-182: Document ownership for plisqlstmtspsqlplus_cmd_execute.plisqlstmts lacks ownership/length semantics. Please document who allocates/frees it to avoid leaks/misuse.
245-246: Ensure xcstate is consistently initialized and reset across scannersxcstate is added to PsqlScanStateData. It’s reset in ora_psql_scan_reset(), but verify that the generic psql scanner’s psql_scan_reset() also clears it, or initialize it defensively in psql_scan_create/reset to false.
src/oracle_fe_utils/ora_psqlscan.l (1)
417-424: Good: suppressing anonymous-block detection inside C-style commentsSetting xcstate on xcstart, clearing on xcstop, and gating token counting in ora_psqlscan_emit prevents false positives while inside comments. This tightens parsing without affecting normal tokens. One note: nesting is intentionally ignored in ; document that nested comments aren’t supported under Oracle mode to avoid surprises.
Also applies to: 438-447, 1686-1756
src/interfaces/libpq/ivy-exec.c (1)
166-205: Clarify Ivyconnectdb2 lifecycleIvyconnectdb2 wraps an existing PGconn without taking ownership. Ensure callers don’t pass such an Ivyconn to Ivyfinish (which closes PGconn). Consider adding an ownership flag to Ivyconn or assert in Ivyfinish to prevent double-close.
src/oracle_test/regress/sql/psql.sql (1)
6-6: Add matching reset for EXECUTE_RUN_PREPARETo keep tests order-independent, reset the toggle at the end:
@@ -\set EXECUTE_RUN_PREPARE on +\set EXECUTE_RUN_PREPARE on @@ \pset null '' DROP TABLE defprivs; +-- reset prepared-exec test mode +\set EXECUTE_RUN_PREPARE offsrc/oracle_test/regress/sql/join.sql (1)
6-6: Prevent session-scope bleed of EXECUTE_RUN_PREPAREConsider unsetting at the end to isolate test side-effects across scripts.
Example tail addition:
+ \unset EXECUTE_RUN_PREPAREPlease confirm the regression driver doesn’t rely on this variable remaining set past this file.
contrib/pg_stat_statements/sql/select.sql (1)
4-4: Scope EXECUTE_RUN_PREPARE to this scriptUnset at end to avoid affecting downstream scripts.
+ \unset EXECUTE_RUN_PREPAREIf global enablement is intended for the whole test run, ignore; otherwise please add the unset locally.
src/oracle_test/regress/sql/plancache.sql (1)
5-5: Tear down EXECUTE_RUN_PREPARE after useFor test isolation, explicitly unset it at the end of this script.
+ \unset EXECUTE_RUN_PREPAREConfirm whether other scripts assume it remains on.
src/oracle_test/regress/sql/select_parallel.sql (1)
5-5: Unset EXECUTE_RUN_PREPARE at script end for isolationAvoid cross-script behavioral changes by unsetting when done.
+ \unset EXECUTE_RUN_PREPAREConfirm the test runner doesn’t depend on it staying enabled.
src/include/parser/parse_param.h (1)
63-63: Align function name with existing parser setter styleConsider renaming setdynamic_callparser() to match local conventions (e.g., set_parseDynCallParser or set_dynamic_callparser) and add a brief comment describing when it should be toggled.
If renaming is too invasive for 5_STABLE, at least document usage in parse_param.h above this declaration.
src/include/parser/parse_node.h (1)
264-264: Consider adding a documentation comment for the new field.The new
do_from_callfield lacks inline documentation explaining its purpose and usage. Based on the AI summary, this flag tracks whether a DoStmt originated from a CALL-like context, but this should be documented in the code itself for maintainability.Apply this diff to add documentation:
+ /* true if currently parsing a DO statement that originated from a CALL */ bool do_from_call;src/pl/plisql/src/pl_exec.c (1)
8668-8696: Make the cast form explicit for clarity; keep EXPLICIT context.You use COERCION_EXPLICIT with COERCE_IMPLICIT_CAST. For readability and tooling, prefer COERCE_EXPLICIT_CAST; ParseParamVariable still unwraps (it only preserves COERCE_EXPLICIT_CALL).
Apply:
- cast_expr = coerce_to_target_type(NULL, - (Node *) placeholder, srctype, - dsttype, dsttypmod, - COERCION_EXPLICIT, - COERCE_IMPLICIT_CAST, - -1); + cast_expr = coerce_to_target_type(NULL, + (Node *) placeholder, srctype, + dsttype, dsttypmod, + COERCION_EXPLICIT, + COERCE_EXPLICIT_CAST, + -1);Additionally, consider extracting the ORA varchar/char “same-family” check into a helper (e.g., is_ora_char_family_pair(srctype, dsttype)) and add regression covering:
- CALL INTO with matching-family types and over-length value: truncates.
- Mismatched family: raises error.
Would you like a patch and tests?src/bin/psql/variables.c (1)
210-214: Defensive cleanup: guard PQclear for safety.valid_bind_variable_value() calls PQclear(res) unconditionally. While PQexec usually returns non-NULL, add a simple guard to be robust against connection/OOM failures:
- PQclear(res); + if (res) + PQclear(res);src/backend/parser/parse_param.c (2)
662-670: Misleading error text: no OUT-mode check is performed.The error “output parameter cannot be a duplicate bind” fires for any duplicate name in CALL context, regardless of mode. Either:
- Reword generically (e.g., “duplicate bind variable name in CALL”), or
- Check actual mode before raising.
Minimal rewording:
- /* ORA-06578: output parameter cannot be a duplicate bind */ - if (IsDynCallStmt) - ereport(ERROR, - (errcode(ERRCODE_AMBIGUOUS_PARAMETER), - errmsg("output parameter cannot be a duplicate bind"))); + if (IsDynCallStmt) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("duplicate bind variable name in CALL")));
2614-2621: Limit visibility of setdynamic_callparser().This helper is only used within this file. Make it static to avoid exporting an unnecessary symbol.
-void +static void setdynamic_callparser(bool value)src/backend/tcop/postgres.c (1)
2023-2033: Nit: fix comment typo“virtual type awalys has null value” → “virtual type always has null value”.
src/bin/psql/psqlplus.h (1)
74-91: Unify scanner/parser YYSTYPE expectations to avoid ABI mismatchesYou expose psqlplus_yylex(union YYSTYPE*) and orapsql_yylex(psql_YYSTYPE*). Ensure both parsers/bridges agree on the layout, or typedef one to the other in a single header to prevent subtle mismatches.
Consider:
-typedef union psql_YYSTYPE { ... } psql_YYSTYPE; -union YYSTYPE; +typedef union psql_YYSTYPE { ... } psql_YYSTYPE; +/* Bridge: make YYSTYPE alias if safe with bison config */ +typedef psql_YYSTYPE YYSTYPE;If Bison generates an incompatible YYSTYPE, keep the bridge helper translating between the two. Please confirm current generator settings.
src/interfaces/libpq/ivytest/testlibpq_call.c (1)
6-15: Remove duplicate include<string.h> is included twice.
src/bin/psql/psqlplusscan.l (1)
283-297: Prefer matching bind variables before single-colon tokensFlex chooses the longest match, so ':ident' will beat ':' even with {self} earlier. Still, moving {bindvarref} above {self} reduces ambiguity and eases maintenance.
- {self} { - cur_state->token_count++; - return yytext[0]; - } - - {bindvarref} { + {bindvarref} { /* * BINDVARREF is not a token number defined by bison, * it is just a simple macro within psqlplus.h. */ yylval->str = pg_strdup(yytext); cur_state->token_count++; return BINDVARREF; } + + {self} { + cur_state->token_count++; + return yytext[0]; + }src/pl/plisql/src/pl_comp.c (1)
1383-1387: Typmod propagation for inline paramsPassing param->ptypmod to plisql_build_datatype is a good fix for typmod-aware parameters. Consider guarding for param->ptypmod == -1 if any callers assume a default length.
src/oracle_test/regress/sql/ora_psql.sql (1)
486-510: Pre-clean test table to avoid flakinessIf a prior run left bindvar_test, CREATE TABLE will fail. Add a pre-clean:
drop table if exists bindvar_test; create table bindvar_test(a number);src/bin/psql/common.c (3)
2812-2865: Avoid duplicating AcceptResult logicIvyAcceptResult duplicates AcceptResult with slightly fewer statuses. Prefer reusing AcceptResult or refactor into a shared helper to reduce drift and maintenance.
3246-3341: Prefer reusing PrintQueryResultIvyPrintQueryResults largely mirrors PrintQueryResult with extra Oracle messages. Consider calling PrintQueryResult for common paths and only override the special-message cases to reduce duplication and inconsistencies (e.g., status printing and logging).
3570-3623: Limit symbol visibilitypsqlplus_skip_space need not be exported from common.c. Make it static to avoid symbol collisions.
-char * +static char * psqlplus_skip_space(char *query)src/bin/psql/psqlplusparse.y (2)
63-69: Remove unused token (TYPECAST) to keep grammar tight.TYPECAST is declared but unused here. Either wire it up or drop it.
-%token TYPECAST +/* TYPECAST token removed: unused in psqlplusparse.y */
593-604: psqlplus_yylex wrapper is correct if union field name is intentional.The indirection through orapsql_yylex using lvalp->psql_yysype matches the union. If the “yysype” spelling is unintentional, consider renaming to “psql_yystype” for clarity (no functional impact).
src/backend/oracle_parser/ora_gram.y (4)
10661-10668: Validate PARAMSLENGTH arity and non-negative lengthsGrammar collects paramslen but does not enforce that:
- params_length_list count equals param_mode_list count
- values are non-negative (and within sensible bounds, e.g., ≤ INT_MAX)
Add checks during parse analysis (or here) and emit a clear error if mismatched. Also ensure do_from_call is propagated to InlineCodeBlock/ExecuteDoStmt.
Also applies to: 10670-10679
10344-10353: Prefer size_t for lengths and restrict range earlyUsing SignedIconst allows negatives. Consider rejecting negative or zero values at parse time to fail fast:
- | params_length_list ',' SignedIconst + | params_length_list ',' SignedIconst { - $$ = lappend($1, makeInteger($3)); + if ($3 <= 0) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("parameter length must be positive"), + parser_errposition(@3))); + $$ = lappend($1, makeInteger($3)); }
655-661: param_mode should duplicate keyword text safelyparam_mode returns (Node*) makeString((char*)$1) from . Cast drops const and may reference static storage. Prefer duplicating:
- IN_P { $$ = (Node *) makeString((char *) $1); } + IN_P { $$ = (Node *) makeString(pstrdup($1)); }Apply similarly to OUT_P, INOUT, and IN OUT.
10661-10668: determineLanguage side-effect in DO USING pathdetermineLanguage(n->args) is called in both DO USING paths. Confirm it’s intended to set default PL/SQL language for anonymous DO blocks and that it doesn't alter behavior for non-Oracle compatibility modes. Add a brief comment to clarify intent.
src/backend/utils/adt/rowtypes.c (3)
2157-2174: Make language check case-insensitive and guard source_text
- Language names are case-insensitive; prefer pg_strcasecmp over strcmp.
- source_text may be NULL; compile_inline_internal should not receive NULL/empty.
- if (language && - strcmp(language, "plisql") == 0 && + if (language && + pg_strcasecmp(language, "plisql") == 0 && stmt->paramsmode == NIL) { PG_TRY(); { if (!plisql_internal_funcs.isload) { ... } - //plisql_internal_funcs.compile_inline_internal(sql); - plisql_internal_funcs.compile_inline_internal(source_text); + if (source_text == NULL || *source_text == '\0') + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("DO statement requires a non-empty body for language plisql"))); + plisql_internal_funcs.compile_inline_internal(source_text); nparams = calculate_oraparamname(¶mnames); }
2219-2220: Remove commented-out debug lineLeftover //plisql_internal_funcs.compile_inline_internal(sql); should be removed.
- //plisql_internal_funcs.compile_inline_internal(sql);
2145-2146: Optional: ensure cmdtag lifetime in multi-call contextCreateCommandName(...) likely returns a stable pointer, but to avoid lifetime ambiguity across SRF calls, consider copying into funcctx->multi_call_memory_ctx.
- user_fctx->cmdtag = CreateCommandName(linitial(parsetree)); + user_fctx->cmdtag = pstrdup(CreateCommandName(linitial(parsetree)));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (32)
contrib/file_fdw/expected/ivy_file_fdw.outis excluded by!**/*.outcontrib/pg_stat_statements/expected/ivy_squashing.outis excluded by!**/*.outcontrib/pg_stat_statements/expected/ivy_utility.outis excluded by!**/*.outcontrib/pg_stat_statements/expected/planning.outis excluded by!**/*.outcontrib/pg_stat_statements/expected/select.outis excluded by!**/*.outsrc/include/catalog/pg_proc.datis excluded by!**/*.datsrc/interfaces/libpq/ivytest/expected/testlibpq_call.outis excluded by!**/*.outsrc/interfaces/libpq/ivytest/expected/testlibpq_prepare_call.outis excluded by!**/*.outsrc/interfaces/libpq/ivytest/expected/testlibpq_prepare_plsql.outis excluded by!**/*.outsrc/interfaces/libpq/ivytest/expected/testlibpqstmt2_call.outis excluded by!**/*.outsrc/oracle_test/regress/expected/constraints.outis excluded by!**/*.outsrc/oracle_test/regress/expected/domain.outis excluded by!**/*.outsrc/oracle_test/regress/expected/functional_deps.outis excluded by!**/*.outsrc/oracle_test/regress/expected/functional_deps_1.outis excluded by!**/*.outsrc/oracle_test/regress/expected/invisible.outis excluded by!**/*.outsrc/oracle_test/regress/expected/join.outis excluded by!**/*.outsrc/oracle_test/regress/expected/ora_plisql.outis excluded by!**/*.outsrc/oracle_test/regress/expected/ora_psql.outis excluded by!**/*.outsrc/oracle_test/regress/expected/partition_prune.outis excluded by!**/*.outsrc/oracle_test/regress/expected/plancache.outis excluded by!**/*.outsrc/oracle_test/regress/expected/portals.outis excluded by!**/*.outsrc/oracle_test/regress/expected/prepare.outis excluded by!**/*.outsrc/oracle_test/regress/expected/psql.outis excluded by!**/*.outsrc/oracle_test/regress/expected/rowsecurity.outis excluded by!**/*.outsrc/oracle_test/regress/expected/select_parallel.outis excluded by!**/*.outsrc/oracle_test/regress/expected/select_views.outis excluded by!**/*.outsrc/oracle_test/regress/expected/transactions.outis excluded by!**/*.outsrc/oracle_test/regress/expected/updatable_views.outis excluded by!**/*.outsrc/oracle_test/regress/expected/xml.outis excluded by!**/*.outsrc/oracle_test/regress/expected/xml_1.outis excluded by!**/*.outsrc/oracle_test/regress/expected/xml_2.outis excluded by!**/*.outsrc/pl/plisql/src/expected/plisql_out_parameter.outis excluded by!**/*.out
📒 Files selected for processing (66)
contrib/file_fdw/sql/ivy_file_fdw.sql(1 hunks)contrib/ivorysql_ora/src/sysview/sysview--1.0.sql(1 hunks)contrib/pg_stat_statements/sql/ivy_squashing.sql(1 hunks)contrib/pg_stat_statements/sql/ivy_utility.sql(1 hunks)contrib/pg_stat_statements/sql/planning.sql(1 hunks)contrib/pg_stat_statements/sql/select.sql(1 hunks)src/backend/commands/functioncmds.c(1 hunks)src/backend/nodes/params.c(1 hunks)src/backend/oracle_parser/ora_gram.y(8 hunks)src/backend/parser/analyze.c(1 hunks)src/backend/parser/parse_param.c(6 hunks)src/backend/tcop/postgres.c(6 hunks)src/backend/utils/adt/rowtypes.c(5 hunks)src/backend/utils/fmgr/funcapi.c(1 hunks)src/bin/psql/common.c(4 hunks)src/bin/psql/common.h(2 hunks)src/bin/psql/mainloop.c(7 hunks)src/bin/psql/psqlplus.h(1 hunks)src/bin/psql/psqlplusparse.y(9 hunks)src/bin/psql/psqlplusscan.l(11 hunks)src/bin/psql/settings.h(1 hunks)src/bin/psql/startup.c(3 hunks)src/bin/psql/variables.c(3 hunks)src/bin/psql/variables.h(1 hunks)src/include/catalog/dependency.h(1 hunks)src/include/fe_utils/psqlscan_int.h(2 hunks)src/include/nodes/params.h(1 hunks)src/include/nodes/parsenodes.h(3 hunks)src/include/oracle_parser/ora_kwlist.h(1 hunks)src/include/parser/parse_node.h(1 hunks)src/include/parser/parse_param.h(1 hunks)src/interfaces/libpq/exports.txt(1 hunks)src/interfaces/libpq/ivy-exec.c(37 hunks)src/interfaces/libpq/ivytest/Makefile(1 hunks)src/interfaces/libpq/ivytest/regression.txt(1 hunks)src/interfaces/libpq/ivytest/testlibpq.c(7 hunks)src/interfaces/libpq/ivytest/testlibpq_call.c(1 hunks)src/interfaces/libpq/ivytest/testlibpq_prepare_call.c(1 hunks)src/interfaces/libpq/ivytest/testlibpq_prepare_plsql.c(1 hunks)src/interfaces/libpq/ivytest/testlibpqstmt2_call.c(1 hunks)src/interfaces/libpq/libpq-ivy.h(3 hunks)src/interfaces/libpq/meson.build(1 hunks)src/oracle_fe_utils/ora_psqlscan.l(6 hunks)src/oracle_test/regress/parallel_schedule(1 hunks)src/oracle_test/regress/sql/constraints.sql(1 hunks)src/oracle_test/regress/sql/domain.sql(1 hunks)src/oracle_test/regress/sql/functional_deps.sql(1 hunks)src/oracle_test/regress/sql/invisible.sql(1 hunks)src/oracle_test/regress/sql/join.sql(1 hunks)src/oracle_test/regress/sql/ora_plisql.sql(3 hunks)src/oracle_test/regress/sql/ora_psql.sql(3 hunks)src/oracle_test/regress/sql/partition_prune.sql(1 hunks)src/oracle_test/regress/sql/plancache.sql(1 hunks)src/oracle_test/regress/sql/portals.sql(1 hunks)src/oracle_test/regress/sql/prepare.sql(1 hunks)src/oracle_test/regress/sql/psql.sql(1 hunks)src/oracle_test/regress/sql/rowsecurity.sql(1 hunks)src/oracle_test/regress/sql/select_parallel.sql(1 hunks)src/oracle_test/regress/sql/select_views.sql(1 hunks)src/oracle_test/regress/sql/transactions.sql(1 hunks)src/oracle_test/regress/sql/updatable_views.sql(1 hunks)src/oracle_test/regress/sql/xml.sql(1 hunks)src/pl/plisql/src/pl_comp.c(4 hunks)src/pl/plisql/src/pl_exec.c(2 hunks)src/pl/plisql/src/pl_handler.c(3 hunks)src/pl/plisql/src/plisql.h(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (16)
src/bin/psql/common.h (1)
src/bin/psql/common.c (3)
psqlplus_skip_space(3569-3623)get_hostvariables(3631-3719)SendQuery_PBE(3725-4049)
src/backend/parser/analyze.c (1)
src/backend/parser/parse_coerce.c (1)
coerce_to_target_type(84-138)
src/bin/psql/variables.h (1)
src/bin/psql/variables.c (1)
BindVariableExist(1080-1108)
src/interfaces/libpq/libpq-ivy.h (1)
src/interfaces/libpq/ivy-exec.c (4)
Ivyconnectdb(127-164)Ivyconnectdb2(170-205)Ivyfinish2(210-240)IvyStmtExecute2(2475-2742)
src/include/parser/parse_param.h (1)
src/backend/parser/parse_param.c (1)
setdynamic_callparser(2617-2621)
src/backend/parser/parse_param.c (1)
src/backend/utils/error/elog.c (2)
errcode(853-864)errmsg(1070-1086)
src/interfaces/libpq/ivytest/testlibpq_call.c (2)
src/interfaces/libpq/ivytest/testlibpq_prepare_call.c (2)
exit_nicely(30-35)exec_prepare_func(37-177)src/interfaces/libpq/ivy-exec.c (11)
Ivyfinish(943-973)IvyCreatePreparedStatement(251-307)IvybindOutParameterByPos(1123-1144)IvyexecPreparedStatement(318-543)IvyresultStatus(1028-1035)Ivyclear(925-937)IvyFreePreparedStatement(837-918)Ivyconnectdb(127-164)Ivystatus(978-985)IvyerrorMessage(1091-1098)Ivyexec(990-1023)
src/pl/plisql/src/pl_handler.c (3)
src/pl/plisql/src/pl_comp.c (1)
plisql_compile_inline(1256-1481)src/include/utils/palloc.h (1)
MemoryContextSwitchTo(123-130)src/backend/utils/error/elog.c (4)
CopyErrorData(1750-1814)FlushErrorState(1871-1884)ThrowErrorData(1899-1948)FreeErrorData(1822-1827)
src/bin/psql/startup.c (1)
src/bin/psql/variables.c (3)
SetVariable(493-589)ParseVariableBool(318-356)SetVariableHooks(607-662)
src/backend/tcop/postgres.c (2)
src/backend/utils/error/elog.c (2)
errcode(853-864)errmsg(1070-1086)src/include/nodes/pg_list.h (2)
list_length(151-155)list_nth(298-303)
src/pl/plisql/src/pl_exec.c (2)
src/backend/parser/parse_param.c (1)
ParseParamVariable(437-470)src/backend/parser/parse_coerce.c (1)
coerce_to_target_type(84-138)
src/bin/psql/common.c (6)
src/interfaces/libpq/fe-exec.c (9)
PQresultStatus(3418-3424)PQexec(2270-2278)PQntuples(3488-3494)PQclear(717-768)PQcmdStatus(3759-3765)PQoidValue(3800-3818)PQbinaryTuples(3504-3510)PQgetvalue(3883-3889)PQgetisnull(3908-3917)src/interfaces/libpq/fe-connect.c (3)
PQerrorMessage(7623-7638)PQtransactionStatus(7568-7576)PQclientEncoding(7713-7719)src/bin/psql/variables.c (3)
SetVariable(493-589)BindVariableExist(1080-1108)AssignBindVariable(974-1031)src/interfaces/libpq/fe-misc.c (1)
PQmblenBounded(1265-1269)src/interfaces/libpq/ivy-exec.c (9)
Ivyconnectdb2(170-205)Ivystatus(978-985)IvyerrorMessage(1091-1098)IvyHandleAlloc(1149-1218)IvyStmtPrepare(1245-1279)IvyBindByPos(2024-2053)IvyStmtExecute2(2475-2742)IvyresultStatus(1028-1035)Ivyclear(925-937)src/common/wchar.c (1)
pg_encoding_max_length(2301-2313)
src/backend/utils/adt/rowtypes.c (9)
src/backend/parser/parse_param.c (4)
backward_oraparam_stack(636-645)pop_oraparam_stack(593-621)set_ParseDynSql(2528-2532)calculate_oraparamname(2698-2716)src/backend/utils/mmgr/mcxt.c (2)
palloc(1320-1348)pstrdup(1702-1706)src/backend/utils/cache/syscache.c (2)
SearchSysCache1(220-229)ReleaseSysCache(268-272)src/backend/utils/fmgr/fmgr.c (1)
fmgr_info(129-133)src/backend/access/common/tupdesc.c (2)
CreateTemplateTupleDesc(181-219)TupleDescInitEntry(850-920)src/backend/executor/execTuples.c (1)
BlessTupleDesc(2305-2313)src/include/utils/palloc.h (1)
MemoryContextSwitchTo(123-130)src/backend/access/common/heaptuple.c (1)
heap_form_tuple(1149-1235)src/include/funcapi.h (1)
HeapTupleGetDatum(317-321)
src/bin/psql/mainloop.c (1)
src/bin/psql/common.c (5)
psqlplus_skip_space(3569-3623)standard_strings(2601-2615)get_hostvariables(3631-3719)SendQuery_PBE(3725-4049)SendQuery(1210-1431)
src/pl/plisql/src/plisql.h (1)
src/pl/plisql/src/pl_comp.c (1)
plisql_compile_inline(1256-1481)
src/interfaces/libpq/ivy-exec.c (3)
src/common/wchar.c (1)
pg_encoding_max_length(2301-2313)src/interfaces/libpq/fe-connect.c (1)
PQerrorMessage(7623-7638)src/interfaces/libpq/fe-exec.c (7)
PQexecStart(2352-2413)PQsendPrepare(1543-1633)PQexecFinish(2418-2448)PQresultStatus(3418-3424)PQclear(717-768)PQsendQueryPrepared(1640-1674)PQgetisnull(3908-3917)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: pg_regression (ubuntu-latest)
- GitHub Check: oracle_pg_regression (ubuntu-latest)
- GitHub Check: meson_build (ubuntu-latest)
- GitHub Check: oracle_regression (ubuntu-latest)
- GitHub Check: contrib_regression
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (60)
contrib/ivorysql_ora/src/sysview/sysview--1.0.sql (1)
1089-1089: Inconsistent owner comparison method across USER_ views.*Line 1089 now uses
pg_get_userbyid(c.relowner) = current_userfor owner filtering, but other USER_* views (lines 262, 293, 407, 1038) still use::REGROLE = CURRENT_USER::REGROLE. Additionally, line 1192 (USER_VIEWS) already uses thepg_get_userbyid()approach.These comparison methods are semantically different:
::REGROLEcomparison operates on role OIDspg_get_userbyid()converts OID to role name string for comparisonThis inconsistency could lead to subtle behavioral differences. Consider whether all USER_* views should adopt the same pattern for consistency and predictability.
src/oracle_test/regress/sql/ora_plisql.sql (4)
805-805: Approved: Consistent block termination formatting.Minor slash delimiter formatting aligns with existing anonymous block conventions throughout the file.
Also applies to: 812-812
968-978: Approved: Proper test coverage for CALL syntax with no-argument procedures.Tests correctly validate that empty parentheses are required when calling parameterless procedures (CALL procedure() succeeds; CALL procedure fails). Cleanup is in place.
1193-1266: Approved: Comprehensive tests for no-argument procedures/functions and default parameters.Tests correctly validate empty-parentheses requirement and INTO clause behavior. Variable declarations and cleanup are properly structured. Aligns with Oracle compatibility for CALL INTO syntax.
1268-1475: Approved: Extensive bind variable type handling and CALL INTO feature tests.Excellent coverage of type compatibility (varchar2/char variants, truncation, implicit casting, error cases), IN/OUT parameter binding, and duplicate-bind error detection. Cleanup is complete. The truncation and type-mismatch behavior documented in lines 1268–1272 is clearly tested. Minor formatting variations in variable declarations (with/without semicolons, initialization patterns) appear intentional for syntax coverage.
src/interfaces/libpq/ivytest/testlibpq.c (3)
79-79: String literal form is correct for PL/SQL-style blockSingle-quoted string assignment looks good.
366-366: Arg modes match param_types3 (all INOUT)Aligned with 23/25 | 0x60000000 above. Good.
Please confirm server expects all four as INOUT for this block.
421-453: Result-format vs printingYou set result_format=1 for stmt1 but print with %s. If 1 means binary, this will misprint.
Please confirm IvyexecPreparedStatement’s result_format semantics; switch to text or decode binary as needed.
src/oracle_test/regress/parallel_schedule (1)
162-162: LGTMAdded trailing newline only. No functional impact.
src/bin/psql/common.h (1)
19-19: No build portability issues found—include paths already configuredThe build system already includes src/interfaces/libpq in the compile path. Autotools uses
-I$(libpq_srcdir)in CPPFLAGS, and Meson includes the libpq dependency which provides necessary include directories. The header is installed as a public interface and already used successfully in other parts of the codebase.src/include/catalog/dependency.h (1)
103-103: LGTM! Typo fix improves documentation.Good catch correcting "funtions" to "functions" in the comment.
src/oracle_test/regress/sql/select_views.sql (1)
6-6: LGTM! Directive correctly enables prepared statement testing.The
EXECUTE_RUN_PREPAREdirective is appropriately placed at the top of the test file to enable prepared statement execution for all subsequent queries.src/oracle_test/regress/sql/portals.sql (1)
5-5: LGTM! Consistent with test framework updates.The directive is correctly positioned before the test begins.
src/oracle_test/regress/sql/functional_deps.sql (1)
3-3: LGTM! Proper test setup.contrib/pg_stat_statements/sql/ivy_squashing.sql (1)
4-4: LGTM! Extends prepared statement testing to contrib modules.src/backend/commands/functioncmds.c (1)
2868-2868: LGTM! Proper flag propagation for CALL context tracking.The
do_from_callflag is correctly propagated from DoStmt to InlineCodeBlock, enabling the inline code handler to distinguish DO statements originating from CALL contexts. The placement follows the existing pattern for other field initializations.src/oracle_test/regress/sql/invisible.sql (1)
2-2: LGTM! Consistent test configuration.src/oracle_test/regress/sql/partition_prune.sql (1)
5-5: LGTM! Test setup aligns with PR objectives.src/oracle_test/regress/sql/updatable_views.sql (1)
5-5: LGTM! Consistent test configuration.The addition of
\set EXECUTE_RUN_PREPARE onaligns with the PR's objective to enable prepared statement execution across Oracle regression tests. This is consistent with similar changes in other test files.src/oracle_test/regress/sql/prepare.sql (1)
5-5: LGTM! Appropriate for prepared statement tests.Adding
\set EXECUTE_RUN_PREPARE onto the prepare.sql test file is particularly appropriate given the test's focus on prepareable statements. This ensures the test exercises the new prepared execution paths.src/backend/nodes/params.c (1)
65-70: LGTM! Proper typmod initialization.The initialization of
ptypmodto -1 follows PostgreSQL conventions for unspecified type modifiers. The updated comment accurately describes the initialization, and the change aligns with the broader PR's parameter metadata enhancements.src/bin/psql/variables.h (1)
133-133: LGTM! Well-designed API addition.The
BindVariableExistfunction provides a clean way to check for bind variable existence and retrieve the variable structure. The naming is consistent with existing functions, and returning a pointer tostruct _variableallows callers to both check existence and access the variable in one call.src/include/oracle_parser/ora_kwlist.h (1)
394-394: LGTM! Proper keyword addition.The new "paramslength" keyword is correctly positioned in alphabetical order (as required by gen_keywordlist.pl) and appropriately categorized as an UNRESERVED_KEYWORD. This enables the PARAMSLENGTH clause for explicit parameter-length specification in DO statements.
src/interfaces/libpq/meson.build (1)
19-20: Both new source files verified as present.The files
ivy-exec.candivy_sema.care confirmed to exist insrc/interfaces/libpq/. The latter is a symbolic link pointing toposix_ivysema.c, which is the appropriate pattern for platform-specific implementation selection. The addition to the meson.build is correct.src/bin/psql/settings.h (1)
172-172: All initialization concerns verified—no issues found.The
execute_run_preparefield is properly initialized through multiple layers:
- The field exists at settings.h line 172 within the PsqlSettings struct
- The global
psetvariable is zero-initialized by default (C static initialization)- The EXECUTE_RUN_PREPARE variable is explicitly initialized to
"off"at startup.c:205- The variable hook is properly registered at startup.c:1320-1322 with the
execute_run_prepare_hookfunction- The hook correctly parses the variable value and assigns it to
pset.execute_run_prepareThe implementation is complete and correct.
src/interfaces/libpq/exports.txt (1)
236-238: Export ordinals and symbols verified; no issues found.All three symbols are correctly declared in
libpq-ivy.h, implemented inivy-exec.c(which is compiled into libpq via meson.build), and already in use throughout the codebase. Ordinals 335–337 follow sequentially with no gaps. Build integration is confirmed.src/bin/psql/startup.c (3)
205-205: Good defaulting for new boolean psql varSetting EXECUTE_RUN_PREPARE to "off" at init aligns with bool_substitute_hook semantics. Looks consistent with other psql vars.
1234-1239: Hook correctly parses EXECUTE_RUN_PREPAREexecute_run_prepare_hook cleanly delegates to ParseVariableBool into pset.execute_run_prepare. All good.
1320-1322: Variable registered with proper hooksRegistering EXECUTE_RUN_PREPARE with bool_substitute_hook and the assign hook ensures tab-completion presence and live updates.
src/pl/plisql/src/pl_handler.c (2)
407-407: API change usage is correctplisql_compile_inline now receives fromcall; passing codeblock->do_from_call matches the new signature.
375-375: Context capture is fine but couple it with deferred rethrowSaving oldcontext early is good. Ensure it’s used only to copy error data and not to short‑circuit cleanup (see previous comment).
src/backend/parser/analyze.c (1)
3018-3037: Verify intent of COERCE_IMPLICIT_CAST with COERCION_EXPLICIT for Oracle typesThe review correctly identifies semantic inconsistency: COERCION_EXPLICIT indicates an explicit cast operation, but COERCE_IMPLICIT_CAST suppresses its display. The proper form is COERCE_EXPLICIT_CAST, which exists elsewhere in the codebase (e.g., parse_expr.c).
However, this same pattern (COERCION_EXPLICIT + COERCE_IMPLICIT_CAST) appears in similar Oracle type handling at pl_exec.c:8686-8687, suggesting intentional design. Without clear documentation explaining why IMPLICIT_CAST form is chosen despite EXPLICIT context, the inconsistency warrants clarification.
Additionally, the condition is asymmetrical: the second part
(targettype == ORAVARCHARCHAROID || targettype == ORAVARCHARBYTEOID)allows any source type to take the EXPLICIT path to ORAVARCHAR targets, whereas the first part requires both source and target to be Oracle CHAR/BYTE variants. This loose condition may be intentional for SRS-SQL-CALL requirements, but should be verified.
- Fix the coercion form mismatch: change COERCE_IMPLICIT_CAST to COERCE_EXPLICIT_CAST (line 3028).
- Verify the condition logic handles source type consistently for all three Oracle type families.
src/bin/psql/variables.c (2)
1017-1022: Good: avoid overwriting with NULL on failed cast.Only assigning current->value when newvalue is non-NULL prevents losing existing values on invalid input.
1076-1108: ****BindVariableExist is already properly declared in src/bin/psql/variables.h (line 133) with struct _variable fully exposed. The API design is intentional: external code (common.c:3858) uses it to look up bind variables for parameterized query execution. The returned pointers are used read-only—passed to psql_exec_pbe for variable value extraction, not for mutation. No action required.
Likely an incorrect or invalid review comment.
src/backend/parser/parse_param.c (2)
2512-2520: Good: walker now descends into CallStmt.Including CallStmt.funccall and CallStmt.hostvariable ensures complete parameter scanning in CALL paths.
579-581: Reset of IsDynCallStmt on stack push looks correct.Reinitializing dynamic flags when the OraParam stack is created avoids stale state.
src/pl/plisql/src/plisql.h (1)
1320-1320: All call sites of plisql_compile_inline() have been properly updated.Verification confirms one call site at pl_handler.c:407 is passing all three arguments (including the new
bool fromcallparameter), and the implementation at pl_comp.c:1324 correctly assigns this parameter tofunction->do_from_call. No incomplete or orphaned call sites remain.src/include/nodes/parsenodes.h (1)
3614-3616: Ensure node serialization is updated for new fields (DoStmt/CallStmt).DoStmt.paramslen/do_from_call and CallStmt.hostvariable/callinto must be handled in outfuncs.c/readfuncs.c/copyfuncs.c; otherwise nodeToString, debug dumps, or any ruleutils usage will drop them. InlineCodeBlock.do_from_call is nodetag_only, so fine.
Please confirm corresponding WRITE_/READ_/COPY_ macros exist for these fields. If missing, add them.
Also applies to: 3627-3627, 3651-3653
src/backend/tcop/postgres.c (2)
1130-1142: Avoid hardcoding OUT parameter ptype to INT4 (23) in anonymous DO initializationInitializing all OUT params as INT4 risks type mismatches during analysis/planning. Consider using InvalidOid (0) and letting the PL/iSQL layer or later binding resolve types, or pass no fixed param types for DO and rely on varparams. If fixed types are truly required for this path, document why 23 is safe or derive from paramsmode metadata.
Would you like a patch switching to InvalidOid and using pg_analyze_and_rewrite_varparams for this branch?
1979-2015: Protocol alignment for OUT parameters during BindYou read plength and optionally a value for every parameter before handling pmode == 'o'. If the frontend omits OUT values (common), this can desynchronize. Confirm that clients always send placeholders/lengths for OUT/INOUT here; otherwise, gate reading by pmode first.
src/interfaces/libpq/libpq-ivy.h (1)
223-227: Symbols are already exported—no action requiredVerification confirms all three symbols (Ivyconnectdb2, Ivyfinish2, IvyStmtExecute2) are present in exports.txt with proper ordinal assignments (lines 236–238). ABI consistency is maintained.
src/bin/psql/psqlplus.h (1)
67-68: Remove this review comment—BINDVARREF is not a Bison token and poses no collision risk.The lexer explicitly documents that "BINDVARREF is not a token number defined by bison, it is just a simple macro within psqlplus.h" (psqlplusscan.l:290–291). BINDVARREF (3000) is a semantic marker returned by the lexer to identify bind variable references, not a Bison token competing for token numbers. Bison auto-assigns token values sequentially starting at 256 for declared tokens like K_VAR, K_VARIABLE, etc., which occupy a range safely below 3000. No collision occurs.
Likely an incorrect or invalid review comment.
src/bin/psql/psqlplusscan.l (1)
39-46: Confirm keyword array remains ASCII-sortedThe binary search requires psqlplus_keywords to be ASCII-sorted. Adding "exec", "execute", and "var" appears correct here; please confirm no other insertions break sort order.
src/pl/plisql/src/pl_comp.c (2)
1324-1325: Call-context propagation looks goodSetting function->do_from_call = fromcall enables downstream pstate->do_from_call to be set in resolve_column_ref. LGTM.
1257-1257: Signature change ripple check verified—no issues foundAll declarations, definitions, and call sites for
plisql_compile_inlinehave been correctly updated with the newbool fromcallparameter. The function signature is consistent across the header file (plisql.h:1320), the definition (pl_comp.c:1257), and the only identified call site (pl_handler.c:407), which correctly passescodeblock->do_from_callas the third argument.src/bin/psql/common.c (1)
1534-1544: Oracle mode describe pathRouting DescribeQuery output through IvyPrintQueryResults in DB_ORACLE is fine. Ensure both quiet and host-variable scenarios emit consistent messages with the execution path.
src/interfaces/libpq/ivytest/testlibpq_prepare_call.c (1)
12-12: Duplicate include<string.h> is included twice. Remove the second include.
Apply this diff:
-#include <string.h>Likely an incorrect or invalid review comment.
src/bin/psql/mainloop.c (4)
963-977: Correct EOL send behavior for EXECUTE path.Treating ORAPSCAN_EOL as a send trigger when exec_dostmt is set, and resetting scanner/state/history flags, is sound. No change requested.
638-641: Nice: accept VAR as alias of VARIABLE.Good UX parity with SQL*Plus.
548-601: All edge cases properly handled; EXECUTE rewrite flow is correct.Verification confirms:
- Empty/whitespace input: Guarded by
psqlplus_skip_space()and null/newline check (lines 561–565).- Semicolon handling: Conditionally appended if absent (lines 573–574).
- Multi-statement bodies: Wrapped in
BEGIN...END;block (lines 571–576).- No unintended persistence:
exec_dostmtis a per-iteration local variable (line 101), declared fresh each loop; reset tofalseonORAPSCAN_EOL(line 974). All 5 references are accounted for with clean control flow.
460-465: ****The original review comment mischaracterizes the code. Both
psqlplus_callbacks(defined inpsqlplus.h:93) andOra_psqlscan_callbacks(defined inmainloop.c:32) are the same type (const Ora_psqlScanCallbacks) and compile safely—there is no type mismatch.The code uses two intentionally separate callback objects for different scanning contexts:
Ora_psqlscan_callbacksat line 87: persistent global Oracle scannerpsqlplus_callbacksat lines 460 and 1003: ephemeral local Oracle command-specific parsersThe
psqlplus_callbacksis static-scoped to the header and serves as a placeholder for future Oracle command-specific callbacks. Replacing it with the globalOra_psqlscan_callbackswould incorrectly mix global scanner state into local command parsing. The original code is correct as written.Likely an incorrect or invalid review comment.
src/bin/psql/psqlplusparse.y (3)
488-496: make_exec_node() is fine.Straightforward init of the execute node. LGTM.
446-460: Manual verification of parser conflicts required—bison unavailable in sandbox.The grammar structure shows a legitimate conflict risk: K_EXEC, K_EXECUTE, and K_VAR appear in both
variable_keyword/exec_keywordrules and theunreserved_keywordrule. When the parser encounters these tokens (e.g., inbind_varname,bind_varvalue, orprint_itemscontexts), it could face shift/reduce conflicts choosing between reducing tounreserved_keywordor shifting to match statement-level rules.To verify the concern, run locally:
bison -Wall -Wcounterexamples -v -o /tmp/psqlplusparse.c src/bin/psql/psqlplusparse.y grep -i conflict /tmp/psqlplusparse.outputConfirm the conflict count is acceptable (ideally 0, or document any expected conflicts).
404-424: Verify whitespace handling for EXEC statement capture with test case covering variadic spacing.The review comment requests verification that the offset calculation in the
exec_stmtrule correctly captures SQL*PLUS EXEC statements with variable whitespace (e.g., "EXEC do_something(:x)"). While the offset logic (strlen(state->scanbuf)as offset intoscanline) appears to rely on downstream trimming viapsqlplus_skip_space()in mainloop.c (line 557), the relationship betweenscanbuf(outer-level buffer start) andscanline(current input line) during EXEC keyword parsing requires manual verification.Confirm that:
- Commands like "EXEC statement" with multiple spaces capture the statement correctly
- Edge cases with tabs or mixed whitespace are handled as intended
- The downstream
psqlplus_skip_space()call adequately handles any residual leading whitespace from the offset calculationsrc/backend/oracle_parser/ora_gram.y (2)
9438-9443: Check for new precedence/ambiguity with '=' in parameter defaultsAllowing plassign_equals (':=' or '=') in func_arg_with_default can conflict with existing productions (e.g., named args with '=', default value expressions). Please reconfirm %expect 0 and add tests like:
- CREATE FUNCTION f(a int := 1) ...
- CREATE FUNCTION f(a int = 1) ...
- CREATE FUNCTION f(a text default 'x') ...
to ensure no shift/reduce regressions.
852-853: Keyword table synchronization for PARAMSLENGTHPARAMSLENGTH is added as a token and listed as unreserved and bare label. Ensure:
- src/include/oracle_parser/ora_kwlist.h contains PARAMSLENGTH entry with the same category.
- No collisions with existing function names (e.g., PARAMS LENGTH-like identifiers).
Run parser generation to confirm no conflicts.
Also applies to: 20334-20335, 21041-21042
src/backend/utils/adt/rowtypes.c (4)
26-26: Include looks correct for CreateCommandName()Adding tcop/utility.h to use CreateCommandName() is appropriate.
2234-2239: ****The code is correct. Verification confirms that
CallStmt.callintois achar *field (line 3652,src/include/nodes/parsenodes.h) containing a NUL-terminated C string, not a Node. Thepstrdup()call is appropriate for duplicating C strings, and the NULL check is properly in place. No encoding or size safety issues exist.
2198-2221: Remove this review comment — headers are properly includedThe symbols
plisql_internal_funcsandLANG_PLISQL_OIDare correctly declared through headers already included in rowtypes.c:
#include "funcapi.h"declaresplisql_internal_funcs(line 210 of funcapi.h)#include "commands/proclang.h"definesLANG_PLISQL_OIDas a macro (line 24 of proclang.h)The code will compile without undefined symbol errors.
Likely an incorrect or invalid review comment.
2253-2269: All in-tree consumers verified and updated; schema change is correctly implementedVerification confirms:
- psql (src/bin/psql/common.c lines 3669–3705) correctly handles the 3-column schema and header row: reads cmdtag and hint from row 0, then processes placeholder rows 1..ntuples-1
- libpq callers (src/interfaces/libpq/ivy-exec.c lines 3829, 4115) validate n_fields==3 and explicitly extract header row (row 0) with position==0 check
- Test expectations (src/pl/plisql/src/expected/plisql_out_parameter.out lines 1234–1239) show header row followed by parameter rows across 3 columns
The wire-protocol change is intentional and properly updated throughout the codebase. No backward-compatibility concerns remain unaddressed.
| | CALL func_application INTO ORAPARAM | ||
| { | ||
| CallStmt *n = makeNode(CallStmt); | ||
| OraParamRef *hostvar = makeNode(OraParamRef); | ||
| char *callstr = NULL; | ||
| n->funccall = castNode(FuncCall, $2); | ||
| hostvar->number = 0; | ||
| hostvar->location = @4; | ||
| hostvar->name = $4; | ||
| n->hostvariable = hostvar; | ||
| callstr = pnstrdup(pg_yyget_extra(yyscanner)->core_yy_extra.scanbuf + @2, @3 - @2); | ||
| n->callinto = psprintf("%s := %s;", $4, callstr); | ||
| pfree(callstr); | ||
| $$ = (Node *)n; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Guard scanbuf slicing and sanitize the constructed assignment
Building n->callinto via pnstrdup(scanbuf + @2, @3 - @2) assumes valid locations. If @2 is -1 or @3 <= @2 (comments/whitespace/lexer quirks), this can underflow or slice garbage. Also, the captured text may include trailing spaces.
- Add bounds checks and trim the substring (e.g., skip trailing whitespace before psprintf).
- Consider using updateRawStmtEnd-style logic (already present below) for consistency.
Example:
- callstr = pnstrdup(pg_yyget_extra(yyscanner)->core_yy_extra.scanbuf + @2, @3 - @2);
+ if (@2 >= 0 && @3 > @2) {
+ const char *base = pg_yyget_extra(yyscanner)->core_yy_extra.scanbuf;
+ int len = @3 - @2;
+ while (len > 0 && isspace((unsigned char)base[@2 + len - 1])) len--;
+ callstr = pnstrdup(base + @2, len);
+ } else {
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("cannot construct CALL INTO assignment"),
+ parser_errposition(@3)));
+ }Also verify OraParamRef has all fields initialized (indicator, datatype, etc.) or zeroed by makeNode to avoid uninitialized reads downstream.
🏁 Script executed:
#!/bin/bash
# Find struct definitions for OraParamRef, DoStmt, CallStmt
echo "=== Searching for OraParamRef definition ==="
rg -n "struct\s+OraParamRef|typedef.*OraParamRef" --type=c --type=h
echo -e "\n=== Searching for DoStmt definition ==="
rg -n "struct\s+DoStmt|typedef.*DoStmt" --type=c --type=h
echo -e "\n=== Searching for CallStmt definition ==="
rg -n "struct\s+CallStmt|typedef.*CallStmt" --type=c --type=hLength of output: 308
🏁 Script executed:
#!/bin/bash
# Search for downstream usage of new fields: paramslen, do_from_call, hostvariable, callinto
echo "=== Usage of DoStmt.paramslen ==="
rg -n "paramslen" --type=c --type=h
echo -e "\n=== Usage of DoStmt.do_from_call ==="
rg -n "do_from_call" --type=c --type=h
echo -e "\n=== Usage of CallStmt.hostvariable ==="
rg -n "hostvariable" --type=c --type=h
echo -e "\n=== Usage of CallStmt.callinto ==="
rg -n "callinto" --type=c --type=hLength of output: 3991
🏁 Script executed:
#!/bin/bash
# Check grammar file for %expect directive and conflict count
echo "=== Checking ora_gram.y for %expect and conflict status ==="
rg -n "%expect" src/backend/oracle_parser/ora_gram.y
echo -e "\n=== Looking for grammar test/build output if available ==="
find . -name "*.output" -o -name "*gram.output" | head -5Length of output: 133
🏁 Script executed:
#!/bin/bash
# Verify location validity patterns and scanbuf usage elsewhere
echo "=== Existing scanbuf slicing patterns in ora_gram.y ==="
rg -n "scanbuf.*@" src/backend/oracle_parser/ora_gram.y -A 2 -B 2
echo -e "\n=== Looking for location validation patterns ==="
rg -n "location.*>=.*0|@.*>=.*0|-1.*location" src/backend/oracle_parser/ --type=c --type=hLength of output: 828
🏁 Script executed:
#!/bin/bash
# Examine the actual ora_gram.y file around the target lines
echo "=== Content around lines 1260-1274 ==="
sed -n '1250,1280p' src/backend/oracle_parser/ora_gram.y
echo -e "\n=== Looking for OraParamRef initialization patterns ==="
rg -n "OraParamRef.*makeNode|makeNode.*OraParamRef" src/backend/oracle_parser/ --type=c --type=h -A 3Length of output: 986
🏁 Script executed:
#!/bin/bash
# Examine struct definitions to verify field completeness
echo "=== OraParamRef struct definition ==="
sed -n '332,360p' src/include/nodes/parsenodes.h
echo -e "\n=== DoStmt struct definition ==="
sed -n '3609,3640p' src/include/nodes/parsenodes.h
echo -e "\n=== CallStmt struct definition ==="
sed -n '3642,3660p' src/include/nodes/parsenodes.hLength of output: 2917
🏁 Script executed:
#!/bin/bash
# Check how makeNode is defined and whether it zeroes memory
echo "=== makeNode macro definition ==="
rg -n "define.*makeNode|#define.*makeNode" --type=c --type=h -A 5
echo -e "\n=== palloc0 usage (zero allocation) ==="
rg -n "palloc0.*Node|makeNode.*palloc" --type=c --type=h | head -20Length of output: 1619
🏁 Script executed:
#!/bin/bash
# Compare other scanbuf slicing for consistency
echo "=== Context around line 12521 (stmt_literal pattern 1) ==="
sed -n '12515,12530p' src/backend/oracle_parser/ora_gram.y
echo -e "\n=== Context around line 12545 (stmt_literal pattern 2) ==="
sed -n '12540,12555p' src/backend/oracle_parser/ora_gram.yLength of output: 1309
🏁 Script executed:
#!/bin/bash
# Search for any bounds validation on parser locations
echo "=== Any @N < 0 or location < 0 checks in ora_gram.y ==="
rg -n "@.*<.*0|location.*<.*0" src/backend/oracle_parser/ora_gram.y
echo -e "\n=== Any error handling on location/-1 in backend ==="
rg -n "location.*==.*-1|yylloc.*invalid|-1.*location" src/backend/ --type=c --type=h -C 2 | head -40Length of output: 853
🏁 Script executed:
#!/bin/bash
# Verify param_mode and params_length_list handling
echo "=== param_mode rule in ora_gram.y ==="
rg -n "param_mode\s*:" src/backend/oracle_parser/ora_gram.y -A 10 | head -30
echo -e "\n=== params_length_list rule in ora_gram.y ==="
rg -n "params_length_list\s*:" src/backend/oracle_parser/ora_gram.y -A 10 | head -30Length of output: 773
Fix scanbuf slicing bounds checking; verify grammar conflicts remain at 0; OraParamRef field initialization is complete but fragile
Bounds on @2 and @3 are not validated before slicing. Parser locations can be -1 (indicating unknown position), which would cause pnstrdup(base + (-1), len) to underflow. The same unguarded pattern appears at lines 12521 and 12545 for stmt_literal. Add validation:
- callstr = pnstrdup(pg_yyget_extra(yyscanner)->core_yy_extra.scanbuf + @2, @3 - @2);
+ if (@2 >= 0 && @3 > @2) {
+ const char *base = pg_yyget_extra(yyscanner)->core_yy_extra.scanbuf;
+ int len = @3 - @2;
+ while (len > 0 && isspace((unsigned char)base[@2 + len - 1])) len--;
+ callstr = pnstrdup(base + @2, len);
+ } else {
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("cannot construct CALL INTO assignment"),
+ parser_errposition(@3)));
+ }Apply the same fix to lines 12521 and 12545. OraParamRef initialization (lines 1263–1268) is complete for all struct fields but the pattern is fragile; consider initializing all fields explicitly in future. Verify %expect 0 still holds after grammar changes via build output.
📝 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.
| | CALL func_application INTO ORAPARAM | |
| { | |
| CallStmt *n = makeNode(CallStmt); | |
| OraParamRef *hostvar = makeNode(OraParamRef); | |
| char *callstr = NULL; | |
| n->funccall = castNode(FuncCall, $2); | |
| hostvar->number = 0; | |
| hostvar->location = @4; | |
| hostvar->name = $4; | |
| n->hostvariable = hostvar; | |
| callstr = pnstrdup(pg_yyget_extra(yyscanner)->core_yy_extra.scanbuf + @2, @3 - @2); | |
| n->callinto = psprintf("%s := %s;", $4, callstr); | |
| pfree(callstr); | |
| $$ = (Node *)n; | |
| } | |
| | CALL func_application INTO ORAPARAM | |
| { | |
| CallStmt *n = makeNode(CallStmt); | |
| OraParamRef *hostvar = makeNode(OraParamRef); | |
| char *callstr = NULL; | |
| n->funccall = castNode(FuncCall, $2); | |
| hostvar->number = 0; | |
| hostvar->location = @4; | |
| hostvar->name = $4; | |
| n->hostvariable = hostvar; | |
| if (@2 >= 0 && @3 > @2) { | |
| const char *base = pg_yyget_extra(yyscanner)->core_yy_extra.scanbuf; | |
| int len = @3 - @2; | |
| while (len > 0 && isspace((unsigned char)base[@2 + len - 1])) len--; | |
| callstr = pnstrdup(base + @2, len); | |
| } else { | |
| ereport(ERROR, | |
| (errcode(ERRCODE_SYNTAX_ERROR), | |
| errmsg("cannot construct CALL INTO assignment"), | |
| parser_errposition(@3))); | |
| } | |
| n->callinto = psprintf("%s := %s;", $4, callstr); | |
| pfree(callstr); | |
| $$ = (Node *)n; | |
| } |
🤖 Prompt for AI Agents
In src/backend/oracle_parser/ora_gram.y around lines 1260 to 1274, the code uses
@2 and @3 to slice the scanner buffer without validating their values which can
be -1 and cause pointer underflow; change the logic to check that @2 and @3 are
non-negative and that @3 > @2 before computing the slice (fall back to an empty
string or a safe default when positions are invalid), and free/assign
accordingly; apply the identical bounds checks and safe-fallback behavior to the
same pnstrdup usages at lines ~12521 and ~12545 (stmt_literal locations);
additionally keep the OraParamRef initialization as-is now but consider
switching to explicit zero-initialization of all fields in future; after making
these edits rebuild and confirm the parser reports “%expect 0” (no conflicts).
| if (nodeTag(parsetree) == T_RawStmt && | ||
| nodeTag(((RawStmt *)parsetree)->stmt) == T_CallStmt && | ||
| CurrentOraParamNode && | ||
| CurrentOraParamNode->param_count == 0) | ||
| setdynamic_callparser(true); | ||
| else | ||
| /* In any case, it should't affect other statements in the list */ | ||
| setdynamic_callparser(false); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid global parser state for CallStmt detection; scope per-walk.
IsDynCallStmt is toggled globally per top-level node. This can leak across nested parses or subsequent items. Prefer computing a local “isCallCtx” based on the node being walked and pass it via walker state, or push it on the existing OraParam stack.
Minimal fix: limit the scope by setting/restoring the previous value within this loop.
- if (nodeTag(parsetree) == T_RawStmt &&
+ bool prev = IsDynCallStmt;
+ if (nodeTag(parsetree) == T_RawStmt &&
nodeTag(((RawStmt *)parsetree)->stmt) == T_CallStmt &&
CurrentOraParamNode &&
CurrentOraParamNode->param_count == 0)
setdynamic_callparser(true);
else
/* In any case, it should't affect other statements in the list */
setdynamic_callparser(false);
+ (void)0; /* walk */
if (calculate_oraparamnumbers_walker(parsetree, NULL) == true)
return true;
+ /* restore */
+ setdynamic_callparser(prev);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/backend/parser/parse_param.c around lines 700 to 708, the code toggles
the global IsDynCallStmt via setdynamic_callparser based solely on the current
top-level node, which can leak state across nested walks or subsequent list
items; modify it to save the current IsDynCallStmt, set it to the new value for
the duration of processing this node, then restore the saved value immediately
after processing (or use the existing OraParam stack/walker state to carry an
isCallCtx flag) so the dynamic call parser flag is confined to this node's scope
and cannot affect other nodes.
| if (parsetree->stmt->type == T_CallStmt && | ||
| ((CallStmt *) parsetree->stmt)->callinto != NULL) | ||
| ereport(ERROR, | ||
| (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), | ||
| errmsg("Simple query protocol does not support CALL INTO stmt"))); | ||
|
|
There was a problem hiding this comment.
Use FEATURE_NOT_SUPPORTED for simple-protocol CALL INTO and clarify message
ERRCODE_PROGRAM_LIMIT_EXCEEDED is misleading here. Prefer ERRCODE_FEATURE_NOT_SUPPORTED and pluralize “statements”.
- ereport(ERROR,
- (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
- errmsg("Simple query protocol does not support CALL INTO stmt")));
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("simple query protocol does not support CALL INTO statements")));📝 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.
| if (parsetree->stmt->type == T_CallStmt && | |
| ((CallStmt *) parsetree->stmt)->callinto != NULL) | |
| ereport(ERROR, | |
| (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), | |
| errmsg("Simple query protocol does not support CALL INTO stmt"))); | |
| if (parsetree->stmt->type == T_CallStmt && | |
| ((CallStmt *) parsetree->stmt)->callinto != NULL) | |
| ereport(ERROR, | |
| (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), | |
| errmsg("simple query protocol does not support CALL INTO statements"))); |
🤖 Prompt for AI Agents
In src/backend/tcop/postgres.c around lines 1163 to 1168, the error raised for a
CALL INTO detected in the simple query protocol uses
ERRCODE_PROGRAM_LIMIT_EXCEEDED and a singular "stmt"; change the SQLSTATE to
ERRCODE_FEATURE_NOT_SUPPORTED and update the errmsg to pluralize "statements"
(e.g. "Simple query protocol does not support CALL INTO statements") so the
error code accurately reflects feature support and the message reads correctly.
| /* Validate parameter length list length matches parameter count */ | ||
| if (dostmt_plength != NIL && list_length(dostmt_plength) != numParams) | ||
| elog(ERROR, "the specified parameter length list does not match the number of parameters"); | ||
|
|
There was a problem hiding this comment.
Consistency: use ereport() with SQLSTATE for length-list mismatch
elog(ERROR, "...") lacks SQLSTATE. Use ereport(ERROR, (errcode + errmsg)).
- elog(ERROR, "the specified parameter length list does not match the number of parameters");
+ ereport(ERROR,
+ (errcode(ERRCODE_PROTOCOL_VIOLATION),
+ errmsg("parameter length list does not match number of parameters")));📝 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.
| /* Validate parameter length list length matches parameter count */ | |
| if (dostmt_plength != NIL && list_length(dostmt_plength) != numParams) | |
| elog(ERROR, "the specified parameter length list does not match the number of parameters"); | |
| /* Validate parameter length list length matches parameter count */ | |
| if (dostmt_plength != NIL && list_length(dostmt_plength) != numParams) | |
| ereport(ERROR, | |
| (errcode(ERRCODE_PROTOCOL_VIOLATION), | |
| errmsg("parameter length list does not match number of parameters"))); |
🤖 Prompt for AI Agents
In src/backend/tcop/postgres.c around lines 1930 to 1933, the code uses
elog(ERROR, "...") for a parameter length-list vs parameter count mismatch;
replace it with ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("the specified parameter length list does not match the number of
parameters"))); ensure you include the proper SQLSTATE macro
(ERRCODE_INVALID_PARAMETER_VALUE) and keep the original errmsg text so the error
is raised with both message and SQLSTATE.
| if (dostmt_plength != NIL) | ||
| { | ||
| Integer *v= NULL; | ||
|
|
||
| v = (Integer *) list_nth(dostmt_plength, paramno); | ||
|
|
||
| switch(ptype) | ||
| { | ||
| case ORACHARCHAROID: | ||
| case ORACHARBYTEOID: | ||
| case ORAVARCHARCHAROID: | ||
| case ORAVARCHARBYTEOID: | ||
| /* Hard-code typmod for Oracle char/varchar types to avoid ArrayType construction */ | ||
| params->params[paramno].ptypmod = intVal(v) + VARHDRSZ; | ||
| break; | ||
| default: | ||
| params->params[paramno].ptypmod = -1; | ||
| break; | ||
| } | ||
| } | ||
| else | ||
| params->params[paramno].ptypmod = -1; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Validate and clamp Oracle char/varchar length to avoid invalid typmods and overflow
intVal(v) can be negative or huge. Also, typmod = VARHDRSZ + n must fit int32 and respect Oracle-compatible maxima.
- /* Hard-code typmod for Oracle char/varchar types to avoid ArrayType construction */
- params->params[paramno].ptypmod = intVal(v) + VARHDRSZ;
+ /* Compute typmod for Oracle char/varchar types with bounds checking */
+ {
+ int32 n = intVal(v);
+ /* reject negative lengths and enforce reasonable upper bounds */
+ if (n < 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("negative length for parameter %d is not allowed", paramno + 1)));
+ /* Use Oracle-compatible maxima; adjust symbols to your backend header if different */
+ /* e.g., MaxOraCharLen/MaxOraVarcharLen or a single cap to prevent overflow */
+ if ((ptype == ORACHARCHAROID || ptype == ORACHARBYTEOID) && n > MaxOraCharLen)
+ ereport(ERROR,
+ (errcode(ERRCODE_STRING_DATA_RIGHT_TRUNCATION),
+ errmsg("CHAR length %d exceeds maximum %d", n, MaxOraCharLen)));
+ if ((ptype == ORAVARCHARCHAROID || ptype == ORAVARCHARBYTEOID) && n > MaxOraVarcharLen)
+ ereport(ERROR,
+ (errcode(ERRCODE_STRING_DATA_RIGHT_TRUNCATION),
+ errmsg("VARCHAR2 length %d exceeds maximum %d", n, MaxOraVarcharLen)));
+ /* safe add */
+ if (n > (INT_MAX - VARHDRSZ))
+ ereport(ERROR,
+ (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("length %d too large", n)));
+ params->params[paramno].ptypmod = n + VARHDRSZ;
+ }If MaxOraCharLen/MaxOraVarcharLen are not available in this TU, include the appropriate header or substitute with backend caps.
I'll verify the review comment by examining the codebase for the relevant code, constants, and validation patterns.
---
🏁 Script executed:
```shell
#!/bin/bash
# Check if the file exists and examine the target lines
if [ -f "src/backend/tcop/postgres.c" ]; then
echo "=== File found. Examining lines 2140-2180 ==="
sed -n '2140,2180p' src/backend/tcop/postgres.c
else
echo "File not found at src/backend/tcop/postgres.c"
find . -name "postgres.c" -type f | head -5
fi
Length of output: 1256
🏁 Script executed:
#!/bin/bash
# Search for Oracle char/varchar max length constants
echo "=== Searching for MaxOraCharLen, MaxOraVarcharLen, or similar ==="
rg -n "MaxOra(Char|Varchar)" --type=c --type=h
echo ""
echo "=== Searching for ORACHARCHAROID and related type definitions ==="
rg -n "ORACHARCHAROID|ORAVARCHARCHAROID" --type=c --type=h | head -20Length of output: 2195
🏁 Script executed:
#!/bin/bash
# Search for existing validation patterns and ereport usage in postgres.c
echo "=== Checking for ereport patterns with ERRCODE_INVALID_PARAMETER_VALUE ==="
rg -n "ERRCODE_INVALID_PARAMETER_VALUE" src/backend/tcop/postgres.c | head -10
echo ""
echo "=== Checking for validation of parameter lengths in this file ==="
rg -nC3 "intVal.*length|negative.*length" src/backend/tcop/postgres.cLength of output: 248
🏁 Script executed:
#!/bin/bash
# Check what headers are included in postgres.c
echo "=== Include statements in postgres.c (first 100 lines) ==="
head -100 src/backend/tcop/postgres.c | rg -n "#include"Length of output: 1944
---
🏁 Script executed:
```shell
#!/bin/bash
# Check if MaxOraCharLen/MaxOraVarcharLen are in ora_compatible.h
echo "=== Checking utils/ora_compatible.h ==="
if [ -f "src/backend/utils/ora_compatible.h" ]; then
rg -n "MaxOra(Char|Varchar)" src/backend/utils/ora_compatible.h
echo ""
echo "=== Full content of ora_compatible.h ==="
cat src/backend/utils/ora_compatible.h
else
echo "File not found"
fi
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Search all backend headers for Oracle char/varchar max length constants
echo "=== Searching backend headers for Oracle max length constants ==="
find src/backend -name "*.h" -exec grep -l "MaxOra.*Len\|MAXORA.*LEN" {} \;Length of output: 65
🏁 Script executed:
#!/bin/bash
# Check intVal() macro definition and understand its range
echo "=== Searching for intVal macro definition ==="
rg -n "define intVal\(|^intVal" --type=c --type=h | head -5
echo ""
echo "=== Checking Integer node structure and intVal context ==="
rg -nC5 "intVal.*v\)|intVal.*list_nth" src/backend/tcop/postgres.c | head -30Length of output: 590
🏁 Script executed:
#!/bin/bash
# Verify that VARHDRSZ is a defined constant
echo "=== Checking VARHDRSZ definition ==="
rg -n "define VARHDRSZ" --type=c --type=hLength of output: 366
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for where Oracle varchar/char size limits are used in backend code
echo "=== Searching for Oracle char/varchar size references in backend ==="
rg -n "2000|32767" src/backend --type=c --type=h | grep -i "oracle\|char\|varchar" | head -20
echo ""
echo "=== Searching for other typmod bounds checking patterns ==="
rg -nC3 "ptypmod.*overflow|typmod.*INT_MAX|typmod.*bounds" src/backend --type=c | head -40
echo ""
echo "=== Looking for typmod calculation patterns with validation ==="
rg -nC3 "typmod.*=.*\+.*VARHDRSZ|typmod.*=.*\+" src/backend/tcop/postgres.c | head -50
Length of output: 611
🏁 Script executed:
#!/bin/bash
# Check if there's any backend definition we missed
echo "=== Checking catalog headers for Oracle type OIDs and limits ==="
rg -n "ORACHAR|ORAVARCHAR" src/include/catalog/ --type=h
echo ""
echo "=== Checking if limits are defined anywhere with different names ==="
rg -n "2000|32767" src/include --type=h | grep -v "Binary\|psql" | head -20Length of output: 1886
🏁 Script executed:
#!/bin/bash
# Verify the actual issue by checking what intVal can legitimately return
echo "=== Checking Integer node definition ==="
rg -nA10 "typedef struct Integer" src/include --type=hLength of output: 1406
---
🏁 Script executed:
```shell
#!/bin/bash
# Check the full ora_compatible.h file - it exists!
echo "=== Contents of src/include/utils/ora_compatible.h ==="
cat src/include/utils/ora_compatible.h
Length of output: 1593
🏁 Script executed:
#!/bin/bash
# Search for VARCHAR max length constant
echo "=== Searching for VARCHAR/varchar max constants in backend ==="
rg -n "VARCHAR.*LENGTH|VARCHAR.*MAX|VARCHARLEN" src/include --type=h
echo ""
echo "=== Check what's around line 35 in ora_compatible.h context ==="
rg -nC10 "CHAR_TYPE_LENGTH_MAX" src/include/utils/ora_compatible.hLength of output: 554
🏁 Script executed:
#!/bin/bash
# Verify whether intVal returned values can legitimately be negative or huge
echo "=== Checking for any documentation or type enforcement on parameter length ==="
rg -nB5 "dostmt_plength\s*=" src/backend/tcop/postgres.c | head -40Length of output: 495
Validate bounds and clamp Oracle char/varchar typmod with proper constant definitions
The concern about unchecked intVal(v) is valid—it can be negative or arbitrarily large, risking overflow when computing intVal(v) + VARHDRSZ. However, the proposed diff references undefined constants.
Backend defines only CHAR_TYPE_LENGTH_MAX (2000) in src/include/utils/ora_compatible.h. No MaxOraCharLen or MaxOraVarcharLen exist in backend headers—they are frontend-only symbols. A corrected fix must either:
- Define
VARCHAR2_TYPE_LENGTH_MAXinora_compatible.h(to match Oracle's 32767 limit) - Reference the actual constant symbol names, or
- Use a single cap for both types
Additionally, check that dostmt_plength values cannot be negative before extraction (line 1900 in postgres.c assigns from parser-supplied DoStmt).
| res = IvyexecPreparedStatement2(tconn, stmt, 5, param_values, param_lengths, | ||
| param_formats, argmodes, | ||
| bindinfos, 1, errmsg, 256); | ||
|
|
||
| Ivyclear(res); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Assert execution succeeded before clearing result
Check IvyresultStatus and print errmsg to aid diagnosis; otherwise failures go unnoticed.
res = IvyexecPreparedStatement2(tconn, stmt, 5, param_values, param_lengths,
param_formats, argmodes,
bindinfos, 1, errmsg, 256);
-
- Ivyclear(res);
+ if (!res || (IvyresultStatus(res) != PGRES_TUPLES_OK &&
+ IvyresultStatus(res) != PGRES_COMMAND_OK &&
+ IvyresultStatus(res) != PGRES_EMPTY_QUERY)) {
+ fprintf(stderr, "IvyexecPreparedStatement2 failed: %s\n", errmsg);
+ if (res) Ivyclear(res);
+ exit_nicely(tconn);
+ }
+ Ivyclear(res);📝 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.
| res = IvyexecPreparedStatement2(tconn, stmt, 5, param_values, param_lengths, | |
| param_formats, argmodes, | |
| bindinfos, 1, errmsg, 256); | |
| Ivyclear(res); | |
| return; | |
| } | |
| res = IvyexecPreparedStatement2(tconn, stmt, 5, param_values, param_lengths, | |
| param_formats, argmodes, | |
| bindinfos, 1, errmsg, 256); | |
| if (!res || (IvyresultStatus(res) != PGRES_TUPLES_OK && | |
| IvyresultStatus(res) != PGRES_COMMAND_OK && | |
| IvyresultStatus(res) != PGRES_EMPTY_QUERY)) { | |
| fprintf(stderr, "IvyexecPreparedStatement2 failed: %s\n", errmsg); | |
| if (res) Ivyclear(res); | |
| exit_nicely(tconn); | |
| } | |
| Ivyclear(res); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/interfaces/libpq/ivytest/testlibpqstmt2_call.c around lines 208-214, the
code clears the Ivyresult immediately after IvyexecPreparedStatement2; instead,
check the result status with IvyresultStatus(res) and if it indicates failure
print the errmsg buffer (or use IvyresultErrorMessage if available) to stderr
(or processLogger), then handle the error (return or exit non-zero) before
calling Ivyclear(res); only Ivyclear when done with a successful or handled
result.
| typedef struct HostVariableEntry | ||
| { | ||
| char *name; | ||
| int position; | ||
| } HostVariableEntry; | ||
|
|
||
| typedef struct HostVariable | ||
| { | ||
| HostVariableEntry *hostvars; | ||
| int length; | ||
| bool isdostmt; | ||
| bool iscallstmt; | ||
| char *convertcall; | ||
| } HostVariable; | ||
|
|
There was a problem hiding this comment.
Public header uses bool without including <stdbool.h>
This will break C builds depending on include order. Include stdbool.h in this header.
#include "postgres_ext.h"
+#include <stdbool.h>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/interfaces/libpq/libpq-ivy.h around lines 149 to 163, the public header
uses the bool type without including <stdbool.h>, which can break builds
depending on include order; add an #include <stdbool.h> at the top of this
header (or before any bool usage) so bool is properly defined for all
translation units that include this header.
| /* | ||
| * I'm not sure if this is a bug, so I'm only temporarily | ||
| * fixing it for scenarios that I care about. | ||
| */ | ||
| if (estate->func && estate->func->do_from_call && | ||
| strcmp(estate->func->fn_signature, "inline_code_block") == 0) | ||
| n = ParseParamVariable(n); | ||
|
|
There was a problem hiding this comment.
Remove “temporary fix” comment; use a robust condition for DO-from-CALL.
The “I’m not sure...” comment cannot ship. Also, avoid magic string "inline_code_block"; prefer fn_prokind check. Keep ParseParamVariable to unwrap wrappers.
Apply:
- /*
- * I'm not sure if this is a bug, so I'm only temporarily
- * fixing it for scenarios that I care about.
- */
- if (estate->func && estate->func->do_from_call &&
- strcmp(estate->func->fn_signature, "inline_code_block") == 0)
+ /* Unwrap Param wrappers when executing a DO block originated from CALL INTO. */
+ if (estate->func && estate->func->do_from_call &&
+ estate->func->fn_prokind == PROKIND_ANONYMOUS_BLOCK)
n = ParseParamVariable(n);🤖 Prompt for AI Agents
In src/pl/plisql/src/pl_exec.c around lines 2517-2524, remove the temporary "I'm
not sure..." comment and replace the magic-string check
strcmp(estate->func->fn_signature, "inline_code_block") with a robust check
using the function kind: test estate->func->do_from_call &&
estate->func->fn_prokind == PROKIND_PROCEDURE (so we detect DO-from-CALL by
fn_prokind rather than fn_signature) and keep the existing call to n =
ParseParamVariable(n); adjust includes if necessary so PROKIND_PROCEDURE is
defined.
| if (codeblock->do_from_call && oldcontext != ErrorContext) | ||
| { | ||
| ErrorData *edata; | ||
|
|
||
| /* Save error info */ | ||
| MemoryContextSwitchTo(oldcontext); | ||
| edata = CopyErrorData(); | ||
| FlushErrorState(); | ||
|
|
||
| /* For CALL statements expect only primary error message */ | ||
| if (edata->message) | ||
| elog(ERROR, "%s", edata->message); | ||
| else | ||
| ThrowErrorData(edata); | ||
| FreeErrorData(edata); | ||
| } | ||
|
|
There was a problem hiding this comment.
Early rethrow skips cleanup and loses SQLSTATE; rework error path
Inside PG_CATCH, rethrowing immediately when do_from_call is true bypasses the subsequent cleanup (oraparam stack pop, EState/ResourceOwner release, func memory free). Also, elog(ERROR, "%s", ...) discards the original SQLSTATE and error metadata. Capture ErrorData, perform the usual cleanup, then rethrow using ThrowErrorData with only the primary message to preserve SQLSTATE.
Apply this diff to defer the rethrow and keep SQLSTATE:
@@
- PG_CATCH();
- {
- if (codeblock->do_from_call && oldcontext != ErrorContext)
- {
- ErrorData *edata;
-
- /* Save error info */
- MemoryContextSwitchTo(oldcontext);
- edata = CopyErrorData();
- FlushErrorState();
-
- /* For CALL statements expect only primary error message */
- if (edata->message)
- elog(ERROR, "%s", edata->message);
- else
- ThrowErrorData(edata);
- FreeErrorData(edata);
- }
+ /* Stash minimal error for CALL, then cleanup, then rethrow */
+ ErrorData *saved_edata = NULL;
+ PG_CATCH();
+ {
+ if (codeblock->do_from_call && oldcontext != ErrorContext)
+ {
+ MemoryContextSwitchTo(oldcontext);
+ saved_edata = CopyErrorData();
+ /* Drop current error so we can run cleanup safely */
+ FlushErrorState();
+ /* Keep only primary message; preserve SQLSTATE via ThrowErrorData */
+ if (saved_edata)
+ {
+ saved_edata->detail = NULL;
+ saved_edata->detail_log = NULL;
+ saved_edata->hint = NULL;
+ saved_edata->context = NULL;
+ saved_edata->backtrace = NULL;
+ }
+ }
@@
- /* And propagate the error */
- PG_RE_THROW();
+ /* And propagate the error */
+ if (saved_edata)
+ ThrowErrorData(saved_edata); /* does not return */
+ PG_RE_THROW();
}This preserves resource cleanup and the original SQLSTATE while emitting only the primary message for CALL.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/pl/plisql/src/pl_handler.c around lines 498-514, the PG_CATCH branch
currently elog()s and returns early when codeblock->do_from_call is true which
skips later cleanup and loses SQLSTATE; instead, capture the ErrorData via
CopyErrorData(), FlushErrorState(), but do not call elog() or ThrowErrorData()
there; store the captured ErrorData and a flag indicating a deferred rethrow,
proceed to run the normal cleanup (oraparam stack pop, EState/ResourceOwner
release, func memory free) and only after those cleanups call ThrowErrorData()
to rethrow the error while preserving SQLSTATE, making sure the rethrown
ErrorData contains only the primary message (modify edata->message to the
primary message if needed) and do not free the ErrorData before calling
ThrowErrorData().
| bool fn_no_return; /* when the function return type is not VOIDOID, if the body has not a RETURN statment, | ||
| * the CREATE FUNCTION can be executed successfully, but when the function is called, | ||
| * an error will be reported */ | ||
| bool do_from_call; | ||
| char **paramnames; /* saved do + using parameter'name */ | ||
| } PLiSQL_function; |
There was a problem hiding this comment.
Possible dangling pointer: copy ParamListInfo.paramnames into function context.
PLiSQL_function.paramnames is assigned from inparams->paramnames in plisql_compile_inline(), but not copied. If ParamListInfo is freed or reused after compilation, this pointer may dangle.
Deep‑copy paramnames into function->fn_cxt and track count with fn_nargs to avoid UAF.
Apply this change in src/pl/plisql/src/pl_comp.c:
@@
- function->fn_nargs = inparams->numParams;
- function->paramnames = inparams->paramnames;
+ function->fn_nargs = inparams->numParams;
+ if (inparams->paramnames && function->fn_nargs > 0)
+ {
+ Size sz = sizeof(char*) * function->fn_nargs;
+ function->paramnames = (char **) MemoryContextAlloc(function->fn_cxt, sz);
+ for (i = 0; i < function->fn_nargs; i++)
+ function->paramnames[i] = inparams->paramnames[i]
+ ? MemoryContextStrdup(function->fn_cxt, inparams->paramnames[i])
+ : NULL;
+ }
+ else
+ function->paramnames = NULL;Committable suggestion skipped: line range outside the PR's diff.
|
no comments. |
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Tests