Re: compute_query_id and pg_stat_statements

On 2021-May-14, Julien Rouhaud wrote:

> On Fri, May 14, 2021 at 12:20:00PM +0900, Fujii Masao wrote:

> > +void
> > +EnableQueryId(void)
> > +{
> > + if (compute_query_id == COMPUTE_QUERY_ID_AUTO)
> > + auto_query_id_enabled = true;
> >
> > Shouldn't EnableQueryId() enable auto_query_id_enabled whatever compute_query_id is?
> > Otherwise, for example, the following scenario can happen and it's a bit strange.
> >
> > 1. The server starts up with shared_preload_libraries=pg_stat_statements and compute_query_id=on
> > 2. compute_query_id is set to auto and the configuration file is reloaded
> > Then, even though compute_query_id is auto and pg_stat_statements is loaded,
> > query ids are not computed and no queries are tracked by pg_stat_statements.
>
> +1.

That makes sense. Done in this version.

I took out the new WARNING in pg_stat_statements. It's not clear to me
that that's terribly useful (it stops working as soon as you have one
query in the pg_stat_statements stash and later disable everything).
Maybe there is an useful warning to add, but I think it's independent of
changing the GUC behabior.

I also made IsQueryIdEnabled() a static inline in queryjumble.h, to
avoid a function call at every site where we need that. Also did some
light doc editing.

I think I should aim at pushing this tomorrow morning.

> Note that if you switch from compute_query_id = off + custom
> query_id + pg_stat_statements to compute_query_id = auto then thing will
> immediately break (as we instruct third-party plugins authors to error out in
> that case), which is way better than breaking at the next random service
> restart.

Hmm, ok. I tested pg_queryid and that behavior of throwing an error
seems quite unhelpful -- it basically makes every single query fail if
you set things wrong. I think that instruction is bogus and should be
reconsidered. Maybe pg_queryid could use a custom Boolean GUC that
tells it to overwrite the core query_id if that is enabled, or to just
silently do nothing in that case.

While thinking about this patch it occurred to that an useful gadget
might be to let pg_stat_statements be loaded always, but with
compute_query_id=false; so it's never active; except if you
ALTER {DATABASE, USER} foo SET (compute_query_id=on);
so that it's possible to enable it selectively. I think this doesn't
currently achieve anything because pgss_store is always called
regardless of query ID being active (so you'd always have at least one
function call as performance penalty, only that the work would be for
naught), but that seems a simple change to make. I didn't look closely
to see what other things would need patched.

--
Álvaro Herrera 39°49'30"S 73°17'W
Maybe there's lots of data loss but the records of data loss are also lost.
(Lincoln Yeoh)