Re: [HACKERS] BUG: pg_stat_statements query normalization issueswith combined queries

Hello Tom,

> Yeah, I doubt that this technique for getting the raw locations in the
> grammar+lexer works reliably.

It worked reliably on all tests, and the assumption seemed sound to me, 
given the one-look-ahead property and that statement reductions can only 
triggered when ';' or end of input comes.

> Anyway, I decided to see what it would take to do it the way I had
> in mind, which was to stick a separate RawStmt node atop each statement
> parsetree.  The project turned out a bit larger than I hoped :-(,

Indeed: I tried it as it looked elegant, but it did not show to be the 
simple thing you announced...

> [...] 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.

> So this ends up costing one extra palloc per statement, or two extra
> in the case of a utility statement, but really that's quite negligible
> compared to everything else that happens in processing a statement.

> As against that, I think it makes the related code a lot clearer.

Having spent some time there, I agree that a refactoring and cleanup was 
somehow needed...

> The sheer size of the patch is a bit more than Fabien's patch,

Yep, nearly twice as big and significantly more complex, but the result is 
a definite improvement.

> but what it is touching is not per-statement-type code but code that 
> works generically with lists of statements.  So I think it's less likely 
> to cause problems for other patches.

Patch applies (with "patch", "git apply" did not like it though), 
compiles, overall make check ok, pgss make check ok as well. I do not know 
whether the coverage of pg tests is enough to know whether anything is 
broken...

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.

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*. That would 
avoid some casts in updateRawStmtEnd, but maybe add some other elsewhere.

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.

-- 
Fabien.