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

On 21 February 2012 01:48, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> you're proposing to move the error pointer to the "42", and that does
> not seem like an improvement, especially not if it only happens when the
> cast subject is a simple constant rather than an expression.

2008's commit a2794623d292f7bbfe3134d1407281055acce584 [1] added the
following code to parse_coerce.c [2]:

/* Use the leftmost of the constant's and coercion's locations */
if (location < 0)
newcon->location = con->location;
else if (con->location >= 0 && con->location < location)
newcon->location = con->location;
else
newcon->location = location;

With that commit, Tom made a special case of both Const and Param
nodes, and had them take the leftmost location of the original Const
location and the coercion location. Clearly, he judged that the
current exact set of behaviours with regard to caret position were
optimal. It is my contention that:

A. They may not actually be optimal, at least not according to my
taste. At the very least, it is a hack to misrepresent the location of
Const nodes just so the core system can blame things on Const nodes
and have the user see the coercion being at fault. I appreciate that
it wouldn't have seemed to matter at the time, but the fact remains.

B. The question of where the caret goes in relevant cases - the
location of the coercion, or the location of the constant - is
inconsequential to the vast majority of Postgres users, if not all,
even if the existing behaviour is technically superior according to
the prevailing aesthetic. On the other hand, it matters a lot to me
that I be able to trust the Const location under all circumstances -
I'd really like to not have to engineer a way around this behaviour,
because the only way to do that is with tricks with the low-level
scanner API, which would be quite brittle. The fact that "select
integer '5'" is canonicalised to "select ?" isn't very pretty. That's
not the only issue though, as even to get that more limited behaviour
lots more code is required, that is more difficult to verify as
correct. "Canonicalise one token at each Const location" is a simple
and robust approach, if only the core system could be tweaked to make
this assumption hold in all circumstances, rather than just the vast
majority.

Tom's point example does not seem to be problematic to me - the cast
*should* blame the 42 const token, as the cast doesn't work as a
result of its representation, which is in point of fact why the core
system blames the Const node and not the coercion one. For that
reason, the constant vs expression thing strikes me as false
equivalency. All of that said, I must reiterate that the difference in
behaviour strike me as very unimportant, or it would if it was not so
important to what I'm trying to do with pg_stat_statements.

Can this be accommodated? It might be a matter of changing the core
system to blame the coercion node rather than the Const node, if
you're determined to preserve the existing behaviour.

[1] http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=a2794623d292f7bbfe3134d1407281055acce584

[2] http://git.postgresql.org/gitweb/?p=postgresql.git;a=blobdiff;f=src/backend/parser/parse_coerce.c;h=cd9b7b0cfbed03ec74f2cf295e4a7113627d7f72;hp=1244498ffb291b67d35917a6fdddb54b0d8d759d;hb=a2794623d292f7bbfe3134d1407281055acce584;hpb=6734182c169a1ecb74dd8495004e896ee4519adb

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