Обсуждение: shared_preload_libraries = 'pg_stat_statements' failing with installcheck (compute_query_id = auto)

Hi all,
(Added Bruce and Julien in CC)

While testing installcheck with various server configurations to see
how the main regression test suites could break, I found that loading
pg_stat_statements into the backend is enough to break installcheck
as compute_query_id = auto, the default, lets the decision to compute
query IDs to pg_stat_statements itself.  In short, loading
pg_stat_statements breaks EXPLAIN outputs of any SQL-based regression
test.

Running installcheck on existing installations is a popular sanity
check, as much as is enabling pg_stat_statements by default, so it
seems to me that we'd better disable compute_query_id by default in
the databases created for the sake of the regression tests, enabling
it only in places where it is relevant.  We do that in explain.sql for
a test with compute_query_id, but pg_stat_statements does not do
that.

I'd like to suggest a fix for that, by tweaking the tests of
pg_stat_statements to use compute_query_id = auto, so as we would
still stress the code paths where the module takes the decision to
compute query IDs, while the default regression databases would
disable it.  Please note that this also fixes the case of any
out-of-core modules that have EXPLAIN cases.

The attached is enough to pass installcheck-world, on an instance
where pg_stat_statements is loaded.

Thoughts?
--
Michael

Hi,

On Tue, Feb 08, 2022 at 12:38:46PM +0900, Michael Paquier wrote:
> 
> While testing installcheck with various server configurations to see
> how the main regression test suites could break, I found that loading
> pg_stat_statements into the backend is enough to break installcheck
> as compute_query_id = auto, the default, lets the decision to compute
> query IDs to pg_stat_statements itself.  In short, loading
> pg_stat_statements breaks EXPLAIN outputs of any SQL-based regression
> test.

Indeed, that's unfortunate but that's also a known behavior.

> Running installcheck on existing installations is a popular sanity
> check, as much as is enabling pg_stat_statements by default, so it
> seems to me that we'd better disable compute_query_id by default in
> the databases created for the sake of the regression tests, enabling
> it only in places where it is relevant.  We do that in explain.sql for
> a test with compute_query_id, but pg_stat_statements does not do
> that.

I agree that enabling pg_stat_statements is something common when you're
actually going to use your instance, I'm not sure that it also applies to
environment for running regression tests.

> I'd like to suggest a fix for that, by tweaking the tests of
> pg_stat_statements to use compute_query_id = auto, so as we would
> still stress the code paths where the module takes the decision to
> compute query IDs, while the default regression databases would
> disable it.  Please note that this also fixes the case of any
> out-of-core modules that have EXPLAIN cases.

That's already been discussed in [1] and rejected, as it would also mean losing
the ability to have pg_stat_statements (or any similar extension) coverage
using the regression tests.  I personally rely on regression tests for such
custom extensions quite a lot, so I'm still -1 on that.

[1] https://www.postgresql.org/message-id/flat/1634283396.372373993%40f75.i.mail.ru

On Tue, Feb 08, 2022 at 11:48:15AM +0800, Julien Rouhaud wrote:
> That's already been discussed in [1] and rejected, as it would also mean losing
> the ability to have pg_stat_statements (or any similar extension) coverage
> using the regression tests.  I personally rely on regression tests for such
> custom extensions quite a lot, so I'm still -1 on that.

Well, I can see that this is a second independent complain after a few
months.  If you wish to keep this capability, wouldn't it be better to
add a "regress" mode to compute_query_id, where we would mask
automatically this information in the output of EXPLAIN but still run
the computation?
--
Michael

On Tue, Feb 08, 2022 at 12:56:59PM +0900, Michael Paquier wrote:
> On Tue, Feb 08, 2022 at 11:48:15AM +0800, Julien Rouhaud wrote:
> > That's already been discussed in [1] and rejected, as it would also mean losing
> > the ability to have pg_stat_statements (or any similar extension) coverage
> > using the regression tests.  I personally rely on regression tests for such
> > custom extensions quite a lot, so I'm still -1 on that.
> 
> Well, I can see that this is a second independent complain after a few
> months.

> If you wish to keep this capability, wouldn't it be better to
> add a "regress" mode to compute_query_id, where we would mask
> automatically this information in the output of EXPLAIN but still run
> the computation?

Did you just realize you had some script broken because of that (or have an
actual need) or did you only try to see what setting breaks the regression
tests?  If the latter, it seems that the complaint seems a bit artificial.

No one complained about it since, and I'm assuming that pgpro could easily fix
their test workflow since they didn't try to do add such a new mode.

I suggested something like that in  the thread but Tom didn't seemed
interested.  Note that it would be much cleaner to do now that we rely on an
internal query_id_enabled variable rather than changing compute_query_id value,
but I don't know if it's really worth the effort given the number of things
that already breaks the regression tests.

On Tue, Feb 08, 2022 at 12:56:59PM +0900, Michael Paquier wrote:
> Well, I can see that this is a second independent complain after a few
> months.  If you wish to keep this capability, wouldn't it be better to
> add a "regress" mode to compute_query_id, where we would mask
> automatically this information in the output of EXPLAIN but still run
> the computation?

So, I have been looking at this problem, and I don't see a problem in
doing something like the attached, where we add a "regress" mode to
compute_query_id that is a synonym of "auto". Or, in short, we have
the default of letting a module decide if a query ID can be computed
or not, at the exception that we hide its result in EXPLAIN outputs.

Julien, what do you think?

FWIW, about your question of upthread, I have noticed the behavior
while testing, but I know of some internal customers that enable
pg_stat_statements and like doing tests on the PostgreSQL instance
deployed this way, so that would break.  They are not on 14 yet as far
as I know.
--
Michael

Hi,

On Fri, Feb 18, 2022 at 05:22:36PM +0900, Michael Paquier wrote:
>
> So, I have been looking at this problem, and I don't see a problem in
> doing something like the attached, where we add a "regress" mode to
> compute_query_id that is a synonym of "auto". Or, in short, we have
> the default of letting a module decide if a query ID can be computed
> or not, at the exception that we hide its result in EXPLAIN outputs.
>
> Julien, what do you think?

I don't see any problem with that patch.
>
> FWIW, about your question of upthread, I have noticed the behavior
> while testing, but I know of some internal customers that enable
> pg_stat_statements and like doing tests on the PostgreSQL instance
> deployed this way, so that would break.  They are not on 14 yet as far
> as I know.

Are they really testing EXPLAIN output, or just doing application-level tests?

On Fri, Feb 18, 2022 at 05:38:56PM +0800, Julien Rouhaud wrote:
> On Fri, Feb 18, 2022 at 05:22:36PM +0900, Michael Paquier wrote:
>> So, I have been looking at this problem, and I don't see a problem in
>> doing something like the attached, where we add a "regress" mode to
>> compute_query_id that is a synonym of "auto". Or, in short, we have
>> the default of letting a module decide if a query ID can be computed
>> or not, at the exception that we hide its result in EXPLAIN outputs.
>>
>> Julien, what do you think?
>
> I don't see any problem with that patch.

Thanks for looking at it.  An advantage of this version is that it is
backpatchable as the option enum won't break any ABI compatibility, so
that's a win-win.

>> FWIW, about your question of upthread, I have noticed the behavior
>> while testing, but I know of some internal customers that enable
>> pg_stat_statements and like doing tests on the PostgreSQL instance
>> deployed this way, so that would break.  They are not on 14 yet as far
>> as I know.
>
> Are they really testing EXPLAIN output, or just doing application-level tests?

With the various internal customers, both.  And installcheck implies
EXPLAIN outputs.

First, let's wait a couple of days and see if anyone has an extra
opinion to offer on this topic.
--
Michael

On Fri, Feb 18, 2022 at 05:38:56PM +0800, Julien Rouhaud wrote:
> On Fri, Feb 18, 2022 at 05:22:36PM +0900, Michael Paquier wrote:
>> So, I have been looking at this problem, and I don't see a problem in
>> doing something like the attached, where we add a "regress" mode to
>> compute_query_id that is a synonym of "auto". Or, in short, we have
>> the default of letting a module decide if a query ID can be computed
>> or not, at the exception that we hide its result in EXPLAIN outputs.
>>
>> Julien, what do you think?
>
> I don't see any problem with that patch.

Okay, thanks.  I have worked on that today and applied the patch down
to 14, but without the change to enforce the parameter in the
databases created by pg_regress.  Perhaps we should do that in the
long-term, but it is also possible to pass down the option to
PGOPTIONS via command line as compute_query_id is a SUSET or just set
it in postgresql.conf, and being able to do so is enough to address
all the cases I have seen and/or could think about.
--
Michael

On Tue, Feb 22, 2022 at 11:04:16AM +0900, Michael Paquier wrote:
> On Fri, Feb 18, 2022 at 05:38:56PM +0800, Julien Rouhaud wrote:
> > On Fri, Feb 18, 2022 at 05:22:36PM +0900, Michael Paquier wrote:
> >> So, I have been looking at this problem, and I don't see a problem in
> >> doing something like the attached, where we add a "regress" mode to
> >> compute_query_id that is a synonym of "auto". Or, in short, we have
> >> the default of letting a module decide if a query ID can be computed
> >> or not, at the exception that we hide its result in EXPLAIN outputs.

> Okay, thanks.  I have worked on that today and applied the patch down

While rebasing, I noticed that this patch does part of what I added in another
thread.

https://commitfest.postgresql.org/37/3409/
| explain_regress, explain(MACHINE), and default to explain(BUFFERS)

-       if (es->verbose && plannedstmt->queryId != UINT64CONST(0))
+       if (es->verbose && plannedstmt->queryId != UINT64CONST(0) && es->machine)
        {
                /*
                 * Output the queryid as an int64 rather than a uint64 so we match

Apparently, I first wrote this two years ago today.

-- 
Justin