At Wed, 12 May 2021 18:09:30 +0800, Julien Rouhaud <rjuju...@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 <rjuju...@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
