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

Hello,

At Tue, 27 Dec 2016 10:28:53 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20161227.102853.204155513.horiguchi.kyotaro@lab.ntt.co.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
hereis 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'sno need to copy the partial debug_query_string into palloc'ed memory.  Just replacing the query with query_loc
willbe sufficient.
 

Have a nice holidays.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center