Re: Planning counters in pg_stat_statements (using pgss_store)

On Sat, Mar 23, 2019 at 11:08 PM legrand legrand
<legrand_legrand(at)hotmail(dot)com> wrote:
>
> > This patch has multiple trailing whitespace, indent and coding style
> > issues. You should consider running pg_indent before submitting a
> > patch. I attach the diff after running pgindent if you want more
> > details about the various issues.
>
> fixed

There are still trailing whitespaces and comments wider than 80
characters in the C code that should be fixed.

> > + pgss_store("",
> > + parse->queryId, /* signal that it's a
> > utility stmt */
> > + -1,
>
> > the comment makes no sense, and also you can't pass an empty query
> > string / unknown len. There's no guarantee that the entry for the
> > given queryId won't have been evicted, and in this case you'll create
> > a new and unrelated entry.
>
> Fixed, comment was wrong
> Query text is not available in pgss_planner_hook
> that's why pgss_store execution is forced in pgss_post_parse_analyze
> (to initialize pgss entry with its query text).
>
> There is a very small risk that query has been evicted between
> pgss_post_parse_analyze and pgss_planner_hook.
>
> > @@ -832,13 +931,13 @@ pgss_post_parse_analyze(ParseState *pstate, Query *query)
> > * the normalized string would be the same as the query text anyway, so
> > * there's no need for an early entry.
> > */
> > - if (jstate.clocations_count > 0)
> > pgss_store(pstate->p_sourcetext,
>
> > Why did you remove this? pgss_store() isn't free, and asking to
> > generate a normalized query for a query that doesn't have any constant
> > or storing the entry early won't do anything useful AFAICT. Though if
> > that's useful, you definitely can't remove the test without adapting
> > the comment and the indentation.
>
> See explanation in previous answer (comments have been updated accordingly)

The alternative being to expose query text to the planner, which could
fix this (unlikely) issue and could also sometimes save a pgss_store()
call. I did a quick check and at least AQO and pg_hint_plan
extensions have some hacks to be able to access the query text from
the planner, so there are at least multiple needs for that. Perhaps
it's time to do it?

> > there are 4 tests to check if planning_time is zero or not, it's quite
> > messy. Could you refactor the code to avoid so many tests? It would
> > probably be useful to add some asserts to check that we don't provide
> > both planning_time == 0 and execution related values. The function's
> > comment would also need to be adapted to mention the new rationale
> > with planning_time.
>
> Fixed

+ /* updating counters for execute OR planning */
+ Assert(planning_time > 0 && total_time > 0);
+ if (planning_time == 0)

This is obviously incorrect. The general sanity check for exclusion
between planning_time and total_time should be at the beginning of
pgss_store. Maybe some others asserts are needed to verify that
planning_time cannot be provided along jstate or other conditions.