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

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

- 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

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.


Sent via pgsql-hackers mailing list (
To make changes to your subscription:

Reply via email to