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

On Thu, Mar 25, 2021 at 10:36:38AM +0800, Julien Rouhaud wrote:
> On Wed, Mar 24, 2021 at 01:02:00PM -0300, Alvaro Herrera wrote:
> > On 2021-Mar-24, Julien Rouhaud wrote:
> > 
> > > From e08c9d5fc86ba722844d97000798de868890aba3 Mon Sep 17 00:00:00 2001
> > > From: Bruce Momjian <bruce@momjian.us>
> > > Date: Mon, 22 Mar 2021 17:43:23 -0400
> > > Subject: [PATCH v20 2/3] Expose queryid in pg_stat_activity and
> > 
> > >  src/backend/executor/execMain.c               |   9 ++
> > >  src/backend/executor/execParallel.c           |  14 ++-
> > >  src/backend/executor/nodeGather.c             |   3 +-
> > >  src/backend/executor/nodeGatherMerge.c        |   4 +-
> > 
> > Hmm...
> > 
> > I find it odd that there's executor code that acquires the current query
> > ID from pgstat, after having been put there by planner or ExecutorStart
> > itself.  Seems like a modularity violation.  I wonder if it would make
> > more sense to have the value maybe in struct EState (or perhaps there's
> > a better place -- but I don't think they have a way to reach the
> > QueryDesc anyhow), put there by ExecutorStart, so that places such as
> > execParallel, nodeGather etc don't have to fetch it from pgstat but from
> > EState.
> 
> The current queryid is already available in the Estate, as the underlying
> PlannedStmt contains it.  The problem is that we want to display the top level
> queryid, not the current query one, and the top level queryid is held in
> pgstat.

So is the current approach ok?  If not I'm afraid that detecting and caching
the top level queryid in the executor parts would lead to some code
duplication.