From: Michael Paquier <michael@paquier.xyz>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: cyberdemn@gmail.com
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15734: Walsender process crashing when executing SHOW ALL;
Date: Thu, 11 Apr 2019 12:07:12 +0900
Message-ID: <20190411030712.GH2728@paquier.xyz> (raw)
In-Reply-To: <20190410150121.GA26340@alvherre.pgsql>
References: <20190410060427.GB2728@paquier.xyz>
<20190410150121.GA26340@alvherre.pgsql>
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
Attachments:wal-sender-show-v2.patch (text/x-diff, 3.4K) downloaddiff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.cindex 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 = CreateDestReceiver(DestRemoteSimple);
VariableShowStmt *n = (VariableShowStmt *) cmd_node;
+ /* syscache access needs a transaction environment */+ StartTransactionCommand();
GetPGVariable(n->name, dest);
+ CommitTransactionCommand();
}
break;
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.plindex 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 => 26;+use Test::More tests => 32;
# Initialize master node
my $node_master = 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);
+# 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 = $node_master->host;+my $master_port = $node_master->port;+my $connstr_common = "host=$master_host port=$master_port user=repl_role";+my $connstr_rep = "$connstr_common replication=1";+my $connstr_db = "$connstr_common replication=database dbname=postgres";++# Test SHOW ALL+my ($ret, $stdout, $stderr) =+ $node_master->psql('postgres', 'SHOW ALL;',+ on_error_die => 1,+ extra_params => [ '-d', $connstr_rep ]);+ok($ret == 0, "SHOW ALL with replication role and physical replication");+($ret, $stdout, $stderr) =+ $node_master->psql('postgres', 'SHOW ALL;',+ on_error_die => 1,+ extra_params => [ '-d', $connstr_db ]);+ok($ret == 0, "SHOW ALL with replication role and logical replication");++# Test SHOW with a user-settable parameter+($ret, $stdout, $stderr) =+ $node_master->psql('postgres', 'SHOW work_mem;',+ on_error_die => 1,+ extra_params => [ '-d', $connstr_rep ]);+ok($ret == 0, "SHOW with user-settable parameter, replication role and physical replication");+($ret, $stdout, $stderr) =+ $node_master->psql('postgres', 'SHOW work_mem;',+ on_error_die => 1,+ extra_params => [ '-d', $connstr_db ]);+ok($ret == 0, "SHOW with user-settable parameter, replication role and logical replication");++# Test SHOW with a superuser-settable parameter+($ret, $stdout, $stderr) =+ $node_master->psql('postgres', 'SHOW primary_conninfo;',+ on_error_die => 1,+ extra_params => [ '-d', $connstr_rep ]);+ok($ret == 0, "SHOW with superuser-settable parameter, replication role and physical replication");+($ret, $stdout, $stderr) =+ $node_master->psql('postgres', 'SHOW primary_conninfo;',+ on_error_die => 1,+ extra_params => [ '-d', $connstr_db ]);+ok($ret == 0, "SHOW with superuser-settable parameter, replication role and logical replication");+
note "switching to physical replication slot";
# Switch to using a physical replication slot. We can do this without a new
signature.asc (application/pgp-signature, 832B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: michael@paquier.xyz, alvherre@2ndquadrant.com, cyberdemn@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15734: Walsender process crashing when executing SHOW ALL;
In-Reply-To: <20190411030712.GH2728@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox