Re: BUG: pg_stat_statements query normalization issues with combined queries

Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> writes:
>> [...] Furthermore, the output of pg_plan_queries is now always a list of
>> PlannedStmt nodes, even for utility statements.

> I stopped exactly there when I tried: I thought that changing that would
> be enough to get a reject because it would be considered much too invasive
> in too many places for fixing a small bug.

Well, I don't know that we'd have bothered to change this without a
reason, but now that the effort's been put in, it seems a lot cleaner
to me.

> I noticed that you also improved the pgss fix and tests. Good. I was
> unsure about removing spaces at the end of the query because of possible
> encoding issues.

It's safe in backend-legal encodings. I agree it could be problematic
in, say, SJIS, but we disallow that as a backend encoding precisely
because of this type of hazard.

> The update end trick is nice to deal with empty statements. I wonder
> whether the sub "query" in Copy, View, CreateAs, Explain, Prepare,
> Execute, Declare... could be typed RawStmt* instead of Node*.

I did that for some of them, eg CopyStmt. The trouble for statements like
EXPLAIN is that we replace the sub-statement during parse analysis, so
that the field points to either a raw grammar output node or to a Query
depending on when you look. If we wanted to get rid of using a Node*
field there, we'd have to invent two separate struct types to represent
raw EXPLAIN and post-parse-analyze EXPLAIN (analogously to the way that,
say, ColumnRef and Var are distinct node types). It doesn't seem worth
the trouble to me.

> One point bothered me a bit when looking at the initial code, that your
> refactoring has not changed: the location is about a string which is not
> accessible from the node, so that the string must be kept elsewhere and
> passed as a second argument here and there. ISTM that it would make some
> sense to have the initial string directly available from RawStmt, Query
> and PlannedStmt.

Yeah, perhaps, but that would be something to tackle as a separate round
of changes I think. This might also tie into the nearby thread about
having that string available to pass to parallel workers. The history
here is that we didn't use to keep the source string around after parsing,
but that's not the case anymore. But we've not entirely accepted that
in all the internal APIs ...

regards, tom lane