Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 13 Feb 2023 14:00:36 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Force testing of query jumbling code in TAP tests
Message-ID: <Y+nD9LN70w+8eaG9@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="4+xIiclTGy//J/Nc"
Content-Disposition: inline
Archived-At: <https://www.postgresql.org/message-id/Y%2BnD9LN70w%2B8eaG9%40paquier.xyz>
Precedence: bulk


--4+xIiclTGy//J/Nc
Content-Type: multipart/mixed; boundary="GKRRXC0VA3EoySCh"
Content-Disposition: inline


--GKRRXC0VA3EoySCh
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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/ig6@paquier.xyz

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

--GKRRXC0VA3EoySCh
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="query_jumble_test.patch"
Content-Transfer-Encoding: quoted-printable

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 @@
 #
 #-------------------------------------------------------------------------
=20
-EXTRA_INSTALL=3Dcontrib/test_decoding contrib/pg_prewarm
+EXTRA_INSTALL=3Dcontrib/pg_prewarm contrib/pg_stat_statements contrib/test=
_decoding
=20
 subdir =3D src/test/recovery
 top_builddir =3D ../../..
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 =3D> 1);
 $node_primary->adjust_conf('postgresql.conf', 'max_connections', '25');
 $node_primary->append_conf('postgresql.conf',
 	'max_prepared_transactions =3D 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 =3D 'pg_stat_statements'
+pg_stat_statements.max =3D 50000
+compute_query_id =3D '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.

--GKRRXC0VA3EoySCh--

--4+xIiclTGy//J/Nc
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmPpw/QACgkQnvQgOdby
QH1V2xAAgwgPk9ppkMVSxwbisvhpMcnLnEAi8pYiWJBxauRKVMIgqzkmQdjBot79
8mGXQd7gGSAf0a+0DKTn0UhiyDpXvj5k8YgdVGkSeFDRHr0fLdIk8Px5HEGy7SOg
SnHIHnjuJON/K9MzU9R1WF5SeHEDwX/WVu6o/7lqr1lFgFSAW3WkzoChCsoSfTAW
rRixMOvMgT00dCT4jLnGw50D/StA1sGLAuCy/J17IapKnRdkePOAiWh0RIdjY/mL
1/r/ST0ZELMm4+nW1EDwUQrdTGT5sYDeLd2nNDBFM67u4ZOYl3jwyIuB4bAUrn2D
gT6vM9E3eXhSzAVin4szgngDzacl+9clzJpioibQmwo2W1smME9ot0jjshJ+SLdR
rT9orxi6HrM6WWulxidmeZMb6vRmvKQDNe9USRco2Axzb+VtdxKL9AKAZaUi16GG
Kwxm+YT0pjPNAbAVb/PyElnunzLhOeW/tf6QDVjcBgCYSmKYLz5E0yI/lOjcVN8s
Xr8mH3ego8XJiejCuvnlGyIVL63c9tssZVZkrTwegzJIgirlqDEw29sH7pxoxgji
O9K79Sc3FXYA0F/L/aQtyenof5fjI9xElbVVbfKBMDmLU2odhZdi1s+pGtUTVDXR
1aUFAnfWRYtCbjjL8t/Y2iatrtgvcnH61Pnsu6cx6j5crl9Vl84=
=UNy9
-----END PGP SIGNATURE-----

--4+xIiclTGy//J/Nc--