Fix negative bitmapset member not allowed error in logical replication

This happens when we add a replica identity column on a subscriber
that does not yet exist on the publisher, according to the mapping
maintained by the subscriber.  Code that checks whether the target
relation on the subscriber is updatable would check the replica
identity attribute bitmap with a column number -1, which would result
in an error.  To fix, skip such columns in the bitmap lookup and
consider the relation not updatable.  The result is consistent with
the rule that the replica identity columns on the subscriber must be a
subset of those on the publisher, since if the column doesn't exist on
the publisher, the column set on the subscriber can't be a subset.

Reported-by: Tim Clarke <tim.clarke@minerva.info>
Analyzed-by: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
Discussion: https://www.postgresql.org/message-id/flat/a9139c29-7ddd-973b-aa7f-71fed9c38d75%40minerva.info

diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index 54bcd16ec12ee45f90faac698b2229666e6785e1..aa59c455d4cdb10b1d4b8cd1f8641d29ef10d601 100644 (file)
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -339,7 +339,8 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode)
 
                        attnum = AttrNumberGetAttrOffset(attnum);
 
-                       if (!bms_is_member(entry->attrmap[attnum], remoterel->attkeys))
+                       if (entry->attrmap[attnum] < 0 ||
+                               !bms_is_member(entry->attrmap[attnum], remoterel->attkeys))
                        {
                                entry->updatable = false;
                                break;

diff --git a/src/test/subscription/t/008_diff_schema.pl b/src/test/subscription/t/008_diff_schema.pl
index cca4480f52ea123a9c53bd8dcac9057129c6f655..2c94d4dfe4324851e4946b9a6128d770ec83a82a 100644 (file)
--- a/src/test/subscription/t/008_diff_schema.pl
+++ b/src/test/subscription/t/008_diff_schema.pl
@@ -3,7 +3,7 @@ use strict;
 use warnings;
 use PostgresNode;
 use TestLib;
-use Test::More tests => 4;
+use Test::More tests => 5;
 
 sub wait_for_caught_up
 {
@@ -35,7 +35,7 @@ $node_subscriber->safe_psql('postgres', "CREATE TABLE test_tab (a int primary ke
 
 # Setup logical replication
 my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
-$node_publisher->safe_psql('postgres', "CREATE PUBLICATION tap_pub FOR TABLE test_tab");
+$node_publisher->safe_psql('postgres', "CREATE PUBLICATION tap_pub FOR ALL TABLES");
 
 my $appname = 'tap_sub';
 $node_subscriber->safe_psql('postgres',
@@ -86,5 +86,38 @@ $result =
   $node_subscriber->safe_psql('postgres', "SELECT count(*), count(c), count(d = 999), count(e) FROM test_tab");
 is($result, qq(3|3|3|3), 'check extra columns contain local defaults after apply');
 
+
+# Check a bug about adding a replica identity column on the subscriber
+# that was not yet mapped to a column on the publisher.  This would
+# result in errors on the subscriber and replication thus not
+# progressing.
+# (https://www.postgresql.org/message-id/flat/a9139c29-7ddd-973b-aa7f-71fed9c38d75%40minerva.info)
+
+$node_publisher->safe_psql('postgres',
+       "CREATE TABLE test_tab2 (a int)");
+
+$node_subscriber->safe_psql('postgres',
+       "CREATE TABLE test_tab2 (a int)");
+
+$node_subscriber->safe_psql('postgres',
+       "ALTER SUBSCRIPTION tap_sub REFRESH PUBLICATION");
+
+# Add replica identity column.  (The serial is not necessary, but it's
+# a convenient way to get a default on the new column so that rows
+# from the publisher that don't have the column yet can be inserted.)
+$node_subscriber->safe_psql('postgres',
+       "ALTER TABLE test_tab2 ADD COLUMN b serial PRIMARY KEY");
+
+$node_publisher->safe_psql('postgres',
+       "INSERT INTO test_tab2 VALUES (1)");
+
+wait_for_caught_up($node_publisher, $appname);
+
+is($node_subscriber->safe_psql('postgres',
+                                                          "SELECT count(*), min(a), max(a) FROM test_tab2"),
+   qq(1|1|1),
+   'check replicated inserts on subscriber');
+
+
 $node_subscriber->stop;
 $node_publisher->stop;