From: Mark Dilger <mark.dilger@enterprisedb.com>
Message-Id: <CD02D159-7260-4247-888F-2CFA7AD3798D@enterprisedb.com>
Content-Type: multipart/mixed;
	boundary="Apple-Mail=_8151DED1-3EF4-49C6-A52A-0F73A5CF5A9F"
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.7\))
Subject: Re: Non-superuser subscription owners
Date: Mon, 1 Nov 2021 10:58:25 -0700
In-Reply-To: <c1c334a2-5d85-687f-f0dc-0ac8c422ca1a@dunslane.net>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
To: Andrew Dunstan <andrew@dunslane.net>
References: <9DFC88D3-1300-4DE8-ACBC-4CEF84399A53@enterprisedb.com>
 <c1c334a2-5d85-687f-f0dc-0ac8c422ca1a@dunslane.net>
Archived-At: 
 <https://www.postgresql.org/message-id/CD02D159-7260-4247-888F-2CFA7AD3798D%40enterprisedb.com>
Precedence: bulk


--Apple-Mail=_8151DED1-3EF4-49C6-A52A-0F73A5CF5A9F
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii


> On Nov 1, 2021, at 7:18 AM, Andrew Dunstan <andrew@dunslane.net> =
wrote:
>=20
> w.r.t. this:
>=20
> +   On the subscriber, the subscription owner's privileges are
> re-checked for
> +   each change record when applied, but beware that a change of
> ownership for a
> +   subscription may not be noticed immediately by the replication =
workers.
> +   Changes made on the publisher may be applied on the subscriber as
> +   the old owner.  In such cases, the old owner's privileges will be
> the ones
> +   that matter.  Worse still, it may be hard to predict when =
replication
> +   workers will notice the new ownership.  Subscriptions created
> disabled and
> +   only enabled after ownership has been changed will not be subject =
to
> this
> +   race condition.
>=20
>=20
> maybe we should disable the subscription before making such a change =
and
> then re-enable it?

Right.  I commented the code that way because there is a clear concern, =
but I was uncertain which way around the problem was best.

ALTER SUBSCRIPTION..[ENABLE | DISABLE] do not synchronously start or =
stop subscription workers.  The ALTER command updates the catalog's =
subenabled field, but workers only lazily respond to that.  Disabling =
and enabling the subscription as part of the OWNER TO would not reliably =
accomplish anything.

The attached patch demonstrates the race condition.  It sets up a =
publisher and subscriber, and toggles the subscription on and off on the =
subscriber node, interleaved with inserts and deletes on the publisher =
node.  If the ALTER SUBSCRIPTION commands were synchronous, the test =
results would be deterministic, with only the inserts performed while =
the subscription is enabled being replicated, but because the ALTER =
commands are asynchronous, the results are nondeterministic.

It is unclear that I can make ALTER SUBSCRIPTION..OWNER TO synchronous =
without redesigning the way workers respond to pg_subscription catalog =
updates generally.  That may be a good project to eventually tackle, but =
I don't see that it is more important to close the race condition in an =
OWNER TO than for a DISABLE.

Thoughts?


--Apple-Mail=_8151DED1-3EF4-49C6-A52A-0F73A5CF5A9F
Content-Disposition: attachment;
	filename=alter_subscription_race.patch
Content-Type: application/octet-stream;
	x-unix-mode=0644;
	name="alter_subscription_race.patch"
Content-Transfer-Encoding: 7bit

diff --git a/src/test/subscription/t/026_race.pl b/src/test/subscription/t/026_race.pl
new file mode 100644
index 0000000000..cbbe6c34e2
--- /dev/null
+++ b/src/test/subscription/t/026_race.pl
@@ -0,0 +1,86 @@
+
+# Copyright (c) 2021, PostgreSQL Global Development Group
+
+# Test subscriptions owned by non-superusers
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More;
+
+our $LOOPCOUNT = 100;
+plan tests => 2 * $LOOPCOUNT;
+my $result;
+
+# Setup
+
+my $node_publisher = PostgresNode->new('publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->start;
+
+my $node_subscriber = PostgresNode->new('subscriber');
+$node_subscriber->init(allows_streaming => 'logical');
+$node_subscriber->start;
+
+my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+
+$node_publisher->safe_psql('postgres', qq(
+CREATE TABLE t (i INTEGER);
+ALTER TABLE t REPLICA IDENTITY FULL;
+CREATE PUBLICATION pub FOR TABLE t;
+));
+
+$node_subscriber->safe_psql('postgres', qq(
+CREATE TABLE t (i INTEGER);
+CREATE SUBSCRIPTION sub CONNECTION '$publisher_connstr' PUBLICATION pub;
+CREATE TABLE b (LIKE t);
+));
+
+# Wait for initial sync of all subscriptions.
+my $synced_query =
+	"SELECT count(1) = 0 FROM pg_subscription_rel WHERE srsubstate NOT IN ('r', 's');";
+$node_subscriber->poll_query_until('postgres', $synced_query)
+	or die "Timed out while waiting for subscriber to synchronize data";
+
+# Upon first entry to this loop, the subscription is enabled
+foreach my $loop (1..$LOOPCOUNT)
+{
+	my $even = $loop * 2;
+	my $odd = ($loop * 2) + 1;
+
+	# The subscription is enabled.  This even number is fair game for
+	# replication.
+	$node_publisher->safe_psql('postgres', "INSERT INTO t (i) VALUES ($even)");
+
+	# We may replicate the even number before the subscription is disabled, but
+	# maybe not.
+	$node_subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION sub DISABLE");
+
+	# The subscription is now disabled, but workers on the subscriber side may
+	# not have gotten the message yet.  As such, this odd number may slip
+	# through.
+	$node_publisher->safe_psql('postgres', "INSERT INTO t (i) VALUES ($odd)");
+
+	# See if we can catch any odd numbers in the subscriber's copy of table t
+	$result = $node_subscriber->safe_psql('postgres',
+		"SELECT COUNT(*) FROM t WHERE i % 2 = 1");
+	is ($result, 0, "No odd numbers in subscriber copy of table t before delete on publisher");
+
+	# Clear out all values on the publisher side, so that the odd number cannot
+	# be replicated after we re-enable the subscription.  Of course, it may have
+	# already slipped through, and it is also possible that the even number has
+	# not replicated yet.
+	#
+	# Since the publication only publishes INSERTs, this delete should only have
+	# effect on the publisher side.
+	$node_publisher->safe_psql('postgres', "DELETE FROM t WHERE i = $odd");
+
+	# Enable the subscription for the next loop.  Note that at this point, the
+	# table is empty on the publisher side.
+	$node_subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION sub ENABLE");
+
+	# See if we can catch an odd number before the delete is propogated
+	$result = $node_subscriber->safe_psql('postgres',
+		"SELECT COUNT(*) FROM t WHERE i % 2 = 1");
+	is ($result, 0, "No odd numbers in subscriber copy of table t after delete on publisher");
+}

--Apple-Mail=_8151DED1-3EF4-49C6-A52A-0F73A5CF5A9F
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8


=E2=80=94
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


--Apple-Mail=_8151DED1-3EF4-49C6-A52A-0F73A5CF5A9F--