Re: Planning counters in pg_stat_statements (using pgss_store)

On Sun, Mar 1, 2020 at 3:55 PM legrand legrand
<legrand_legrand(at)hotmail(dot)com> wrote:
>
> >> I like the idea of adding a check for a non-zero queryId in the new
> >> pgss_planner_hook() (zero queryid shouldn't be reserved for
> >> utility_statements ?).
>
> > Some assert hit later, I can say that it's not always true. For
> > instance a CREATE TABLE AS won't run parse analysis for the underlying
> > query, as this has already been done for the original statement, but
> > will still call the planner. I'll change pgss_planner_hook to ignore
> > such cases, as pgss_store would otherwise think that it's a utility
> > statement. That'll probably incidentally fix the IVM incompatibility.
>
> Today with or without test on parse->queryId != UINT64CONST(0),
> CTAS is collected as a utility_statement without planning counter.
> This seems to me respectig the rule, not sure that this needs any
> new (risky) change to the actual (quite stable) patch.

But the queryid ends up not being computed the same way:

# select queryid, query, plans, calls from pg_stat_statements where
query like 'create table%';
queryid | query | plans | calls
---------------------+--------------------------------+-------+-------
8275950546884151007 | create table test as select 1; | 1 | 0
7546197440584636081 | create table test as select 1 | 0 | 1
(2 rows)

That's because CreateTableAsStmt->query doesn't have a query
location/len, as transformTopLevelStmt is only setting that for the
top level Query. That's probably an oversight in ab1f0c82257, but I'm
not sure what's the best way to fix that. Should we pass that
information to all transformXXX function, or let transformTopLevelStmt
handle that.