Date: Thu, 11 Apr 2019 12:07:12 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: cyberdemn@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15734: Walsender process crashing when executing SHOW ALL;
Message-ID: <20190411030712.GH2728@paquier.xyz>
References: <20190410060427.GB2728@paquier.xyz>
 <20190410150121.GA26340@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="mFHiwr52TKrxpkjc"
Content-Disposition: inline
In-Reply-To: <20190410150121.GA26340@alvherre.pgsql>
Precedence: bulk


--mFHiwr52TKrxpkjc
Content-Type: multipart/mixed; boundary="MPkR1dXiUZqK+927"
Content-Disposition: inline


--MPkR1dXiUZqK+927
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Wed, Apr 10, 2019 at 11:01:21AM -0400, Alvaro Herrera wrote:
> I think we should have a few tests issuing SHOW ALL in a replication
> connection with various levels of privilege; it's annoying that this bug
> took two years to find.  With that, special-purpose buildfarm members
> would tell us if we've made some mistake in transaction handling or
> whatever.

What do you think about the set attached?  I prefer that we use
psql(on_error_die => 1) so as in the case of a crash or an error the
test dies hard and fast. In consequence, I have granted
pg_read_all_settings to the replication user to ensure that no errors
happen, still the syscache lookup is done.  We could have negative
tests but I don't think that's worth the addition as we already have
tests for pg_read_all_settings in src/test/regress/.  If I remove the
patched patch from walsender.c, the test fails immediately.
--
Michael

--MPkR1dXiUZqK+927
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="wal-sender-show-v2.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/replication/walsender.c b/src/backend/replication/=
walsender.c
index aae6adc15c..440b6aac4b 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -1552,7 +1552,10 @@ exec_replication_command(const char *cmd_string)
 				DestReceiver *dest =3D CreateDestReceiver(DestRemoteSimple);
 				VariableShowStmt *n =3D (VariableShowStmt *) cmd_node;
=20
+				/* syscache access needs a transaction environment */
+				StartTransactionCommand();
 				GetPGVariable(n->name, dest);
+				CommitTransactionCommand();
 			}
 			break;
=20
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/00=
1_stream_rep.pl
index beb45551a2..e866db3581 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -3,7 +3,7 @@ use strict;
 use warnings;
 use PostgresNode;
 use TestLib;
-use Test::More tests =3D> 26;
+use Test::More tests =3D> 32;
=20
 # Initialize master node
 my $node_master =3D get_new_node('master');
@@ -117,6 +117,55 @@ test_target_session_attrs($node_master, $node_standby_=
1, $node_master, "any",
 test_target_session_attrs($node_standby_1, $node_master, $node_standby_1,
 	"any", 0);
=20
+# Test for SHOW commands using a WAL sender connection with a replication
+# role.
+note "testing SHOW commands for replication connection";
+
+$node_master->psql('postgres',"
+CREATE ROLE repl_role REPLICATION LOGIN;
+GRANT pg_read_all_settings TO repl_role;");
+my $master_host =3D $node_master->host;
+my $master_port =3D $node_master->port;
+my $connstr_common =3D "host=3D$master_host port=3D$master_port user=3Drep=
l_role";
+my $connstr_rep =3D "$connstr_common replication=3D1";
+my $connstr_db =3D "$connstr_common replication=3Ddatabase dbname=3Dpostgr=
es";
+
+# Test SHOW ALL
+my ($ret, $stdout, $stderr) =3D
+    $node_master->psql('postgres', 'SHOW ALL;',
+		       on_error_die =3D> 1,
+		       extra_params =3D> [ '-d', $connstr_rep ]);
+ok($ret =3D=3D 0, "SHOW ALL with replication role and physical replication=
");
+($ret, $stdout, $stderr) =3D
+    $node_master->psql('postgres', 'SHOW ALL;',
+		       on_error_die =3D> 1,
+		       extra_params =3D> [ '-d', $connstr_db ]);
+ok($ret =3D=3D 0, "SHOW ALL with replication role and logical replication"=
);
+
+# Test SHOW with a user-settable parameter
+($ret, $stdout, $stderr) =3D
+    $node_master->psql('postgres', 'SHOW work_mem;',
+		       on_error_die =3D> 1,
+		       extra_params =3D> [ '-d', $connstr_rep ]);
+ok($ret =3D=3D 0, "SHOW with user-settable parameter, replication role and=
 physical replication");
+($ret, $stdout, $stderr) =3D
+    $node_master->psql('postgres', 'SHOW work_mem;',
+		       on_error_die =3D> 1,
+		       extra_params =3D> [ '-d', $connstr_db ]);
+ok($ret =3D=3D 0, "SHOW with user-settable parameter, replication role and=
 logical replication");
+
+# Test SHOW with a superuser-settable parameter
+($ret, $stdout, $stderr) =3D
+    $node_master->psql('postgres', 'SHOW primary_conninfo;',
+		       on_error_die =3D> 1,
+		       extra_params =3D> [ '-d', $connstr_rep ]);
+ok($ret =3D=3D 0, "SHOW with superuser-settable parameter, replication rol=
e and physical replication");
+($ret, $stdout, $stderr) =3D
+    $node_master->psql('postgres', 'SHOW primary_conninfo;',
+		       on_error_die =3D> 1,
+		       extra_params =3D> [ '-d', $connstr_db ]);
+ok($ret =3D=3D 0, "SHOW with superuser-settable parameter, replication rol=
e and logical replication");
+
 note "switching to physical replication slot";
=20
 # Switch to using a physical replication slot. We can do this without a new

--MPkR1dXiUZqK+927--

--mFHiwr52TKrxpkjc
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlyur2AACgkQnvQgOdby
QH0JTA/+KxHeINE2IGHtzGsgFBXQBFEU0ojhwvY2f+/Oe177BewM0lhr27vLzVSu
kYw3d022AByFjydJXY4YvxPVod+IDDafK+uhKQVQbaWdUqrZwlyA0YckSOgMVWF2
HiWGokDfu6icM4SVLpNSjnzN+jw3VOiklWrQG4tRo8ilsZWBYiXWBqlRc5Xm3IbQ
D7mZyw5IPFHhHtLyZtnj2zCvEU6KII3vMMDBi90zCr0gFAP0FRIoz3Wqt87HfOL+
H+FWs5PwuYSac4FwDWaLAsauCBdRUYLlIVjuhnwjUt201GBhQER+Vu0Dxx9L7UFo
N44Wjz7LROYcm3TFht4kcvL9SakkRGVqg8cUbOUwSF1yL5ORhflmXXFU9Sj7yyCp
5USGP3DTzaZFSgoC2E3I19pGimUBkUDnemVai+Cuf/9VidzJKuTfyeIh8LJOT1Aw
b8zMJV+2lpjr410XH/JVR6G9HQ/JA/G5zwEVFu1coYHLgC90YnRPiRARt6fvxo3u
POX/fpPCnc+a3rfGugjf8P6VOpAERNFk1UJaRvXebkVAjLalXWxPLmKi0ZdLO17b
BP1x1t0nyRu1nSDo3tF81o+MivRwxO/+n/tqeLNtvdrAEOBWhnwI+mYYobIxyF32
L5fusibjoOQkfaIvFFO+E4p9VjgQUQ5ctqUo7JHOTCoTpHA64v8=
=cthZ
-----END PGP SIGNATURE-----

--mFHiwr52TKrxpkjc--