Re: stringtype=unspecified is null check problem - Mailing list pgsql-jdbc

"David G. Johnston" <david.g.johnston@gmail.com> 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

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",