Re: BUG: pg_stat_statements query normalization issues with combined queries

Hello,

At Tue, 27 Dec 2016 10:28:53 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20161227(dot)102853(dot)204155513(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> Putting the two above together, the following is my suggestion
> for the parser part.
>
> - Make all parse nodes have the following two members at the
> beginning. This unifies all parse node from the view of setting
> their location. Commenting about this would be better.
>
> | NodeTag type;
> | int location;
>
> - Remove struct ParseNode and setQueryLocation. stmtmulti sets
> location in the same manner to other kind of nodes, just doing
> '= @n'. base_yyparse() doesn't calculate statement length.
>
> - Make raw_parser caluclate statement length then store it into
> each stmt scanning the yyextra.parsetree.

An additional comment on parser(planner?) part.

This patch make planner() copy the location and length from
parse to result, but copying such stuffs is a job of
standard_planner.

Then the following is a comment on pg_stat_statements.c

- pg_stat_statements.c:977 - isParseNode(node)
node should be parsenode.

- The name for the addional parameter query_loc is written as
query_len in its prototype.

- In pgss_store, "else if (strlen(query)) != query_len)" doesn't
work as expected because of one-off error. query_len here is
the length of the query *excluding* the last semicolon.

- qtext_store doesn't rely on terminating nul character and the
function already takes query length as a parameter. So, there's
no need to copy the partial debug_query_string into palloc'ed
memory. Just replacing the query with query_loc will be
sufficient.

Have a nice holidays.

--
Kyotaro Horiguchi
NTT Open Source Software Center