pgsql: Coerce 'unknown' type parameters to the right type in the

Started by Nonameover 15 years ago8 messages
#1Noname
heikki@postgresql.org

Log Message:
-----------
Coerce 'unknown' type parameters to the right type in the fixed-params
parse_analyze() function. That case occurs e.g with PL/pgSQL
EXECUTE ... USING 'stringconstant'.

The coercion with a CoerceViaIO node. The result is similar to the coercion
via input function performed for unknown constants in coerce_type(),
except that this happens at runtime.

Backpatch to 9.0. The issue is present in 8.4 as well, but the coerce param
hook infrastructure this patch relies on was introduced in 9.0. Given the
lack of user reports and harmlessness of the bug, it's not worth attempting
a different fix just for 8.4.

Modified Files:
--------------
pgsql/src/backend/parser:
parse_param.c (r2.4 -> r2.5)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/parser/parse_param.c?r1=2.4&r2=2.5)

#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Noname (#1)
Re: pgsql: Coerce 'unknown' type parameters to the right type in the

heikki@postgresql.org (Heikki Linnakangas) writes:

Log Message:
-----------
Coerce 'unknown' type parameters to the right type in the fixed-params
parse_analyze() function. That case occurs e.g with PL/pgSQL
EXECUTE ... USING 'stringconstant'.

The coercion with a CoerceViaIO node. The result is similar to the coercion
via input function performed for unknown constants in coerce_type(),
except that this happens at runtime.

Unfortunately, this entirely fails to enforce the rule that an unknown
Param be coerced the same way everywhere. You'd need a cleanup pass as
well, cf check_variable_parameters().

regards, tom lane

#3Heikki Linnakangas
heikki.linnakangas@enterprisedb.com
In reply to: Tom Lane (#2)
Re: pgsql: Coerce 'unknown' type parameters to the right type in the

On 18/08/10 16:57, Tom Lane wrote:

heikki@postgresql.org (Heikki Linnakangas) writes:

Log Message:
-----------
Coerce 'unknown' type parameters to the right type in the fixed-params
parse_analyze() function. That case occurs e.g with PL/pgSQL
EXECUTE ... USING 'stringconstant'.

The coercion with a CoerceViaIO node. The result is similar to the coercion
via input function performed for unknown constants in coerce_type(),
except that this happens at runtime.

Unfortunately, this entirely fails to enforce the rule that an unknown
Param be coerced the same way everywhere. You'd need a cleanup pass as
well, cf check_variable_parameters().

Yeah, you're right. I'll find a way to do the cleanup pass in fixed
params case too.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

#4Heikki Linnakangas
heikki.linnakangas@enterprisedb.com
In reply to: Heikki Linnakangas (#3)
1 attachment(s)
Re: [COMMITTERS] pgsql: Coerce 'unknown' type parameters to the right type in the

On 18/08/10 18:03, Heikki Linnakangas wrote:

On 18/08/10 16:57, Tom Lane wrote:

heikki@postgresql.org (Heikki Linnakangas) writes:

Log Message:
-----------
Coerce 'unknown' type parameters to the right type in the fixed-params
parse_analyze() function. That case occurs e.g with PL/pgSQL
EXECUTE ... USING 'stringconstant'.

The coercion with a CoerceViaIO node. The result is similar to the
coercion
via input function performed for unknown constants in coerce_type(),
except that this happens at runtime.

Unfortunately, this entirely fails to enforce the rule that an unknown
Param be coerced the same way everywhere. You'd need a cleanup pass as
well, cf check_variable_parameters().

Yeah, you're right. I'll find a way to do the cleanup pass in fixed
params case too.

It turned out to be messier than I imagined, but I have a working patch
now. It still doesn't behave exactly like the variable params case,
though. To wit:

postgres=# DO $$
declare
t text;
begin
EXECUTE 'SELECT 1+ $1, $1' INTO t USING '123' ;
RAISE NOTICE '%', t;
end;
$$;
ERROR: could not determine data type of parameter $1
LINE 1: SELECT 1+ $1, $1
^
QUERY: SELECT 1+ $1, $1
CONTEXT: PL/pgSQL function "inline_code_block" line 5 at EXECUTE statement

The varparams code doesn't thrown an error on that. At the first
reference to $1, the parameter is resolved to int4. On the 2nd
reference, it's an int4 and there's nothing to force coercion to
anything else, so it's returned as an int4. In the fixed params case,
however, that throws an error. We could make it behave the same if we
really wanted to, but that would add even more code.

I'm starting to wonder if it's worth enforcing the rule that all unknown
Params must be coerced to the same target type. We could just document
the behavior. Or maybe we should revert the whole thing, and add a check
to PL/pgSQL EXECUTE USING code to just throw a nicer error message if
you pass an unknown parameter in the USING clause.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Attachments:

check-fixed-unknown-param-consistency.patchtext/x-diff; name=check-fixed-unknown-param-consistency.patchDownload
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 6b99a10..2cb9f31 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -92,6 +92,10 @@ parse_analyze(Node *parseTree, const char *sourceText,
 
 	query = transformStmt(pstate, parseTree);
 
+	/* make sure all is well with parameter types */
+	if (numParams > 0)
+		check_fixed_parameters(pstate, query);
+
 	free_parsestate(pstate);
 
 	return query;
diff --git a/src/backend/parser/parse_param.c b/src/backend/parser/parse_param.c
index 60917f4..7cb34c2 100644
--- a/src/backend/parser/parse_param.c
+++ b/src/backend/parser/parse_param.c
@@ -59,7 +59,10 @@ static Node *variable_coerce_param_hook(ParseState *pstate, Param *param,
 static Node *fixed_coerce_param_hook(ParseState *pstate, Param *param,
 						   Oid targetTypeId, int32 targetTypeMod,
 						   int location);
-static bool check_parameter_resolution_walker(Node *node, ParseState *pstate);
+static bool check_fixed_parameter_resolution_walker(Node *node,
+										ParseState *pstate);
+static bool check_variable_parameter_resolution_walker(Node *node,
+										   ParseState *pstate);
 
 
 /*
@@ -322,6 +325,135 @@ variable_coerce_param_hook(ParseState *pstate, Param *param,
 	return NULL;
 }
 
+
+/*
+ * Check for consistent assignment of unknown parameters after completion
+ * of parsing with parse_fixed_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.
+ */
+void
+check_fixed_parameters(ParseState *pstate, Query *query)
+{
+	FixedParamState *parstate = (FixedParamState *) pstate->p_ref_hook_state;
+
+	/*
+	 * If parse_fixed_parameters() didn't resolve any unknown types, there's
+	 * nothing to do.
+	 */
+	if (parstate->unknownParamTypes != NULL)
+		(void) query_tree_walker(query,
+								 check_fixed_parameter_resolution_walker,
+								 (void *) pstate, 0);
+}
+
+/*
+ * Traverse a fully-analyzed tree to verify that all references to unknown
+ * Params are coerced to the same type.  Although we check in
+ * fixed_coerce_param_hook() that an unknown Param is not coerced to different
+ * types at different locations in the query, some Params might still be
+ * uncoerced, if there wasn't anything to force their coercion, and yet other
+ * instances seen later might have gotten coerced.
+ */
+static bool
+check_fixed_parameter_resolution_walker(Node *node, ParseState *pstate)
+{
+	FixedParamState *parstate = (FixedParamState *) pstate->p_ref_hook_state;
+
+	if (node == NULL)
+		return false;
+
+	/*
+	 * Check if this is a CoerceViaIO(Param of type 'unknown') construct,
+	 * created by parse_fixed_parameters(). In theory, it could be a similar
+	 * construct created by other means, but that doesn't currently happen;
+	 * unknown Params needing coercion are always handled by
+	 * fixed_coerce_param_hook(), so the normal logic in coerce_type() which
+	 * could create CoerceViaIO nodes doesn't get to run on unknown Params.
+	 * This needs to be revisited if CoerceViaIO nodes are created elsewhere
+	 * in the future.
+	 */
+	if (IsA(node, CoerceViaIO))
+	{
+		CoerceViaIO *cvio = (CoerceViaIO *) node;
+
+		if (IsA(cvio->arg, Param))
+		{
+			Param *param = (Param *) node;
+
+			if (param->paramkind == PARAM_EXTERN &&
+				param->paramtype == UNKNOWNOID)
+			{
+				int			paramno = param->paramid;
+
+				/*
+				 * This is an already coerced unknown param. Check that
+				 * the type matches what we recorded in unknownParamTypes.
+				 * It always should, this is just a sanity check. What's
+				 * important is to not recurse into the Param node itself.
+				 */
+
+				if (paramno <= 0 || /* shouldn't happen, but... */
+					paramno > parstate->numParams)
+					ereport(ERROR,
+							(errcode(ERRCODE_UNDEFINED_PARAMETER),
+							 errmsg("there is no parameter $%d", paramno),
+							 parser_errposition(pstate, param->location)));
+
+				if (cvio->resulttype != parstate->unknownParamTypes[paramno - 1])
+					ereport(ERROR,
+							(errcode(ERRCODE_AMBIGUOUS_PARAMETER),
+					 errmsg("could not determine data type of parameter $%d",
+									paramno),
+							 parser_errposition(pstate, param->location)));
+			}
+			return false;
+		}
+	}
+
+	/*
+	 * If this is a naked unknown Param, not coerced by parse_fixed_param(),
+	 * check that there was no instances of the same Param that was coerced
+	 * to some other type. Such coercions if they exist have set the type in
+	 * unknownParamTypes, so check that it's still invalid.
+	 */
+	if (IsA(node, Param))
+	{
+		Param	   *param = (Param *) node;
+
+		if (param->paramkind == PARAM_EXTERN && param->paramtype == UNKNOWNOID)
+		{
+			int			paramno = param->paramid;
+
+			if (paramno <= 0 || /* shouldn't happen, but... */
+				paramno > parstate->numParams)
+				ereport(ERROR,
+						(errcode(ERRCODE_UNDEFINED_PARAMETER),
+						 errmsg("there is no parameter $%d", paramno),
+						 parser_errposition(pstate, param->location)));
+
+			if (parstate->unknownParamTypes[paramno - 1] != InvalidOid)
+				ereport(ERROR,
+						(errcode(ERRCODE_AMBIGUOUS_PARAMETER),
+					 errmsg("could not determine data type of parameter $%d",
+							paramno),
+						 parser_errposition(pstate, param->location)));
+		}
+		return false;
+	}
+	if (IsA(node, Query))
+	{
+		/* Recurse into RTE subquery or not-yet-planned sublink subquery */
+		return query_tree_walker((Query *) node,
+								 check_fixed_parameter_resolution_walker,
+								 (void *) pstate, 0);
+	}
+	return expression_tree_walker(node, check_fixed_parameter_resolution_walker,
+								  (void *) pstate);
+}
+
 /*
  * Check for consistent assignment of variable parameters after completion
  * of parsing with parse_variable_parameters.
@@ -338,7 +470,7 @@ check_variable_parameters(ParseState *pstate, Query *query)
 	/* If numParams is zero then no Params were generated, so no work */
 	if (*parstate->numParams > 0)
 		(void) query_tree_walker(query,
-								 check_parameter_resolution_walker,
+								 check_variable_parameter_resolution_walker,
 								 (void *) pstate, 0);
 }
 
@@ -349,7 +481,7 @@ check_variable_parameters(ParseState *pstate, Query *query)
  * and yet other instances seen later might have gotten coerced.
  */
 static bool
-check_parameter_resolution_walker(Node *node, ParseState *pstate)
+check_variable_parameter_resolution_walker(Node *node, ParseState *pstate)
 {
 	if (node == NULL)
 		return false;
@@ -382,9 +514,10 @@ check_parameter_resolution_walker(Node *node, ParseState *pstate)
 	{
 		/* Recurse into RTE subquery or not-yet-planned sublink subquery */
 		return query_tree_walker((Query *) node,
-								 check_parameter_resolution_walker,
+								 check_variable_parameter_resolution_walker,
 								 (void *) pstate, 0);
 	}
-	return expression_tree_walker(node, check_parameter_resolution_walker,
+	return expression_tree_walker(node,
+								  check_variable_parameter_resolution_walker,
 								  (void *) pstate);
 }
diff --git a/src/include/parser/parse_param.h b/src/include/parser/parse_param.h
index 25037d8..dcd26b9 100644
--- a/src/include/parser/parse_param.h
+++ b/src/include/parser/parse_param.h
@@ -19,6 +19,7 @@ extern void parse_fixed_parameters(ParseState *pstate,
 					   Oid *paramTypes, int numParams);
 extern void parse_variable_parameters(ParseState *pstate,
 						  Oid **paramTypes, int *numParams);
+extern void check_fixed_parameters(ParseState *pstate, Query *query);
 extern void check_variable_parameters(ParseState *pstate, Query *query);
 
 #endif   /* PARSE_PARAM_H */
#5Tom Lane
tgl@sss.pgh.pa.us
In reply to: Heikki Linnakangas (#4)
Re: Re: [COMMITTERS] pgsql: Coerce 'unknown' type parameters to the right type in the

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

I'm starting to wonder if it's worth enforcing the rule that all unknown
Params must be coerced to the same target type. We could just document
the behavior. Or maybe we should revert the whole thing, and add a check
to PL/pgSQL EXECUTE USING code to just throw a nicer error message if
you pass an unknown parameter in the USING clause.

+1 for the latter. There's no good reason to be passing unknowns to USING.
I'm also getting more and more uncomfortable with the amount of new
behavior that's being pushed into an existing SPI call.

Another possibility is for EXECUTE USING to coerce any unknowns to TEXT
before it calls the parser at all. This would square with the typical
default assumption for unknown literals, and it would avoid having to
have any semantics changes below the SPI call.

regards, tom lane

#6Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#5)
Re: Re: [COMMITTERS] pgsql: Coerce 'unknown' type parameters to the right type in the

On Thu, Aug 19, 2010 at 9:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Another possibility is for EXECUTE USING to coerce any unknowns to TEXT
before it calls the parser at all.  This would square with the typical
default assumption for unknown literals, and it would avoid having to
have any semantics changes below the SPI call.

That seems more intuitive than just chucking an error.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

#7David E. Wheeler
david@kineticode.com
In reply to: Robert Haas (#6)
Re: Re: [COMMITTERS] pgsql: Coerce 'unknown' type parameters to the right type in the

On Aug 19, 2010, at 8:08 AM, Robert Haas wrote:

Another possibility is for EXECUTE USING to coerce any unknowns to TEXT
before it calls the parser at all. This would square with the typical
default assumption for unknown literals, and it would avoid having to
have any semantics changes below the SPI call.

That seems more intuitive than just chucking an error.

It'd be nice if SPI itself could work this way for UNKNOWNs, too.

Best,

David

#8Heikki Linnakangas
heikki.linnakangas@enterprisedb.com
In reply to: Robert Haas (#6)
Re: Re: [COMMITTERS] pgsql: Coerce 'unknown' type parameters to the right type in the

On 19/08/10 18:08, Robert Haas wrote:

On Thu, Aug 19, 2010 at 9:47 AM, Tom Lane<tgl@sss.pgh.pa.us> wrote:

Another possibility is for EXECUTE USING to coerce any unknowns to TEXT
before it calls the parser at all. This would square with the typical
default assumption for unknown literals, and it would avoid having to
have any semantics changes below the SPI call.

That seems more intuitive than just chucking an error.

Ok, I reverted the previous patch, and did that instead.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com