Re: shared_preload_libraries = 'pg_stat_statements' failing with installcheck (compute_query_id = auto)
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
Re: shared_preload_libraries = 'pg_stat_statements' failing with installcheck (compute_query_id = auto)
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 signature.asc Description: PGP signature
Re: shared_preload_libraries = 'pg_stat_statements' failing with installcheck (compute_query_id = auto)
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 signature.asc Description: PGP signature
Re: shared_preload_libraries = 'pg_stat_statements' failing with installcheck (compute_query_id = auto)
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?
Re: shared_preload_libraries = 'pg_stat_statements' failing with installcheck (compute_query_id = auto)
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 diff --git a/src/include/utils/queryjumble.h b/src/include/utils/queryjumble.h index a4c277269e..c670662db2 100644 --- a/src/include/utils/queryjumble.h +++ b/src/include/utils/queryjumble.h @@ -57,7 +57,8 @@ enum ComputeQueryIdType { COMPUTE_QUERY_ID_OFF, COMPUTE_QUERY_ID_ON, - COMPUTE_QUERY_ID_AUTO + COMPUTE_QUERY_ID_AUTO, + COMPUTE_QUERY_ID_REGRESS }; /* GUC parameters */ diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c index b970997c34..fae3926b42 100644 --- a/src/backend/commands/explain.c +++ b/src/backend/commands/explain.c @@ -604,7 +604,13 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es, /* Create textual dump of plan tree */ ExplainPrintPlan(es, queryDesc); - if (es->verbose && plannedstmt->queryId != UINT64CONST(0)) + /* + * COMPUTE_QUERY_ID_REGRESS means COMPUTE_QUERY_ID_AUTO, but we don't + * show it up in any of the EXPLAIN plans to keep stable the results + * generated by any regression test suite. + */ + if (es->verbose && plannedstmt->queryId != UINT64CONST(0) && + compute_query_id != COMPUTE_QUERY_ID_REGRESS) { /* * Output the queryid as an int64 rather than a uint64 so we match diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 01f373815e..7438df9109 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -412,6 +412,7 @@ static const struct config_enum_entry backslash_quote_options[] = { */ static const struct config_enum_entry compute_query_id_options[] = { {"auto", COMPUTE_QUERY_ID_AUTO, false}, + {"regress", COMPUTE_QUERY_ID_REGRESS, false}, {"on", COMPUTE_QUERY_ID_ON, false}, {"off", COMPUTE_QUERY_ID_OFF, false}, {"true", COMPUTE_QUERY_ID_ON, true}, diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c index e6f71c7582..a4612d9a8a 100644 --- a/src/test/regress/pg_regress.c +++ b/src/test/regress/pg_regress.c @@ -1925,8 +1925,9 @@ create_database(const char *dbname) "ALTER DATABASE \"%s\" SET lc_numeric TO 'C';" "ALTER DATABASE \"%s\" SET lc_time TO 'C';" "ALTER DATABASE \"%s\" SET bytea_output TO 'hex';" + "ALTER DATABASE \"%s\" SET compute_query_id TO 'regress';" "ALTER DATABASE \"%s\" SET timezone_abbreviations TO 'Default';", - dbname, dbname, dbname, dbname, dbname, dbname); + dbname, dbname, dbname, dbname, dbname, dbname, dbname); psql_end_command(buf, "postgres"); /* diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index d99bf38e67..036e6da680 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -7934,9 +7934,11 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv; method is not acceptable. In this case, in-core computation must be always disabled. Valid values are off (always disabled), -on (always enabled) and auto, +on (always enabled), auto, which lets modules such as -automatically enable it. +automatically enable it, and regress which +has the same effect as auto, except that the +query identifier is hidden in the EXPLAIN output. The default is auto. signature.asc Description: PGP signature
Re: shared_preload_libraries = 'pg_stat_statements' failing with installcheck (compute_query_id = auto)
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.
Re: shared_preload_libraries = 'pg_stat_statements' failing with installcheck (compute_query_id = auto)
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 signature.asc Description: PGP signature
Re: shared_preload_libraries = 'pg_stat_statements' failing with installcheck (compute_query_id = auto)
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
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 diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c index e6f71c7582..f305a4e57a 100644 --- a/src/test/regress/pg_regress.c +++ b/src/test/regress/pg_regress.c @@ -1925,8 +1925,9 @@ create_database(const char *dbname) "ALTER DATABASE \"%s\" SET lc_numeric TO 'C';" "ALTER DATABASE \"%s\" SET lc_time TO 'C';" "ALTER DATABASE \"%s\" SET bytea_output TO 'hex';" + "ALTER DATABASE \"%s\" SET compute_query_id TO 'off';" "ALTER DATABASE \"%s\" SET timezone_abbreviations TO 'Default';", - dbname, dbname, dbname, dbname, dbname, dbname); + dbname, dbname, dbname, dbname, dbname, dbname, dbname); psql_end_command(buf, "postgres"); /* diff --git a/contrib/pg_stat_statements/expected/oldextversions.out b/contrib/pg_stat_statements/expected/oldextversions.out index f18c08838f..ff20286db7 100644 --- a/contrib/pg_stat_statements/expected/oldextversions.out +++ b/contrib/pg_stat_statements/expected/oldextversions.out @@ -1,4 +1,5 @@ -- test old extension version entry points +SET compute_query_id = auto; CREATE EXTENSION pg_stat_statements WITH VERSION '1.4'; -- Execution of pg_stat_statements_reset() is granted only to -- superusers in 1.4, so this fails. diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out index e0abe34bb6..e8d22f791b 100644 --- a/contrib/pg_stat_statements/expected/pg_stat_statements.out +++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out @@ -1,4 +1,5 @@ CREATE EXTENSION pg_stat_statements; +SET compute_query_id = auto; -- -- simple and compound statements -- diff --git a/contrib/pg_stat_statements/sql/oldextversions.sql b/contrib/pg_stat_statements/sql/oldextversions.sql index f2e822acd3..773e5f55f9 100644 --- a/contrib/pg_stat_statements/sql/oldextversions.sql +++ b/contrib/pg_stat_statements/sql/oldextversions.sql @@ -1,4 +1,5 @@ -- test old extension version entry points +SET compute_query_id = auto; CREATE EXTENSION pg_stat_statements WITH VERSION '1.4'; -- Execution of pg_stat_statements_reset() is granted only to diff --git a/contrib/pg_stat_statements/sql/pg_stat_statements.sql b/contrib/pg_stat_statements/sql/pg_stat_statements.sql index dffd2c8c18..74ced1a7f4 100644 --- a/contrib/pg_stat_statements/sql/pg_stat_statements.sql +++ b/contrib/pg_stat_statements/sql/pg_stat_statements.sql @@ -1,4 +1,5 @@ CREATE EXTENSION pg_stat_statements; +SET compute_query_id = auto; -- -- simple and compound statements signature.asc Description: PGP signature