> 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.
I put the copy in planner because standard_planner may not be called by
planner, and in all cases I think that the fields should be copied...
Otherwise it is the responsability of the planner hook to do the copy, it
would is ok if the planner hook calls standard_planner, but the fact is
not warranted, the comments says "the plugin would normally call
standard_planner". What if it does not?
So it seemed safer this way.
> Then the following is a comment on pg_stat_statements.c
>
> - pg_stat_statements.c:977 - isParseNode(node)
> node should be parsenode.
Could be. ParseNode is a Node, it is just a pointer cast. I can assert on
pn instead of node.
> - The name for the addional parameter query_loc is written as
> query_len in its prototype.
Indeed, a typo on "generate_normalized_query" 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.
It was somehow intentional: if the query is not the same as the string,
then a copy is performed to have the right null-terminated string...
but
> - 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.
Hmmm... it seemed good so I tried it, and it did not work...
The subtle reason is that qtext_store does assume that the query string is
null terminated... it just does not recompute the length its length.
However it can be changed to behave correctly in this case, so as to avoid
the copy.
--
Fabien.
