Handle concurrent sequence drops during synchronization

Commit d4a657b0a4d added a call to has_sequence_privilege() while
fetching sequence information from the publisher, so that
publisher-side permission failures could be distinguished from missing
sequences. It also assumed that has_sequence_privilege() could never
return NULL, and asserted accordingly.

However, that assumption was incorrect. If a sequence is dropped after
the synchronization worker collects its metadata but while fetching the
sequence information, has_sequence_privilege() can return NULL.
This can trigger the assertion failure. This was also reported in
a buildfarm failure on member culicidae.

Fix this by treating a NULL result from has_sequence_privilege() as
indicating that the sequence was dropped concurrently, and report it as
a missing sequence instead of asserting that the result is never NULL.

Reported-by: Noah Misch <noah@leadboat.com>
Author: Vignesh C <vignesh21@gmail.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/20260710045217.f0.noahmisch@microsoft.com
Discussion: https://postgr.es/m/CALDaNm2fHGLeiQKj0r6OG7N9QeayxSmpLrWYJRyt4dL_m3VRWw@mail.gmail.com
Backpatch-through: 19

diff --git a/src/backend/replication/logical/sequencesync.c b/src/backend/replication/logical/sequencesync.c
index 770fa5de10b8305a0f64a0344172ecb6ae1ab32d..63ad46d7fd7b2a9f76eb294f48988afba47a9c53 100644 (file)
--- a/src/backend/replication/logical/sequencesync.c
+++ b/src/backend/replication/logical/sequencesync.c
@@ -289,14 +289,23 @@ get_and_validate_seq_info(TupleTableSlot *slot, Relation *sequence_rel,
        *seqinfo = seqinfo_local =
                (LogicalRepSequenceInfo *) list_nth(seqinfos, *seqidx);
 
+       /*
+        * has_sequence_privilege() itself returns NULL, rather than false, when
+        * the sequence has been dropped concurrently after it was identified in
+        * the catalog snapshot (see has_sequence_privilege_id()). Treat that as a
+        * missing sequence on the publisher.
+        */
+       datum = slot_getattr(slot, ++col, &isnull);
+       if (isnull)
+               return COPYSEQ_SKIPPED;
+
+       remote_has_select_priv = DatumGetBool(datum);
+
        /*
         * The remote sequence state can be NULL if the publisher lacks the
         * required privileges or if the sequence was dropped concurrently after
         * it was identified in the catalog snapshot (see pg_get_sequence_data()).
         */
-       remote_has_select_priv = DatumGetBool(slot_getattr(slot, ++col, &isnull));
-       Assert(!isnull);
-
        datum = slot_getattr(slot, ++col, &isnull);
        if (isnull)
                return remote_has_select_priv ? COPYSEQ_SKIPPED :

diff --git a/src/test/subscription/t/036_sequences.pl b/src/test/subscription/t/036_sequences.pl
index 8b02b24a7e90cd23d3fdfc1571b084bebd96fe6b..b3b3b20f82be547d8905fa3e12f68416c06ee3d6 100644 (file)
--- a/src/test/subscription/t/036_sequences.pl
+++ b/src/test/subscription/t/036_sequences.pl
@@ -188,6 +188,60 @@ is($result, '1|f',
        'REFRESH PUBLICATION will not sync newly published sequence with copy_data as false'
 );
 
+##########
+# A sequence dropped concurrently on the publisher, while the sequencesync
+# worker's batch query is executing, must be treated the same as any other
+# concurrently-dropped sequence (reported as "missing sequence on publisher").
+##########
+
+my $log_offset = -s $node_subscriber->logfile;
+
+# Block the sequencesync worker's batch query on the publisher: an
+# uncommitted DROP SEQUENCE holds AccessExclusiveLock, on which the
+# pg_get_sequence_data() call in the batch query will wait.
+my $pub_session = $node_publisher->background_psql('postgres');
+$pub_session->query_safe(
+       qq(
+       BEGIN;
+       DROP SEQUENCE regress_s3;
+));
+
+$node_subscriber->safe_psql('postgres',
+       "ALTER SUBSCRIPTION regress_seq_sub REFRESH SEQUENCES");
+
+# Wait until the worker's batch query is blocked on the still uncommitted
+# DROP.
+$node_publisher->poll_query_until(
+       'postgres', qq(
+       SELECT EXISTS (
+               SELECT 1 FROM pg_locks
+               WHERE relation = 'regress_s3'::regclass
+                 AND mode = 'AccessShareLock'
+                 AND NOT granted);
+)) or die "timed out waiting for sequencesync worker to block on publisher";
+
+# Commit the DROP while the batch query is blocked inside it, so the query
+# resumes against a sequence that no longer exists.
+# After pg_get_sequence_data() is unblocked, the batch query evaluates
+# has_sequence_privilege(c.oid, 'SELECT') in the target list. Since the DROP
+# has been committed by then, has_sequence_privilege() observes the missing
+# sequence and returns NULL.
+$pub_session->query_safe("COMMIT");
+$pub_session->quit;
+
+$node_subscriber->wait_for_log(
+       qr/WARNING: ( [A-Z0-9]+:)? missing sequence on publisher \("public.regress_s3"\)/,
+       $log_offset);
+
+$node_publisher->safe_psql(
+       'postgres', qq(
+       CREATE SEQUENCE regress_s3;
+));
+
+# Wait for the recreated sequence to be synced.
+$node_subscriber->poll_query_until('postgres', $synced_query)
+  or die "Timed out while waiting for subscriber to synchronize data";
+
 ##########
 # ALTER SUBSCRIPTION ... REFRESH PUBLICATION should report an error when:
 # a) sequence definitions differ between the publisher and subscriber, or
@@ -206,7 +260,7 @@ $node_subscriber->safe_psql(
        CREATE SEQUENCE regress_s4 START 10 INCREMENT 2;
 ));
 
-my $log_offset = -s $node_subscriber->logfile;
+$log_offset = -s $node_subscriber->logfile;
 
 # Do ALTER SUBSCRIPTION ... REFRESH PUBLICATION
 $node_subscriber->safe_psql('postgres',