Subject: Force testing of query jumbling code in TAP tests

From: Michael Paquier <[email-masked]>
Date: Mon, 13 Feb 2023 14:00:36 +0900

Hi all,

As mentioned two times on this thread, there is not much coverage for
the query jumbling code, even if it is in core:
https://www.postgresql.org/message-id/Y5BHOUhX3zTH/[email-masked]

Ths issue is that we have the options to enable it, but only
pg_stat_statements is able to enable and stress it.  This causes
coverage to be missed for all query patterns that are not covered
directly by pg_stat_statements, like XML expressions, various DML
patterns, etc.  More aggressive testing would also ensure that no
nodes are marked as no_query_jumble while they should be included in a
computation.

Attached is a patch to improve that.  The main regression database is
able to cover everything, basically, so I'd like to propose the
addition of some extra configuration in 027_stream_regress.pl to
enable pg_stat_statements.  This could be added in the pg_upgrade
tests, but that felt a bit less adapted here.  Or can people think
about cases where checking pg_stat_statements makes more sense after
an upgrade or on a standby?  One thing that makes sense for a standby
is to check that the contents of pg_stat_statements are empty?

With this addition, the query jumbling gets covered at 95%~, while
https://coverage.postgresql.org/src/backend/nodes/index.html reports
currently 35%.

Thoughts or comments?
--
Michael
diff --git a/src/test/recovery/Makefile b/src/test/recovery/Makefile
index 570bf42b58..c60314d195 100644
--- a/src/test/recovery/Makefile
+++ b/src/test/recovery/Makefile
@@ -9,7 +9,7 @@
 #
 #-------------------------------------------------------------------------
 
-EXTRA_INSTALL=contrib/test_decoding contrib/pg_prewarm
+EXTRA_INSTALL=contrib/pg_prewarm contrib/pg_stat_statements contrib/test_decoding
 
 subdir = src/test/recovery
 top_builddir = ../../..
diff --git a/src/test/recovery/t/027_stream_regress.pl b/src/test/recovery/t/027_stream_regress.pl
index 69d6ddf281..9fb565f33c 100644
--- a/src/test/recovery/t/027_stream_regress.pl
+++ b/src/test/recovery/t/027_stream_regress.pl
@@ -14,6 +14,16 @@ $node_primary->init(allows_streaming => 1);
 $node_primary->adjust_conf('postgresql.conf', 'max_connections', '25');
 $node_primary->append_conf('postgresql.conf',
 	'max_prepared_transactions = 10');
+
+# Enable pg_stat_statements to force tests with do query jumbling.
+# pg_stat_statements.max should be large enough to hold all the entries
+# of the regression database.
+$node_primary->append_conf('postgresql.conf',
+			   qq{shared_preload_libraries = 'pg_stat_statements'
+pg_stat_statements.max = 50000
+compute_query_id = 'regress'
+});
+
 # We'll stick with Cluster->new's small default shared_buffers, but since that
 # makes synchronized seqscans more probable, it risks changing the results of
 # some test queries.  Disable synchronized seqscans to prevent that.

---

From: Andres Freund <[email-masked]>
Date: Mon, 13 Feb 2023 09:45:12 -0800

Hi,

> https://coverage.postgresql.org/src/backend/nodes/index.html reports

Shouldn't there at least be some basic verification of pg_stat_statements
output being sane after running the test? Even if that's perhaps just actually
printing the statements.

Greetings,

Andres Freund