public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tom Lane <[email protected]>
To: David G. Johnston <[email protected]>
Cc: Martin Handsteiner <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: stringtype=unspecified is null check problem
Date: Wed, 11 Jan 2023 22:13:13 -0500
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAKFQuwYtmLLkmjaKghYXPnBWwvUvsqhZR=9J8KDcLzavf8707w@mail.gmail.com>
References: <VI1PR1001MB1423CDC3E737B4763436B730E8FF9@VI1PR1001MB1423.EURPRD10.PROD.OUTLOOK.COM>
<CAKFQuwZA+9+FNovGXA6moj+sDzR6ShyzzP3H77m-05n3dXDv-w@mail.gmail.com>
<VI1PR1001MB1423C002817FECB26B1AFB3DE8FC9@VI1PR1001MB1423.EURPRD10.PROD.OUTLOOK.COM>
<CAKFQuwZrn0F9RyM=WL9Z=pioGX5pwH1+mbBe-jGNnjy_J2E+Jw@mail.gmail.com>
<[email protected]>
<CAKFQuwYtmLLkmjaKghYXPnBWwvUvsqhZR=9J8KDcLzavf8707w@mail.gmail.com>
"David G. Johnston" <[email protected]> writes:
> Yes, the non-determinism of the above (i.e., reversing the order of the
> tests removes the error), which implies the error is not sufficiently
> delayed to give other parts of the statement a chance to provide context,
> is also annoying. Which I suppose is why you are saying a second pass
> would be needed to get that delay in a minimally-invasive way.
Actually ... looking closer at the relevant code, there already *is*
two-pass processing. We're just using it to throw an error though.
I wonder if we can get away with retroactively changing the types
of Params that didn't get resolved from local context, along the
lines of the attached. The main issues I can see with this are:
* Is there any case where we'd copy UNKNOWNOID as the type of a
parse node just above an unresolved Param? I can't think of a
reason to do that offhand, because any such context would force
identification of the Param's type; but maybe I'm missing something.
If that did happen, this code would not fix the type of the upper
parse node. But maybe that wouldn't matter anyway??
* There is a very gross hack "for JDBC compatibility" right
adjacent to this:
/*
* If the argument is of type void and it's procedure call, interpret it
* as unknown. This allows the JDBC driver to not have to distinguish
* function and procedure calls. See also another component of this hack
* in ParseFuncOrColumn().
*/
if (*pptype == VOIDOID && pstate->p_expr_kind == EXPR_KIND_CALL_ARGUMENT)
*pptype = UNKNOWNOID;
I'm not sure if this change breaks that, and have no idea how to test.
regards, tom lane
Attachments:
[text/x-diff] resolve-unknown-params-better-0.1.patch (4.4K, ../[email protected]/2-resolve-unknown-params-better-0.1.patch)
download | inline diff:
diff --git a/src/backend/parser/parse_param.c b/src/backend/parser/parse_param.c
index 2240284f21..cbd70e3315 100644
--- a/src/backend/parser/parse_param.c
+++ b/src/backend/parser/parse_param.c
@@ -49,6 +49,7 @@ typedef struct VarParamState
{
Oid **paramTypes; /* array of parameter type OIDs */
int *numParams; /* number of array entries */
+ bool haveUnresolved; /* have we made any Params of type UNKNOWN? */
} VarParamState;
static Node *fixed_paramref_hook(ParseState *pstate, ParamRef *pref);
@@ -87,6 +88,7 @@ setup_parse_variable_parameters(ParseState *pstate,
parstate->paramTypes = paramTypes;
parstate->numParams = numParams;
+ parstate->haveUnresolved = false;
pstate->p_ref_hook_state = (void *) parstate;
pstate->p_paramref_hook = variable_paramref_hook;
pstate->p_coerce_param_hook = variable_coerce_param_hook;
@@ -168,6 +170,10 @@ variable_paramref_hook(ParseState *pstate, ParamRef *pref)
if (*pptype == VOIDOID && pstate->p_expr_kind == EXPR_KIND_CALL_ARGUMENT)
*pptype = UNKNOWNOID;
+ /* If we've not yet resolved the type, remember we need to fix it later */
+ if (*pptype == UNKNOWNOID)
+ parstate->haveUnresolved = true;
+
param = makeNode(Param);
param->paramkind = PARAM_EXTERN;
param->paramid = paramno;
@@ -257,30 +263,31 @@ variable_coerce_param_hook(ParseState *pstate, Param *param,
}
/*
- * Check for consistent assignment of variable parameters after completion
+ * Check for complete assignment of variable parameters after completion
* of parsing with parse_variable_parameters.
*
* Note: this code intentionally does not check that all parameter positions
- * were used, nor that all got non-UNKNOWN types assigned. Caller of parser
- * should enforce that if it's important.
+ * were used. Caller of parser should enforce that if it's important.
*/
void
check_variable_parameters(ParseState *pstate, Query *query)
{
VarParamState *parstate = (VarParamState *) pstate->p_ref_hook_state;
- /* If numParams is zero then no Params were generated, so no work */
- if (*parstate->numParams > 0)
+ /* Needn't do anything unless we made some UNKNOWN Params */
+ if (parstate->haveUnresolved)
(void) query_tree_walker(query,
check_parameter_resolution_walker,
(void *) pstate, 0);
}
/*
- * Traverse a fully-analyzed tree to verify that parameter symbols
- * match their types. We need this because some Params might still
- * be UNKNOWN, if there wasn't anything to force their coercion,
- * and yet other instances seen later might have gotten coerced.
+ * Traverse a fully-analyzed tree to back-fill any Params whose types were not
+ * determined at creation or during local parse analysis. This would only be
+ * possible in contexts where we did not have any need to force immediate
+ * resolution of the Param's type, for example "SELECT ... WHERE $1 IS NULL".
+ * If we have no other clue about the type of such a Param, resolve it as
+ * text, following the precedent of unknown-type literals.
*/
static bool
check_parameter_resolution_walker(Node *node, ParseState *pstate)
@@ -295,6 +302,7 @@ check_parameter_resolution_walker(Node *node, ParseState *pstate)
{
VarParamState *parstate = (VarParamState *) pstate->p_ref_hook_state;
int paramno = param->paramid;
+ Oid resolvedType;
if (paramno <= 0 || /* shouldn't happen, but... */
paramno > *parstate->numParams)
@@ -303,7 +311,26 @@ check_parameter_resolution_walker(Node *node, ParseState *pstate)
errmsg("there is no parameter $%d", paramno),
parser_errposition(pstate, param->location)));
- if (param->paramtype != (*parstate->paramTypes)[paramno - 1])
+ resolvedType = (*parstate->paramTypes)[paramno - 1];
+
+ /* If no use of the Param identified a type, resolve it as TEXT */
+ if (resolvedType == UNKNOWNOID)
+ {
+ resolvedType = TEXTOID;
+ (*parstate->paramTypes)[paramno - 1] = resolvedType;
+ }
+
+ /* If this particular Param wasn't resolved previously, fix it */
+ if (param->paramtype == UNKNOWNOID)
+ {
+ param->paramtype = resolvedType;
+ /* Update typmod and collation the same as above */
+ param->paramtypmod = -1;
+ param->paramcollid = get_typcollation(resolvedType);
+ }
+
+ /* Cross-check, just for paranoia */
+ else if (param->paramtype != resolvedType)
ereport(ERROR,
(errcode(ERRCODE_AMBIGUOUS_PARAMETER),
errmsg("could not determine data type of parameter $%d",
view thread (14+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: stringtype=unspecified is null check problem
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox