Re: compute_query_id and pg_stat_statements

At Wed, 12 May 2021 18:09:30 +0800, Julien Rouhaud <rjuju123@gmail.com> wrote in 
> On Wed, May 12, 2021 at 06:37:24PM +0900, Kyotaro Horiguchi wrote:
> > At Wed, 12 May 2021 14:05:16 +0800, Julien Rouhaud <rjuju123@gmail.com> wrote in 
> > > 
> > > And if I'm not mistaken, pg_store_plans also wants a different queryid
> > > implementation, but has to handle a secondary queryid on top of it
> > > (https://github.com/ossc-db/pg_store_plans/blob/master/pg_store_plans.c#L843-L855).
> > 
> > Yeah, the extension intended to be used joining with the
> > pg_stat_statements view. And the reason for the second query-id dates
> > back to the era when query id was not available in the
> > pg_stat_statements view. Now it is mere a fall-back query id when
> > pg_stat_statments is not active.  Now that the in-core query-id is
> > available, I think there's no reason to keep that implement.
> > 
> > > So here again what the extension want is to get rid of pg_stat_statements (and
> > > now core) queryid implementation.
> > 
> > So the extension might be a good reason for the discussion^^;
> 
> Indeed.  So IIUC, what pg_store_plans wants is:

Ugg. Very sorry. My brain should need more oxygen, or caffeine..  The
last sentence forgetting a negation. The plugin does not need a
special query-id provider so the special provider can be removed
without problems if the core provides one.

> - to use its own query_id implementation
> - to be able to be joined to pg_stat_statements
> 
> Is that correct?

It is correct, but a bit short in detail.

The query_id of its own is provided because pg_stat_statements did not
expose query_id. And it has been preserved only for the case the
plugin is used without pg_stat_statements activated.  Now that the
in-core query_id is available, the last reason for the special
provider has gone.

> If yes, it seems that starting with pg14, it can be easily achieved by:

So, it would be a bit different.

> - documenting to disable compute_query_id

 documenting to *not disable* compute_query_id. That is set it to on
 or auto.

> - eventually error out at execution time if it's enabled

So. the extension would check if any query_id provider *is* active.

> - don't call queryIdWanted()
> - expose its query_id
> 
> It will then work just fine, and will be more efficient compared to what is
> done today as only one queryid will be calculated.

After reading Magnus' comment nearby, I realized that my most
significant concern here is how to know any query_id provider is
active.  The way of setting the hook cannot enforce notifying that
kind of things on plugins. For me implementing them as a dll looked as
one of the most promising way of enabling that without needing any
boiler-plates.

Another not-prefect (in that it needs a boiler-plate) but workable way
is letting query-id providers set some variable including GUC
explicitly as Magnus' suggested.  GUC would be better in that it is
naturally observable from users.

Even though there'a possibility that a developer of a query_id
provider forgets to set it, but maybe it would be easily
noticeable. On the other hand it gives a sure means to know any
query_id provider is active.

How about adding a GUC_INTERNAL "current_query_provider" or such?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center