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

On Fri, Feb 7, 2020 at 11:12 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Thu, Feb 06, 2020 at 02:59:09PM -0500, Robert Haas wrote:
> > On Wed, Feb 5, 2020 at 9:32 AM Julien Rouhaud <julien.rouhaud@free.fr> wrote:
> > > There's also the possibility to reserve 1 bit of the hash to know if
> > > this is a utility command or not, although I don't recall right now
> > > all the possible issues with utility commands and some special
> > > handling of them.  I'll work on it before the next commitfest.
> >
> > FWIW, I don't really see why it would be bad to have 0 mean that
> > "there's no query ID for some reason" without caring whether that's
> > because the current statement is a utility statement or because
> > there's no statement in progress at all or whatever else. The user
> > probably doesn't need our help to distinguish between "no statement"
> > and "utility statement", right?
>
> Sure, but if we don't fix that it means that we also won't expose any queryid
> for utility statement, even if pg_stat_statements is configured to track those
> (with a very poor queryid handling, but still).
>
> While looking at this again, I realized that pg_stat_statements doesn't compute
> a queryid during the post parse analysis hook just to make sure that no query
> identifier will be set during executorStart and the rest of executor functions.
>
> AFAICT, that can't happen anyway since pg_plan_queries() will discard any
> computed queryid for utility statements.  This seems to be an oversight due to
> original pg_stat_statements implementation, so I fixed this.
>
> Then, as processUtility is called between parse analysis and executor, I think
> that we can simply work around this by computing utility statements query
> identifier during parse analysis, removing it in pgss_ProcessUtility and
> keeping a copy of it for the pgss_store calls in that function, as done in the
> attached v5.
>
> This fixes everything except EXECUTE statements, which has to get the
> underlying query's queryid.  The problem is that EXECUTE won't get through
> parse analysis, so while it's correctly handled for execution and pgss_store,
> it's not being exposed in pg_stat_activity and log_line_prefix.  To fix it, I
> added an extra call to pgstat_report_queryid in executorStart.  As this
> function is a no-op if a queryid is already exposed, this shouldn't cause any
> harm and fix any other cases of query execution that don't go through parse
> analysis.
>
> Finally, DEALLOCATE is entirely ignored by pg_stat_statements, so those
> statements will always be reported with a NULL/0 queryid, but this is
> consistent as it's also not present in pg_stat_statements() SRF.

cfbot reports a failure since 2f9661311b (command completion tag
change), so here's a rebased v6, no change otherwise.