compute_query_id and pg_stat_statements

Le sam. 24 avr. 2021 à 22:54, Fujii Masao <masao.fujii@oss.nttdata.com> a
écrit :

For example, we already have one report about this issue, in [1].

this report was only a few days after the patch changing the behavior was
committed, unless you've been following the original thread (which has been
going on for 2 years), that's kind of expected. release notes for pg14
should highlight that change, so hopefully people upgrading will see it.
I'll also try to write some blog article about it to add more warnings.

Shouldn't we do something so that users can avoid such mis-configuration?

One idea is to change the default value of compute_query_id from false to
true.
If enabling compute_query_id doesn't incur any performance penalty,
IMO this idea is very simple and enough.

it adds some noticeable overhead in oltp style workloads. I think that I
did some benchmarks in the original thread, and we decided not to enable it
by default

Another idea is to change pg_stat_statements so that it emits an error

at the server startup (i.e., prevents the server from starting up)
if compute_query_id is not enabled. In this case, users can easily notice
the mis-configuration from the error message in the server log,
enable compute_query_id, and then restart the server.

that's also not an option, as one can now use pg_stat_statetements with a
different queryid calculation. see for instance
https://github.com/rjuju/pg_queryid for a proof a concept extension for
that. I think it's clear that multiple people will want to use a different
calculation as they have been asking for that for years.

IMO the former is better if there is no such performance risk. Otherwise

we should adopt the latter approach. Or you have the better idea?

I'm not sure how to address that, as temporarily disabling queryId
calculation should be allowed. maybe we could raise a warning once per
backend if pgss sees a dml query without queryId? but it could end up
creating more problems than it solves.

for the record people have also raised bugs on the powa project because
planning counters are not tracked by default, so compute_query_id will
probably add a bit of traffic.

On Sat, Apr 24, 2021 at 11:54:25PM +0900, Fujii Masao wrote:

When compute_query_id is not enabled (this is the default setting),
pg_stat_statements doesn't track any statements. This means that
we will see no entries in pg_stat_statements by default. I'm afraid that
users may easily forget to enable compute_query_id
when using pg_stat_statements (because this setting was not necessary
in v13 or before), and finally may have noticed the mis-configuration
and failure of statements tracking after many queries were executed.
For example, we already have one report about this issue, in [1].

Shouldn't we do something so that users can avoid such mis-configuration?

One idea is to change the default value of compute_query_id from false to true.
If enabling compute_query_id doesn't incur any performance penalty,
IMO this idea is very simple and enough.

I think the query overhead was too high (2%) to enable it by default:

/messages/by-id/20201016160355.GA31474@alvherre.pgsql

Another idea is to change pg_stat_statements so that it emits an error
at the server startup (i.e., prevents the server from starting up)
if compute_query_id is not enabled. In this case, users can easily notice
the mis-configuration from the error message in the server log,
enable compute_query_id, and then restart the server.

I think it throws an error in the server logs, but preventing server
start seems extreme. Also, compute_query_id is PGC_SUSET, meaning it
can be changed by the super-user, so you could enable compute_query_id
without a server restart, which makes failing on start kind of odd.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

If only the physical world exists, free will is an illusion.

On Sat, Apr 24, 2021 at 5:22 PM Bruce Momjian <bruce@momjian.us> wrote:

On Sat, Apr 24, 2021 at 11:54:25PM +0900, Fujii Masao wrote:

When compute_query_id is not enabled (this is the default setting),
pg_stat_statements doesn't track any statements. This means that
we will see no entries in pg_stat_statements by default. I'm afraid that
users may easily forget to enable compute_query_id
when using pg_stat_statements (because this setting was not necessary
in v13 or before), and finally may have noticed the mis-configuration
and failure of statements tracking after many queries were executed.
For example, we already have one report about this issue, in [1].

Shouldn't we do something so that users can avoid such mis-configuration?

One idea is to change the default value of compute_query_id from false to true.
If enabling compute_query_id doesn't incur any performance penalty,
IMO this idea is very simple and enough.

I think the query overhead was too high (2%) to enable it by default:

/messages/by-id/20201016160355.GA31474@alvherre.pgsql

Personally I'd say 2% is not too high to turn it on by default, as it
goes down when you move past trivial queries, which is what most
people do. And since you can easily turn it off.

Another idea is to change pg_stat_statements so that it emits an error
at the server startup (i.e., prevents the server from starting up)
if compute_query_id is not enabled. In this case, users can easily notice
the mis-configuration from the error message in the server log,
enable compute_query_id, and then restart the server.

I think it throws an error in the server logs, but preventing server
start seems extreme. Also, compute_query_id is PGC_SUSET, meaning it
can be changed by the super-user, so you could enable compute_query_id
without a server restart, which makes failing on start kind of odd.

How about turning it into an enum instead of a boolean, that can be:

off = always off
auto = pg_stat_statments turns it on when it's loaded in
shared_preload_libraries. Other extensions using it can do that to.
But it remains off if you haven't installed any *extension* that needs
it
on = always on (if you want it in pg_stat_activity regardless of extensions)

The default would be "auto", which means that pg_stat_statements would
work as expected, but those who haven't installed it (or another
extension that changes it) would not have to pay the overhead.

--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/

On Sat, Apr 24, 2021 at 06:48:53PM +0200, Magnus Hagander wrote:

I think the query overhead was too high (2%) to enable it by default:

/messages/by-id/20201016160355.GA31474@alvherre.pgsql

Personally I'd say 2% is not too high to turn it on by default, as it
goes down when you move past trivial queries, which is what most
people do. And since you can easily turn it off.

We would do a lot of work to reduce overhead by 2% on every query, and
to add 2% for a hash that previously was only used by pg_stat_statements
seems unwise.

How about turning it into an enum instead of a boolean, that can be:

off = always off
auto = pg_stat_statments turns it on when it's loaded in
shared_preload_libraries. Other extensions using it can do that to.
But it remains off if you haven't installed any *extension* that needs
it
on = always on (if you want it in pg_stat_activity regardless of extensions)

The default would be "auto", which means that pg_stat_statements would
work as expected, but those who haven't installed it (or another
extension that changes it) would not have to pay the overhead.

That's a pretty weird API. I think we just need people to turn it on
like they are doing when the configure pg_stat_statements anyway.
pg_stat_statements already requires configuration anyway.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

If only the physical world exists, free will is an illusion.

Bruce Momjian <bruce@momjian.us> writes:

That's a pretty weird API. I think we just need people to turn it on
like they are doing when the configure pg_stat_statements anyway.
pg_stat_statements already requires configuration anyway.

Agreed. If pg_stat_statements were zero-configuration today then
this would be an annoying new burden, but it isn't.

I haven't looked, but did we put anything into pg_stat_statements
to make it easy to tell if you've messed up this setting?

regards, tom lane

On Sat, Apr 24, 2021 at 01:43:51PM -0400, Tom Lane wrote:

I haven't looked, but did we put anything into pg_stat_statements
to make it easy to tell if you've messed up this setting?

You mean apart from from having pg_stat_statements' view/SRFs returning
nothing?

I think it's a reasonable use case to sometime disable query_id calculation,
eg. if you know that it will only lead to useless bloat in the entry and that
you won't need the info, so spamming warnings if there are no queryid could
cause some pain.

Julien Rouhaud <rjuju123@gmail.com> writes:

On Sat, Apr 24, 2021 at 01:43:51PM -0400, Tom Lane wrote:

I haven't looked, but did we put anything into pg_stat_statements
to make it easy to tell if you've messed up this setting?

You mean apart from from having pg_stat_statements' view/SRFs returning
nothing?

I think it's a reasonable use case to sometime disable query_id calculation,
eg. if you know that it will only lead to useless bloat in the entry and that
you won't need the info, so spamming warnings if there are no queryid could
cause some pain.

I agree repeated warnings would be bad news. I was wondering if we could
arrange a single warning at the time pg_stat_statements is preloaded into
the postmaster. In this way, if you tried to use a configuration file
that used to work, you'd hopefully get some notice about why it no longer
does what you want. Also, if you are preloading pg_stat_statements, it
seems reasonable to assume that you'd like the global value of the flag
to be "on", even if there are use-cases for transiently disabling it.

I think the way to detect "being loaded into the postmaster" is
if (IsPostmasterEnvironment && !IsUnderPostmaster)

regards, tom lane

On Sun, Apr 25, 2021 at 11:39:55AM -0400, Tom Lane wrote:

I agree repeated warnings would be bad news. I was wondering if we could
arrange a single warning at the time pg_stat_statements is preloaded into
the postmaster. In this way, if you tried to use a configuration file
that used to work, you'd hopefully get some notice about why it no longer
does what you want. Also, if you are preloading pg_stat_statements, it
seems reasonable to assume that you'd like the global value of the flag
to be "on", even if there are use-cases for transiently disabling it.

What about people who wants to use pg_stat_statements but are not ok with our
query_id heuristics and use a third-party plugin for that?

Julien Rouhaud <rjuju123@gmail.com> writes:

On Sun, Apr 25, 2021 at 11:39:55AM -0400, Tom Lane wrote:

I agree repeated warnings would be bad news. I was wondering if we could
arrange a single warning at the time pg_stat_statements is preloaded into
the postmaster. In this way, if you tried to use a configuration file
that used to work, you'd hopefully get some notice about why it no longer
does what you want. Also, if you are preloading pg_stat_statements, it
seems reasonable to assume that you'd like the global value of the flag
to be "on", even if there are use-cases for transiently disabling it.

What about people who wants to use pg_stat_statements but are not ok with our
query_id heuristics and use a third-party plugin for that?

They're still going to want the GUC set to something other than "off",
no? In any case it's just a one-time log message, so it's not likely
to be *that* annoying.

regards, tom lane

On Sun, Apr 25, 2021 at 01:17:03PM -0400, Tom Lane wrote:

Julien Rouhaud <rjuju123@gmail.com> writes:

On Sun, Apr 25, 2021 at 11:39:55AM -0400, Tom Lane wrote:

I agree repeated warnings would be bad news. I was wondering if we could
arrange a single warning at the time pg_stat_statements is preloaded into
the postmaster. In this way, if you tried to use a configuration file
that used to work, you'd hopefully get some notice about why it no longer
does what you want. Also, if you are preloading pg_stat_statements, it
seems reasonable to assume that you'd like the global value of the flag
to be "on", even if there are use-cases for transiently disabling it.

What about people who wants to use pg_stat_statements but are not ok with our
query_id heuristics and use a third-party plugin for that?

They're still going to want the GUC set to something other than "off",
no?

They will want compute_query_id to be off. And they actually will *need* that,
as we recommend third-party plugins computing alternative query_id to error out
if they see a that a query_id has already been generated, to avoid any problem
if compute_query_id is being temporarily toggled. That's what I did in the POC
plugin for external query_id at [1]https://github.com/rjuju/pg_queryid/blob/master/pg_queryid.c#L172.

In any case it's just a one-time log message, so it's not likely
to be *that* annoying.

In that case it should be phrased in a way that makes it clear that
pg_stat_statements can work without enabling compute_query_id, something like:

"compute_query_id is disabled. This module won't track any activity unless you
configured a third-party extension that computes query identifiers"

[1]: https://github.com/rjuju/pg_queryid/blob/master/pg_queryid.c#L172

On 24.04.21 19:43, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

That's a pretty weird API. I think we just need people to turn it on
like they are doing when the configure pg_stat_statements anyway.
pg_stat_statements already requires configuration anyway.

Agreed. If pg_stat_statements were zero-configuration today then
this would be an annoying new burden, but it isn't.

I think people can understand "add pg_stat_statements to
shared_preload_libraries" and "install the extension". You have to turn
it on somehow after all.

Now there is the additional burden of turning on this weird setting that
no one understands. That's a 50% increase in burden.

And almost no one will want to use a nondefault setting.

pg_stat_statements is pretty popular. I think leaving in this
requirement will lead to widespread confusion and complaints.

Re: Peter Eisentraut

Agreed. If pg_stat_statements were zero-configuration today then
this would be an annoying new burden, but it isn't.

I think people can understand "add pg_stat_statements to
shared_preload_libraries" and "install the extension". You have to turn it
on somehow after all.

Fwiw, I'd claim that pg_stat_statements *is* zero-configuration today.
You just have to load the module (= shared_preload_libraries), and it
will start working. Later you can run CREATE EXTENSION to actually see
the stats, but they are already being collected in the background.

Now there is the additional burden of turning on this weird setting that no
one understands. That's a 50% increase in burden.

And almost no one will want to use a nondefault setting.

pg_stat_statements is pretty popular. I think leaving in this requirement
will lead to widespread confusion and complaints.

Ack, please make the default config (i.e. after setting shared_preload_libraries)
do something sensible. Having to enable some "weird" internal other setting
will be very hard to explain to users.

Fwiw, I'd even want to have pg_stat_statements enabled in core by
default. That would awesome UX. (And turning off could be as simple as
setting compute_query_id=off.)

Christoph

On Mon, Apr 26, 2021 at 05:34:30PM +0200, Christoph Berg wrote:

Re: Peter Eisentraut

Agreed. If pg_stat_statements were zero-configuration today then
this would be an annoying new burden, but it isn't.

I think people can understand "add pg_stat_statements to
shared_preload_libraries" and "install the extension". You have to turn it
on somehow after all.

Fwiw, I'd claim that pg_stat_statements *is* zero-configuration today.
You just have to load the module (= shared_preload_libraries), and it
will start working. Later you can run CREATE EXTENSION to actually see
the stats, but they are already being collected in the background.

Now there is the additional burden of turning on this weird setting that no
one understands. That's a 50% increase in burden.

And almost no one will want to use a nondefault setting.

pg_stat_statements is pretty popular. I think leaving in this requirement
will lead to widespread confusion and complaints.

Ack, please make the default config (i.e. after setting shared_preload_libraries)
do something sensible. Having to enable some "weird" internal other setting
will be very hard to explain to users.

Fwiw, I'd even want to have pg_stat_statements enabled in core by
default. That would awesome UX. (And turning off could be as simple as
setting compute_query_id=off.)

Techically, pg_stat_statements can turn on compute_query_id when it is
loaded, even if it is 'off' in postgresql.conf, right? And
pg_stat_statements would know if an alternate hash method is being used,
right?

This is closer to Magnus's idea of having a three-value
compute_query_id, except is it more controlled by pg_stat_statements.
Another idea would be to throw a user-visible warning if the
pg_stat_statements extension is loaded and compute_query_id is off.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

If only the physical world exists, free will is an illusion.

Greetings,

* Bruce Momjian (bruce@momjian.us) wrote:

On Mon, Apr 26, 2021 at 05:34:30PM +0200, Christoph Berg wrote:

Re: Peter Eisentraut

Agreed. If pg_stat_statements were zero-configuration today then
this would be an annoying new burden, but it isn't.

I think people can understand "add pg_stat_statements to
shared_preload_libraries" and "install the extension". You have to turn it
on somehow after all.

Fwiw, I'd claim that pg_stat_statements *is* zero-configuration today.
You just have to load the module (= shared_preload_libraries), and it
will start working. Later you can run CREATE EXTENSION to actually see
the stats, but they are already being collected in the background.

Now there is the additional burden of turning on this weird setting that no
one understands. That's a 50% increase in burden.

And almost no one will want to use a nondefault setting.

pg_stat_statements is pretty popular. I think leaving in this requirement
will lead to widespread confusion and complaints.

Ack, please make the default config (i.e. after setting shared_preload_libraries)
do something sensible. Having to enable some "weird" internal other setting
will be very hard to explain to users.

Fwiw, I'd even want to have pg_stat_statements enabled in core by
default. That would awesome UX. (And turning off could be as simple as
setting compute_query_id=off.)

Techically, pg_stat_statements can turn on compute_query_id when it is
loaded, even if it is 'off' in postgresql.conf, right? And
pg_stat_statements would know if an alternate hash method is being used,
right?

+1 on this approach. I agree that we should avoid having to make every
new user and every user who is upgrading with pg_stat_statements
installed have to go twiddle this parameter.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:

* Bruce Momjian (bruce@momjian.us) wrote:

Techically, pg_stat_statements can turn on compute_query_id when it is
loaded, even if it is 'off' in postgresql.conf, right? And
pg_stat_statements would know if an alternate hash method is being used,
right?

+1 on this approach.

That'd make it impossible to turn off or adjust afterwards, wouldn't it?
I'm afraid the confusion stemming from that would outweigh any simplicity.

I would be in favor of logging a message at startup to the effect of
"this is misconfigured" (as per upthread discussion), although whether
people would see that is uncertain.

In the end, it's not like this is the first time we've ever made an
incompatible change in configuration needs; and it won't be the last
either. I don't buy the argument that pg_stat_statements users can't
cope with adding the additional setting. (Of course, we should be
careful to call it out as an incompatible change in the release notes.)

regards, tom lane

On Mon, Apr 26, 2021 at 12:56:13PM -0400, Tom Lane wrote:

Stephen Frost <sfrost@snowman.net> writes:

* Bruce Momjian (bruce@momjian.us) wrote:

Techically, pg_stat_statements can turn on compute_query_id when it is
loaded, even if it is 'off' in postgresql.conf, right? And
pg_stat_statements would know if an alternate hash method is being used,
right?

+1 on this approach.

That'd make it impossible to turn off or adjust afterwards, wouldn't it?
I'm afraid the confusion stemming from that would outweigh any simplicity.

I would be in favor of logging a message at startup to the effect of
"this is misconfigured" (as per upthread discussion), although whether
people would see that is uncertain.

I think a user-visible warning at CREATE EXNTENSION would help too.

In the end, it's not like this is the first time we've ever made an
incompatible change in configuration needs; and it won't be the last
either. I don't buy the argument that pg_stat_statements users can't
cope with adding the additional setting. (Of course, we should be
careful to call it out as an incompatible change in the release notes.)

Agreed.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

If only the physical world exists, free will is an illusion.

On Mon, Apr 26, 2021 at 6:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Stephen Frost <sfrost@snowman.net> writes:

* Bruce Momjian (bruce@momjian.us) wrote:

Techically, pg_stat_statements can turn on compute_query_id when it is
loaded, even if it is 'off' in postgresql.conf, right? And
pg_stat_statements would know if an alternate hash method is being used,
right?

+1 on this approach.

That'd make it impossible to turn off or adjust afterwards, wouldn't it?
I'm afraid the confusion stemming from that would outweigh any simplicity.

Thatäs why I suggested the three value one. Default to a mode where
it's automatic, which is what the majority is going to want, but have
a way to explicitly turn it on.

I would be in favor of logging a message at startup to the effect of
"this is misconfigured" (as per upthread discussion), although whether
people would see that is uncertain.

Some people would. Many wouldn't, and sadly many hours would be spent
on debugging things before they got there -- based on experience of
how many people actually read the logs..

In the end, it's not like this is the first time we've ever made an
incompatible change in configuration needs; and it won't be the last
either. I don't buy the argument that pg_stat_statements users can't
cope with adding the additional setting. (Of course, we should be
careful to call it out as an incompatible change in the release notes.)

The fact that we've made changes before that complicated our users
experience isn't in itself an argument for doing it again though...

--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/

On Tue, Apr 27, 2021 at 12:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Stephen Frost <sfrost@snowman.net> writes:

* Bruce Momjian (bruce@momjian.us) wrote:

Techically, pg_stat_statements can turn on compute_query_id when it is
loaded, even if it is 'off' in postgresql.conf, right? And
pg_stat_statements would know if an alternate hash method is being used,
right?

+1 on this approach.

That'd make it impossible to turn off or adjust afterwards, wouldn't it?

I think so, which would also make it impossible to use an external
query_id plugin.

Enabling compute_query_id by default or raising a WARNING in
pg_stat_statements' PG_INIT seems like the only 2 sensible options.

On Mon, Apr 26, 2021 at 7:00 PM Bruce Momjian <bruce@momjian.us> wrote:

On Mon, Apr 26, 2021 at 12:56:13PM -0400, Tom Lane wrote:

Stephen Frost <sfrost@snowman.net> writes:

* Bruce Momjian (bruce@momjian.us) wrote:

Techically, pg_stat_statements can turn on compute_query_id when it is
loaded, even if it is 'off' in postgresql.conf, right? And
pg_stat_statements would know if an alternate hash method is being used,
right?

+1 on this approach.

That'd make it impossible to turn off or adjust afterwards, wouldn't it?
I'm afraid the confusion stemming from that would outweigh any simplicity.

I would be in favor of logging a message at startup to the effect of
"this is misconfigured" (as per upthread discussion), although whether
people would see that is uncertain.

I think a user-visible warning at CREATE EXNTENSION would help too.

It would help a bit, but actually logging it would probably help more.
Most people don't run the CREATE EXTENSION commands manually, it's all
done as part of either system install scripts or of application
migrations.

But that doesn't mean it wouldn't be useful to do it for those that
*do* run things manually, it just wouldn't be sufficient in itself.

--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/