Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?

Hi,

On Sat, Aug 3, 2019 at 1:21 AM Andres Freund <andres@anarazel.de> wrote:
>
> On 2019-08-02 10:54:35 +0200, Julien Rouhaud wrote:
> > However having the nested queryid in
> > pg_stat_activity would be convenient to track what is a long stored
> > functions currently doing.  Maybe we could expose something like
> > top_level_queryid and current_queryid instead?
>
> Given that the query string is the toplevel one, I think that'd just be
> confusing. And given the fact that it adds *substantial* additional
> complexity, I'd just rip the subcommand bits out.

Ok, so here's a version that only exposes the top-level queryid only.
There can still be discrepancies with the query field, if a
multi-command string is provided.  The queryid will be updated each
time a new top level statement is executed.

As the queryid cannot be immediately known, and may never exist at all
if a query fails to parse, here are the heuristic I used to update the
stored queryid:

- it's reset to 0 each time pgstat_report_activity(STATE_RUNNING) is
called.  This way, we're sure that we don't display last query's
queryid in the logs if the next query fails to parse
- it's also reset to 0 at the beginning of exec_simple_query() loop on
the parsetree_list (for multi-command string case)
- pg_analyze_and_rewrite() and pg_analyze_and_rewrite_params() will
report the new queryid after parse analysis.
- a non-zero queryid will only be updated if the stored one is zero

This should also work as intended for background worker using SPI,
provided that they correctly call pgstat_report_activity.  I also
modified ExecInitParallelPlan() to publish the queryId in the
serialized plannedStmt, so ParallelQueryMain() can report it to make
the queryid available in the parallel workers too.

Note that this patch makes it clear that a zero queryid means no
queryid computed (and NULL will be displayed in such case in
pg_stat_activity).  pg_stat_statements already makes sure that it
cannot compute a zero queryid.

It also assume that any extension computing a queryid will do that in
the post_parse_analysis hook, which seems like a sane requirement.  We
may want to have a dedicated hook for that instead, if more people get
interested in having the queryid only, possibly different
implementations, if it becomes available outside pgss.