On Mon, 20 Apr 2020 at 10:25, Masahiko Sawada <
masahiko.saw...@2ndquadrant.com> wrote:
> On Thu, 16 Apr 2020 at 17:48, Dilip Kumar <dilipbal...@gmail.com> wrote:
>
> I could reproduce this issue by the steps you shared. For the bug fix
> patch, I basically agree to remove that assertion from
> build_replindex_scan_key() but I think it's better to update the
> assertion instead of removal and update the following comment:
>
> IMO the assertion is using the wrong function because it should test a
replica
identity or primary key (GetRelationIdentityOrPK). RelationGetReplicaIndex
returns InvalidOid even though the table has a primary key.
GetRelationIdentityOrPK tries to obtain a replica identity and if it fails,
it
tries a primary key. That's exact what this assertion should use. We should
also notice that FindReplTupleInLocalRel uses GetRelationIdentityOrPK and
after
a code path like RelationFindReplTupleByIndex -> build_replindex_scan_key it
should also use the same function.
Since GetRelationIdentityOrPK is a fallback function that
uses RelationGetReplicaIndex and RelationGetPrimaryKeyIndex, I propose that
we
move this static function to execReplication.c.
I attached a patch with the described solution. I also included a test that
covers this scenario.
--
Euler Taveira http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 37be6d62bf26a20ec3cba8825082ad92fdd1c6d7 Mon Sep 17 00:00:00 2001
From: Euler Taveira <euler.tave...@2ndquadrant.com>
Date: Sat, 9 May 2020 20:41:50 -0300
Subject: [PATCH] Fix assert failure with REPLICA IDENTITY FULL in the
subscriber
This assertion failure can be reproduced using a replicated table in the
subscriber with REPLICA IDENTITY FULL. Since FindReplTupleInLocalRel
uses GetRelationIdentityOrPK to obtain a replica identity or primary
key, the follow path RelationFindReplTupleByIndex and then
build_replindex_scan_key should also use the same function
(GetRelationIdentityOrPK) to test an assertion condition. The Assert is
using RelationGetReplicaIndex that returns InvalidOid because REPLICA
IDENTITY is FULL (see RelationGetIndexList).
---
src/backend/executor/execReplication.c | 2 +-
src/backend/replication/logical/worker.c | 18 ------------------
src/backend/utils/cache/relcache.c | 19 +++++++++++++++++++
src/include/utils/relcache.h | 1 +
src/test/subscription/t/001_rep_changes.pl | 12 +++++++++++-
5 files changed, 32 insertions(+), 20 deletions(-)
diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c
index 1418746eb8..d9d473a4cd 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -57,7 +57,7 @@ build_replindex_scan_key(ScanKey skey, Relation rel, Relation idxrel,
int2vector *indkey = &idxrel->rd_index->indkey;
bool hasnulls = false;
- Assert(RelationGetReplicaIndex(rel) == RelationGetRelid(idxrel));
+ Assert(GetRelationIdentityOrPK(rel) == RelationGetRelid(idxrel));
indclassDatum = SysCacheGetAttr(INDEXRELID, idxrel->rd_indextuple,
Anum_pg_index_indclass, &isnull);
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index a752a1224d..a2d78e74eb 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -584,24 +584,6 @@ apply_handle_type(StringInfo s)
logicalrep_typmap_update(&typ);
}
-/*
- * Get replica identity index or if it is not defined a primary key.
- *
- * If neither is defined, returns InvalidOid
- */
-static Oid
-GetRelationIdentityOrPK(Relation rel)
-{
- Oid idxoid;
-
- idxoid = RelationGetReplicaIndex(rel);
-
- if (!OidIsValid(idxoid))
- idxoid = RelationGetPrimaryKeyIndex(rel);
-
- return idxoid;
-}
-
/*
* Handle INSERT message.
*/
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 9f1f11d0c1..c5849f15d1 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -4727,6 +4727,25 @@ RelationGetReplicaIndex(Relation relation)
return relation->rd_replidindex;
}
+/*
+ * GetRelationIdentityOrPK -- get relation's replica identity index or, if it
+ * is not defined, the primary key
+ *
+ * If neither is defined, returns InvalidOid.
+ */
+Oid
+GetRelationIdentityOrPK(Relation rel)
+{
+ Oid idxoid;
+
+ idxoid = RelationGetReplicaIndex(rel);
+
+ if (!OidIsValid(idxoid))
+ idxoid = RelationGetPrimaryKeyIndex(rel);
+
+ return idxoid;
+}
+
/*
* RelationGetIndexExpressions -- get the index expressions for an index
*
diff --git a/src/include/utils/relcache.h b/src/include/utils/relcache.h
index 9a85b7dd57..c0f7675cdc 100644
--- a/src/include/utils/relcache.h
+++ b/src/include/utils/relcache.h
@@ -48,6 +48,7 @@ extern List *RelationGetIndexList(Relation relation);
extern List *RelationGetStatExtList(Relation relation);
extern Oid RelationGetPrimaryKeyIndex(Relation relation);
extern Oid RelationGetReplicaIndex(Relation relation);
+extern Oid GetRelationIdentityOrPK(Relation relation);
extern List *RelationGetIndexExpressions(Relation relation);
extern List *RelationGetDummyIndexExpressions(Relation relation);
extern List *RelationGetIndexPredicate(Relation relation);
diff --git a/src/test/subscription/t/001_rep_changes.pl b/src/test/subscription/t/001_rep_changes.pl
index 6da7f71ca3..7d9ea1f002 100644
--- a/src/test/subscription/t/001_rep_changes.pl
+++ b/src/test/subscription/t/001_rep_changes.pl
@@ -34,6 +34,8 @@ $node_publisher->safe_psql('postgres',
$node_publisher->safe_psql('postgres',
"CREATE TABLE tab_include (a int, b text, CONSTRAINT covering PRIMARY KEY(a) INCLUDE(b))"
);
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE tab_full_pk (a int primary key, b text)");
# Let this table with REPLICA IDENTITY NOTHING, allowing only INSERT changes.
$node_publisher->safe_psql('postgres', "CREATE TABLE tab_nothing (a int)");
$node_publisher->safe_psql('postgres',
@@ -46,6 +48,9 @@ $node_subscriber->safe_psql('postgres', "CREATE TABLE tab_full (a int)");
$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_full2 (x text)");
$node_subscriber->safe_psql('postgres',
"CREATE TABLE tab_rep (a int primary key)");
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE tab_full_pk (a int primary key, b text)");
+$node_subscriber->safe_psql('postgres', "ALTER TABLE tab_full_pk REPLICA IDENTITY FULL");
$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_nothing (a int)");
# different column count and order than on publisher
@@ -64,7 +69,7 @@ $node_publisher->safe_psql('postgres', "CREATE PUBLICATION tap_pub");
$node_publisher->safe_psql('postgres',
"CREATE PUBLICATION tap_pub_ins_only WITH (publish = insert)");
$node_publisher->safe_psql('postgres',
- "ALTER PUBLICATION tap_pub ADD TABLE tab_rep, tab_full, tab_full2, tab_mixed, tab_include, tab_nothing"
+ "ALTER PUBLICATION tap_pub ADD TABLE tab_rep, tab_full, tab_full2, tab_mixed, tab_include, tab_nothing, tab_full_pk"
);
$node_publisher->safe_psql('postgres',
"ALTER PUBLICATION tap_pub_ins_only ADD TABLE tab_ins");
@@ -102,6 +107,9 @@ $node_publisher->safe_psql('postgres', "UPDATE tab_rep SET a = -a");
$node_publisher->safe_psql('postgres',
"INSERT INTO tab_mixed VALUES (2, 'bar', 2.2)");
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_full_pk VALUES (1, 'foo')");
+
$node_publisher->safe_psql('postgres',
"INSERT INTO tab_nothing VALUES (generate_series(1,20))");
@@ -159,6 +167,8 @@ $node_publisher->safe_psql('postgres',
"UPDATE tab_full2 SET x = 'bb' WHERE x = 'b'");
$node_publisher->safe_psql('postgres',
"UPDATE tab_mixed SET b = 'baz' WHERE a = 1");
+$node_publisher->safe_psql('postgres',
+ "UPDATE tab_full_pk SET b = 'bar' WHERE a = 1");
$node_publisher->wait_for_catchup('tap_sub');
--
2.20.1
