On Thu, Nov 11, 2021, at 9:01 AM, houzj.f...@fujitsu.com wrote:
> Also attach the patches for back branch and remove some unnecessary
> changes from pgindent.
I reviewed your patch and I think the fix could be simplified by
if (OidIsValid(indexOid))
CacheInvalidateRelcache(rel);
If indexOid is valid it is a REPLICA IDENTITY INDEX. A few lines above there is
a check for a valid indexOid that makes sure the index is already marked as a
replica identity; if so, it bail out. If it is not, the relation should be
invalidated. Am I missing something?
I also modified your test case to include a DELETE command, wait the initial
table sync to avoid failing a subsequent test and improve some comments.
--
Euler Taveira
EDB https://www.enterprisedb.com/
From e81e0bb6c6796e7fd692b7d6643db8fb6770d9a1 Mon Sep 17 00:00:00 2001
From: "houzj.fnst" <houzj.f...@cn.fujitsu.com>
Date: Thu, 11 Nov 2021 18:21:37 +0800
Subject: [PATCH v3] Invalidate relcache entry when changing REPLICA IDENTITY
INDEX
When changing REPLICA IDENTITY INDEX to another one, the table relcache
entry was not invalidated which cause unexpected behaviour when
replicating UPDATE or DELETE changes.
If we are replacing indexes on the REPLICA IDENTITY, invalidate the
relcache entry to make sure the index Bitmapset is rebuild.
---
src/backend/commands/tablecmds.c | 9 ++++
src/test/subscription/t/100_bugs.pl | 80 ++++++++++++++++++++++++++++-
2 files changed, 88 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 857cc5ce6e..97a05da4b1 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -15492,6 +15492,15 @@ relation_mark_replica_identity(Relation rel, char ri_type, Oid indexOid,
heap_freetuple(pg_index_tuple);
}
+ /*
+ * If the correct index was not marked as replica identity, table relcache
+ * should be invalidated. Hence, all sessions will refresh the table
+ * replica identity before any UPDATE or DELETE on the table that was
+ * invalidated.
+ */
+ if (OidIsValid(indexOid))
+ CacheInvalidateRelcache(rel);
+
table_close(pg_index, RowExclusiveLock);
}
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index 34a60fd9ab..00de20e9b5 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -6,7 +6,7 @@ use strict;
use warnings;
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
-use Test::More tests => 5;
+use Test::More tests => 7;
# Bug #15114
@@ -224,3 +224,81 @@ $node_sub->safe_psql('postgres', "DROP TABLE tab1");
$node_pub->stop('fast');
$node_pub_sub->stop('fast');
$node_sub->stop('fast');
+
+# https://postgr.es/m/OS0PR01MB61133CA11630DAE45BC6AD95FB939%40OS0PR01MB6113.jpnprd01.prod.outlook.com'
+
+# The bug was that when changing the REPLICA IDENTITY INDEX to another one, the
+# target table relcache would not be invalidated. This could cause unexpected
+# behaviour when replicating UPDATE/DELETE change.
+$node_publisher = PostgreSQL::Test::Cluster->new('publisher3');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->start;
+
+$node_subscriber = PostgreSQL::Test::Cluster->new('subscriber3');
+$node_subscriber->init(allows_streaming => 'logical');
+$node_subscriber->start;
+
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE tab_replidentity_index(a int not null, b int not null)");
+$node_publisher->safe_psql('postgres',
+ "CREATE UNIQUE INDEX idx_replidentity_index_a ON tab_replidentity_index(a)"
+);
+$node_publisher->safe_psql('postgres',
+ "CREATE UNIQUE INDEX idx_replidentity_index_b ON tab_replidentity_index(b)"
+);
+
+# use index idx_replidentity_index_a as REPLICA IDENTITY on publisher.
+$node_publisher->safe_psql('postgres',
+ "ALTER TABLE tab_replidentity_index REPLICA IDENTITY USING INDEX idx_replidentity_index_a"
+);
+
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_replidentity_index VALUES(1, 1),(2, 2)");
+
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE tab_replidentity_index(a int not null, b int not null)");
+$node_subscriber->safe_psql('postgres',
+ "CREATE UNIQUE INDEX idx_replidentity_index_a ON tab_replidentity_index(a)"
+);
+$node_subscriber->safe_psql('postgres',
+ "CREATE UNIQUE INDEX idx_replidentity_index_b ON tab_replidentity_index(b)"
+);
+# use index idx_replidentity_index_b as REPLICA IDENTITY on subscriber because
+# it reflects the future scenario we are testing: changing REPLICA IDENTITY
+# INDEX.
+$node_subscriber->safe_psql('postgres',
+ "ALTER TABLE tab_replidentity_index REPLICA IDENTITY USING INDEX idx_replidentity_index_b"
+);
+
+$publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+$node_publisher->safe_psql('postgres',
+ "CREATE PUBLICATION tap_pub FOR TABLE tab_replidentity_index");
+$node_subscriber->safe_psql('postgres',
+ "CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr' PUBLICATION tap_pub"
+);
+
+$node_publisher->wait_for_catchup('tap_sub');
+
+# Also wait for initial table sync to finish
+$node_subscriber->poll_query_until('postgres', $synced_query)
+ or die "Timed out while waiting for subscriber to synchronize data";
+
+is($node_subscriber->safe_psql('postgres', "SELECT * FROM tab_replidentity_index"),
+ qq(1|1
+2|2),
+ "check initial data on subscriber");
+
+# Set REPLICA IDENTITY to idx_replidentity_index_b on publisher, then run UPDATE and DELETE.
+$node_publisher->safe_psql('postgres', qq[
+ ALTER TABLE tab_replidentity_index REPLICA IDENTITY USING INDEX idx_replidentity_index_b;
+ UPDATE tab_replidentity_index SET a = -a WHERE a = 1;
+ DELETE FROM tab_replidentity_index WHERE a = 2;
+]);
+
+$node_publisher->wait_for_catchup('tap_sub');
+is( $node_subscriber->safe_psql('postgres', "SELECT * FROM tab_replidentity_index"),
+ qq(-1|1),
+ "update works with REPLICA IDENTITY");
+
+$node_publisher->stop('fast');
+$node_subscriber->stop('fast');
--
2.20.1
