Fix double table_close of sequence_rel in copy_sequences().

sequence_rel was declared at batch scope, so when a row is skipped due to
concurrent drop or insufficient privileges, the end-of-row cleanup closes
the stale pointer from the previous row, tripping the relcache refcount
assertion.

Move sequence_rel inside the per-row loop.

Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: vignesh C <vignesh21@gmail.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Discussion: https://postgr.es/m/CAJTYsWWOuw-yfmzotV4jCJ6LLxEsb=STLcGtDYXOxRcU9Te3Pw@mail.gmail.com

diff --git a/src/backend/replication/logical/sequencesync.c b/src/backend/replication/logical/sequencesync.c
index ec7e76abf934fb0e1b5b61231dca5980ab38a45b..e2ff8d77b16e56c5bfbf779d3bbc03bae2ce1380 100644 (file)
--- a/src/backend/replication/logical/sequencesync.c
+++ b/src/backend/replication/logical/sequencesync.c
@@ -254,8 +254,8 @@ get_and_validate_seq_info(TupleTableSlot *slot, Relation *sequence_rel,
                (LogicalRepSequenceInfo *) list_nth(seqinfos, *seqidx);
 
        /*
-        * last_value can be NULL if the sequence was dropped concurrently (see
-        * pg_get_sequence_data()).
+        * The sequence data can be NULL due to insufficient privileges or if the
+        * sequence was dropped concurrently (see pg_get_sequence_data()).
         */
        datum = slot_getattr(slot, ++col, &isnull);
        if (isnull)
@@ -411,7 +411,6 @@ copy_sequences(WalReceiverConn *conn)
                int                     batch_skipped_count = 0;
                int                     batch_insuffperm_count = 0;
                int                     batch_missing_count;
-               Relation        sequence_rel = NULL;
 
                WalRcvExecResult *res;
                TupleTableSlot *slot;
@@ -494,6 +493,7 @@ copy_sequences(WalReceiverConn *conn)
                {
                        CopySeqResult sync_status;
                        LogicalRepSequenceInfo *seqinfo;
+                       Relation        sequence_rel = NULL;
                        int                     seqidx;
 
                        CHECK_FOR_INTERRUPTS();

diff --git a/src/test/subscription/t/036_sequences.pl b/src/test/subscription/t/036_sequences.pl
index 471780a35858212c9318a2b34f28f3347fde89e5..e6dd3b069b8cbe1b4af1e65511ae56159a819911 100644 (file)
--- a/src/test/subscription/t/036_sequences.pl
+++ b/src/test/subscription/t/036_sequences.pl
@@ -221,4 +221,43 @@ $node_subscriber->wait_for_log(
        qr/WARNING: ( [A-Z0-9]+:)? missing sequence on publisher \("public.regress_s4"\)/,
        $log_offset);
 
+# Recreate regress_s4 so later tests that reuse the subscription do not keep
+# reporting the intentionally-missing sequence from the previous test.
+$node_publisher->safe_psql(
+       'postgres', qq(
+       CREATE SEQUENCE regress_s4 START 10 INCREMENT 2;
+));
+
+##########
+# Ensure that insufficient privileges on the publisher for a sequence do not
+# disrupt the subscriber. The subscriber should log a warning and continue
+# retrying.
+##########
+
+$node_publisher->safe_psql(
+       'postgres', qq(
+       CREATE ROLE regress_seq_repl LOGIN REPLICATION;
+       GRANT USAGE ON SCHEMA public TO regress_seq_repl;
+       GRANT SELECT ON ALL SEQUENCES IN SCHEMA public TO regress_seq_repl;
+       REVOKE ALL ON SEQUENCE regress_s2 FROM regress_seq_repl;
+));
+
+my $publisher_limited_connstr =
+  $node_publisher->connstr . ' dbname=postgres user=regress_seq_repl';
+$log_offset = -s $node_subscriber->logfile;
+
+$node_subscriber->safe_psql(
+       'postgres',
+       "ALTER SUBSCRIPTION regress_seq_sub CONNECTION '$publisher_limited_connstr'"
+);
+
+$node_subscriber->safe_psql(
+       'postgres',
+       "ALTER SUBSCRIPTION regress_seq_sub REFRESH SEQUENCES"
+);
+
+$node_subscriber->wait_for_log(
+       qr/WARNING: ( [A-Z0-9]+:)? missing sequence on publisher \("public.regress_s2"\)/,
+       $log_offset);
+
 done_testing();