Re: compute_query_id and pg_stat_statements

On Sat, May 15, 2021 at 7:50 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> 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).

If no query_id is calculated and you have entries in
pg_stat_statements, it means someone deliberately deactivated
compute_query_id. In that case it's clear that they know the GUC
exists, so there's no much point in warning them that they deactivated
it I think.

> Maybe there is an useful warning to add, but I think it's independent of
> changing the GUC behabior.

I'm fine with it.

> > 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.

Unless I'm missing something, if we remove that instruction it means
that we encourage users to dynamically change the query_id source
without any safeguard, which will in the majority of case result in
unwanted behavior, going from duplicated entries, poor performance in
pg_stat_statements if that leads to more evictions, or even totally
bogus metrics if that leads to hash collision.

> 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.

Couldn't it already be achieved with ALTER [ DATABASE | USER ] foo SET
pg_stat_statements.track = [ none | top | all ] ?