Re: pg_stat_statements normalisation without invasive changes to the parser (was: Next steps on pg_stat_statements normalisation)

On 16 February 2012 21:11, Peter Geoghegan <peter@2ndquadrant.com> wrote:
> *       # XXX: This test currently fails!:
>        #verify_normalizes_correctly("SELECT cast('1' as dnotnull);","SELECT
> cast(? as dnotnull);",conn, "domain literal canonicalization/cast")
>
> It appears to fail because the CoerceToDomain node gives its location
> to the constant node as starting from "cast", so we end up with
> "SELECT ?('1' as dnotnull);". I'm not quite sure if this points to
> there being a slight tension with my use of the location field in this
> way, or if this is something that could be fixed as a bug in core
> (albeit a highly obscure one), though I suspect the latter.

So I looked at this in more detail today, and it turns out that it has
nothing to do with CoerceToDomain in particular. The same effect can
be observed by doing this:

select cast('foo' as text);

In turns out that this happens for the same reason as the location of
the Const token in the following query:

select integer 5;

being given such that the string "select ?" results.

Resolving this one issue resolves some others, as it allows me to
greatly simplify the get_constant_length() logic.

Here is the single, hacky change I've made just for now to the core
parser to quickly see if it all works as expected:

*************** transformTypeCast(ParseState *pstate, Ty
*** 2108,2113 ****
--- 2108,2116 ----
  	if (location < 0)
  		location = tc->typeName->location;

+ 	if (IsA(expr, Const))
+ 		location = ((Const*)expr)->location;
+
  	result = coerce_to_target_type(pstate, expr, inputType,
  								   targetType, targetTypmod,
  								   COERCION_EXPLICIT,

After making this change, I can get all my regression tests to pass
(once I change the normalised representation of certain queries to
look like: "select integer ?" rather than "select ?", which is better
anyway), including the CAST()/CoerceToDomain one that previously
failed. So far so good.

Clearly this change is a quick and dirty workaround, and something
better is required. The question I'd pose to the maintainer of this
code is: what business does the coerce_to_target_type call have
changing the location of the Const node resulting from coercion under
the circumstances described? I understand that the location of the
CoerceToDomain should be at "CAST", but why should the underlying
Const's position be the same? Do you agree that this is a bug, and if
so, would you please facilitate me by committing a fix?

-- 
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services