Re: [BUG]Invalidate relcache when setting REPLICA IDENTITY
On Mon, Nov 15, 2021 at 7:20 AM houzj.f...@fujitsu.com wrote: > > On Sat, Nov 13, 2021 6:50 PM Amit Kapila wrote: > > > > The patch looks mostly good to me. I have slightly tweaked the comments in > > the code (as per my previous suggestion) and test. Also, I have slightly > > modified the commit message. If the attached looks good to you then kindly > > prepare patches for back-branches. > > Thanks for reviewing, the latest patch looks good to me. > Attach the patches for back branch (HEAD ~ v10). > Pushed. -- With Regards, Amit Kapila.
RE: [BUG]Invalidate relcache when setting REPLICA IDENTITY
On Sat, Nov 13, 2021 6:50 PM Amit Kapila wrote: > > The patch looks mostly good to me. I have slightly tweaked the comments in > the code (as per my previous suggestion) and test. Also, I have slightly > modified the commit message. If the attached looks good to you then kindly > prepare patches for back-branches. Thanks for reviewing, the latest patch looks good to me. Attach the patches for back branch (HEAD ~ v10). Best regards, Hou zj v8-HEAD-0001-Invalidate-relcache-when-changing-REPLICA-IDENTIT.patch Description: v8-HEAD-0001-Invalidate-relcache-when-changing-REPLICA-IDENTIT.patch v8-PG10-0001-Invalidate-relcache-when-changing-REPLICA-IDENTITY-i.patch Description: v8-PG10-0001-Invalidate-relcache-when-changing-REPLICA-IDENTITY-i.patch v8-PG12-0001-Invalidate-relcache-when-changing-REPLICA-IDENTITY-i.patch Description: v8-PG12-0001-Invalidate-relcache-when-changing-REPLICA-IDENTITY-i.patch v8-PG11-0001-Invalidate-relcache-when-changing-REPLICA-IDENTITY-i.patch Description: v8-PG11-0001-Invalidate-relcache-when-changing-REPLICA-IDENTITY-i.patch v8-PG13-0001-Invalidate-relcache-when-changing-REPLICA-IDENTITY-i.patch Description: v8-PG13-0001-Invalidate-relcache-when-changing-REPLICA-IDENTITY-i.patch v8-PG14-0001-Invalidate-relcache-when-changing-REPLICA-IDENTITY-i.patch Description: v8-PG14-0001-Invalidate-relcache-when-changing-REPLICA-IDENTITY-i.patch
Re: [BUG]Invalidate relcache when setting REPLICA IDENTITY
On Sat, Nov 13, 2021 at 12:47 AM Euler Taveira wrote: > > On Fri, Nov 12, 2021, at 3:10 AM, houzj.f...@fujitsu.com wrote: > > On Fri, Nov 12, 2021 1:33 PM Amit Kapila wrote: > > On Fri, Nov 12, 2021 at 10:50 AM Amit Kapila > > wrote: > > > But won't that generate invalidation for the rel twice in the case > > > (change Replica Identity from Nothing to some index) you mentioned in > > > the previous email? > > > > > > > Oh, I see the point. I think this is okay because > > AddRelcacheInvalidationMessage doesn't allow to add duplicate rel > > invalidation. If that is the case I wonder why not simply register > > invalidation without any check in the for loop as was the case with > > Tang's original patch? > > OK, I also think the code in Tang's original patch is fine. > Attach the patch which register invalidation without any check in the for > loop. > > WFM. > The patch looks mostly good to me. I have slightly tweaked the comments in the code (as per my previous suggestion) and test. Also, I have slightly modified the commit message. If the attached looks good to you then kindly prepare patches for back-branches. -- With Regards, Amit Kapila. v7-0001-Invalidate-relcache-when-changing-REPLICA-IDENTIT.patch Description: Binary data
Re: [BUG]Invalidate relcache when setting REPLICA IDENTITY
On Fri, Nov 12, 2021, at 3:10 AM, houzj.f...@fujitsu.com wrote: > On Fri, Nov 12, 2021 1:33 PM Amit Kapila wrote: > > On Fri, Nov 12, 2021 at 10:50 AM Amit Kapila > > wrote: > > > But won't that generate invalidation for the rel twice in the case > > > (change Replica Identity from Nothing to some index) you mentioned in > > > the previous email? > > > > > > > Oh, I see the point. I think this is okay because > > AddRelcacheInvalidationMessage doesn't allow to add duplicate rel > > invalidation. If that is the case I wonder why not simply register > > invalidation without any check in the for loop as was the case with > > Tang's original patch? > > OK, I also think the code in Tang's original patch is fine. > Attach the patch which register invalidation without any check in the for > loop. WFM. -- Euler Taveira EDB https://www.enterprisedb.com/
RE: [BUG]Invalidate relcache when setting REPLICA IDENTITY
On Fri, Nov 12, 2021 1:33 PM Amit Kapila wrote: > On Fri, Nov 12, 2021 at 10:50 AM Amit Kapila wrote: > > But won't that generate invalidation for the rel twice in the case > > (change Replica Identity from Nothing to some index) you mentioned in > > the previous email? > > > > Oh, I see the point. I think this is okay because > AddRelcacheInvalidationMessage doesn't allow to add duplicate rel > invalidation. If that is the case I wonder why not simply register > invalidation without any check in the for loop as was the case with > Tang's original patch? OK, I also think the code in Tang's original patch is fine. Attach the patch which register invalidation without any check in the for loop. Best regards, Hou zj v6-0001-Invalidate-relcache-entry-when-changing-REPLICA-IDEN.patch Description: v6-0001-Invalidate-relcache-entry-when-changing-REPLICA-IDEN.patch
Re: [BUG]Invalidate relcache when setting REPLICA IDENTITY
On Fri, Nov 12, 2021 at 10:50 AM Amit Kapila wrote: > > On Fri, Nov 12, 2021 at 10:27 AM houzj.f...@fujitsu.com > wrote: > > > > On Friday, November 12, 2021 10:46 AM I wrote: > > > On Friday, November 12, 2021 8:15 AM Euler Taveira > > > wrote: > > > > 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? > > > > > > Thanks for reviewing ! > > > But I am not sure it's better to simplify the code like "if > > > (OidIsValid(indexOid)) > > > CacheInvalidate". > > > > Oh, I got confused with the logic in relation_mark_replica_identity, sorry > > for that. > > I now realize that you are right, we can just check "if > > (OidIsValid(indexOid))" here > > to simplify the code. > > > > But won't that generate invalidation for the rel twice in the case > (change Replica Identity from Nothing to some index) you mentioned in > the previous email? > Oh, I see the point. I think this is okay because AddRelcacheInvalidationMessage doesn't allow to add duplicate rel invalidation. If that is the case I wonder why not simply register invalidation without any check in the for loop as was the case with Tang's original patch? -- With Regards, Amit Kapila.
Re: [BUG]Invalidate relcache when setting REPLICA IDENTITY
On Fri, Nov 12, 2021 at 10:27 AM houzj.f...@fujitsu.com wrote: > > On Friday, November 12, 2021 10:46 AM I wrote: > > On Friday, November 12, 2021 8:15 AM Euler Taveira > > wrote: > > > 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? > > > > Thanks for reviewing ! > > But I am not sure it's better to simplify the code like "if > > (OidIsValid(indexOid)) > > CacheInvalidate". > > Oh, I got confused with the logic in relation_mark_replica_identity, sorry > for that. > I now realize that you are right, we can just check "if > (OidIsValid(indexOid))" here > to simplify the code. > But won't that generate invalidation for the rel twice in the case (change Replica Identity from Nothing to some index) you mentioned in the previous email? -- With Regards, Amit Kapila.
RE: [BUG]Invalidate relcache when setting REPLICA IDENTITY
On Friday, November 12, 2021 10:46 AM I wrote: > On Friday, November 12, 2021 8:15 AM Euler Taveira > wrote: > > 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? > > Thanks for reviewing ! > But I am not sure it's better to simplify the code like "if > (OidIsValid(indexOid)) > CacheInvalidate". Oh, I got confused with the logic in relation_mark_replica_identity, sorry for that. I now realize that you are right, we can just check "if (OidIsValid(indexOid))" here to simplify the code. Attach the new version patch which simplify the check as pointed out by Euler. Best regards, Hou zj v5-0001-Invalidate-relcache-entry-when-changing-REPLICA-IDEN.patch Description: v5-0001-Invalidate-relcache-entry-when-changing-REPLICA-IDEN.patch
RE: [BUG]Invalidate relcache when setting REPLICA IDENTITY
On Friday, November 12, 2021 8:15 AM Euler Taveira wrote: > 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? Thanks for reviewing ! But I am not sure it's better to simplify the code like "if (OidIsValid(indexOid)) CacheInvalidate". After this change, it will also invalidate the relcache when changing REPLICA IDENTITY from DEFAULT/FULL/NOTHING to USING INDEX which I think is unnecessary, because the invalidate for these cases was already handled by "CatalogTupleUpdate(pg_class". So, I still think the flag need_rel_inval is needed. > 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. Thanks! Attach the patch which changed the code use need_rel_inval flag. Also ran pgperltidy for the testcases. I will post patches for backbranch if there are no more comments. Best regards, Hou zj v4-0001-Invalidate-relcache-entry-when-changing-REPLICA-IDEN.patch Description: v4-0001-Invalidate-relcache-entry-when-changing-REPLICA-IDEN.patch
Re: [BUG]Invalidate relcache when setting REPLICA IDENTITY
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"
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');
RE: [BUG]Invalidate relcache when setting REPLICA IDENTITY
On Thursday, November 11, 2021 5:36 PM houzj.f...@fujitsu.com wrote: > On Thur, Nov 11, 2021 12:08 PM Amit Kapila wrote: > > On Thu, Nov 11, 2021 at 7:07 AM houzj.f...@fujitsu.com > wrote: > > > > > > On Wed, Nov 10, 2021 7:29 PM Amit Kapila > wrote: > > > > > > > > I don't understand the purpose of idx_b in the above test case, > > > > why is it required to reproduce the problem? > > > > @@ -15488,6 +15488,7 @@ relation_mark_replica_identity(Relation > > > > rel, > > char > > > > ri_type, Oid indexOid, > > > > CatalogTupleUpdate(pg_index, _index_tuple->t_self, > > pg_index_tuple); > > > > InvokeObjectPostAlterHookArg(IndexRelationId, thisIndexOid, 0, > > > > InvalidOid, is_internal); > > > > + CacheInvalidateRelcache(rel); > > > > > > > > CatalogTupleUpdate internally calls heap_update which calls > > > > CacheInvalidateHeapTuple(), why is that not sufficient for invalidation? > > > > > > I think it's because the bug happens only when changing REPLICA > > > IDENTITY > > index > > > from one(idx_a) to another one(idx_b). > > > > > > > Okay, but do we need to invalidate the rel cache each time the dirty > > flag is set? Can't we do it once outside the foreach index loop? I > > think we can record whether to do relcache invalidation in a new > > boolean variable need_rel_inval or something like that. Also, it is > > better if you can add a comment on the lines of: "Invalidate the > > relcache for the table so that after we commit all sessions will > > refresh the table's replica identity index before attempting any > > update on the table.". > > I agree. > > Attach the new version fix patch which move the invalidation out of the loop > and > addressed the comments[1] for the testcases. Also attach the patches for back branch and remove some unnecessary changes from pgindent. Best regards, Hou zj PG12-0001-Invalidate-relcache-when-setting-REPLICA-IDENTITY.patch Description: PG12-0001-Invalidate-relcache-when-setting-REPLICA-IDENTITY.patch PG11-0001-Invalidate-relcache-when-setting-REPLICA-IDENTITY.patch Description: PG11-0001-Invalidate-relcache-when-setting-REPLICA-IDENTITY.patch PG10-0001-Invalidate-relcache-when-setting-REPLICA-IDENTITY.patch Description: PG10-0001-Invalidate-relcache-when-setting-REPLICA-IDENTITY.patch PG14-0001-Invalidate-relcache-when-setting-REPLICA-IDENTITY.patch Description: PG14-0001-Invalidate-relcache-when-setting-REPLICA-IDENTITY.patch PG13-0001-Invalidate-relcache-when-setting-REPLICA-IDENTITY.patch Description: PG13-0001-Invalidate-relcache-when-setting-REPLICA-IDENTITY.patch v2-HEAD-0001-Invalidate-relcache-when-setting-REPLICA-IDENTITY.patch Description: v2-HEAD-0001-Invalidate-relcache-when-setting-REPLICA-IDENTITY.patch
RE: [BUG]Invalidate relcache when setting REPLICA IDENTITY
On Thur, Nov 11, 2021 12:08 PM Amit Kapila wrote: > On Thu, Nov 11, 2021 at 7:07 AM houzj.f...@fujitsu.com > wrote: > > > > On Wed, Nov 10, 2021 7:29 PM Amit Kapila wrote: > > > > > > I don't understand the purpose of idx_b in the above test case, why is it > > > required to reproduce the problem? > > > @@ -15488,6 +15488,7 @@ relation_mark_replica_identity(Relation rel, > char > > > ri_type, Oid indexOid, > > > CatalogTupleUpdate(pg_index, _index_tuple->t_self, > pg_index_tuple); > > > InvokeObjectPostAlterHookArg(IndexRelationId, thisIndexOid, 0, > > > InvalidOid, is_internal); > > > + CacheInvalidateRelcache(rel); > > > > > > CatalogTupleUpdate internally calls heap_update which calls > > > CacheInvalidateHeapTuple(), why is that not sufficient for invalidation? > > > > I think it's because the bug happens only when changing REPLICA IDENTITY > index > > from one(idx_a) to another one(idx_b). > > > > Okay, but do we need to invalidate the rel cache each time the dirty > flag is set? Can't we do it once outside the foreach index loop? I > think we can record whether to do relcache invalidation in a new > boolean variable need_rel_inval or something like that. Also, it is > better if you can add a comment on the lines of: "Invalidate the > relcache for the table so that after we commit all sessions will > refresh the table's replica identity index before attempting any > update on the table.". I agree. Attach the new version fix patch which move the invalidation out of the loop and addressed the comments[1] for the testcases. [1] https://www.postgresql.org/message-id/CAA4eK1Ki-kUx52uRER3bZSC6bLc%3Diix%2BQnBxs1pZSHHYZOEF1A%40mail.gmail.com Best regards, Hou zj v2-0001-Invalidate-relcache-when-setting-REPLICA-IDENTITY.patch Description: v2-0001-Invalidate-relcache-when-setting-REPLICA-IDENTITY.patch
Re: [BUG]Invalidate relcache when setting REPLICA IDENTITY
On Thu, Nov 11, 2021 at 9:37 AM Amit Kapila wrote:
>
> On Thu, Nov 11, 2021 at 7:07 AM houzj.f...@fujitsu.com
> wrote:
> >
> > On Wed, Nov 10, 2021 7:29 PM Amit Kapila wrote:
> > >
> > > I don't understand the purpose of idx_b in the above test case, why is it
> > > required to reproduce the problem?
> > > @@ -15488,6 +15488,7 @@ relation_mark_replica_identity(Relation rel, char
> > > ri_type, Oid indexOid,
> > > CatalogTupleUpdate(pg_index, _index_tuple->t_self, pg_index_tuple);
> > > InvokeObjectPostAlterHookArg(IndexRelationId, thisIndexOid, 0,
> > > InvalidOid, is_internal);
> > > + CacheInvalidateRelcache(rel);
> > >
> > > CatalogTupleUpdate internally calls heap_update which calls
> > > CacheInvalidateHeapTuple(), why is that not sufficient for invalidation?
> >
> > I think it's because the bug happens only when changing REPLICA IDENTITY
> > index
> > from one(idx_a) to another one(idx_b).
> >
>
> Okay, but do we need to invalidate the rel cache each time the dirty
> flag is set? Can't we do it once outside the foreach index loop? I
> think we can record whether to do relcache invalidation in a new
> boolean variable need_rel_inval or something like that. Also, it is
> better if you can add a comment on the lines of: "Invalidate the
> relcache for the table so that after we commit all sessions will
> refresh the table's replica identity index before attempting any
> update on the table.".
>
Few comments on testcase added by the patch:
==
1.
+$node_subscriber->safe_psql('postgres',
+);
It is not clear what is the purpose served by this statement.
2.
+# replication of the table without replica identity index but not primary key
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE tab_replidentity_index(a int not null, b int not null)");
I think the part of the comment "but not primary key" is not required.
3. Can we move this test to subscription/t/100_bugs?
--
With Regards,
Amit Kapila.
Re: [BUG]Invalidate relcache when setting REPLICA IDENTITY
On Thu, Nov 11, 2021 at 7:07 AM houzj.f...@fujitsu.com wrote: > > On Wed, Nov 10, 2021 7:29 PM Amit Kapila wrote: > > > > I don't understand the purpose of idx_b in the above test case, why is it > > required to reproduce the problem? > > @@ -15488,6 +15488,7 @@ relation_mark_replica_identity(Relation rel, char > > ri_type, Oid indexOid, > > CatalogTupleUpdate(pg_index, _index_tuple->t_self, pg_index_tuple); > > InvokeObjectPostAlterHookArg(IndexRelationId, thisIndexOid, 0, > > InvalidOid, is_internal); > > + CacheInvalidateRelcache(rel); > > > > CatalogTupleUpdate internally calls heap_update which calls > > CacheInvalidateHeapTuple(), why is that not sufficient for invalidation? > > I think it's because the bug happens only when changing REPLICA IDENTITY index > from one(idx_a) to another one(idx_b). > Okay, but do we need to invalidate the rel cache each time the dirty flag is set? Can't we do it once outside the foreach index loop? I think we can record whether to do relcache invalidation in a new boolean variable need_rel_inval or something like that. Also, it is better if you can add a comment on the lines of: "Invalidate the relcache for the table so that after we commit all sessions will refresh the table's replica identity index before attempting any update on the table.". -- With Regards, Amit Kapila.
RE: [BUG]Invalidate relcache when setting REPLICA IDENTITY
On Wed, Nov 10, 2021 7:29 PM Amit Kapila wrote:
> On Wed, Nov 10, 2021 at 12:42 PM tanghy.f...@fujitsu.com
> wrote:
> >
> > Hi
> >
> > I think I found a bug related to logical replication(REPLICA IDENTITY in
> specific).
> > If I change REPLICA IDENTITY after creating publication, the
> DELETE/UPDATE operations won't be replicated as expected.
> >
> > For example:
> > -- publisher
> > CREATE TABLE tbl(a int, b int);
> > ALTER TABLE tbl ALTER COLUMN a SET NOT NULL; CREATE UNIQUE INDEX
> idx_a
> > ON tbl(a); ALTER TABLE tbl ALTER COLUMN b SET NOT NULL; CREATE UNIQUE
> > INDEX idx_b ON tbl(b); ALTER TABLE tbl REPLICA IDENTITY USING INDEX
> > idx_a; CREATE PUBLICATION pub FOR TABLE tbl; ALTER TABLE tbl REPLICA
> > IDENTITY USING INDEX idx_b; INSERT INTO tbl VALUES (1,1);
> >
> > -- subscriber
> > CREATE TABLE tbl(a int, b int);
> > ALTER TABLE tbl ALTER COLUMN b SET NOT NULL; CREATE UNIQUE INDEX
> idx_b
> > ON tbl(b); ALTER TABLE tbl REPLICA IDENTITY USING INDEX idx_b; CREATE
> > SUBSCRIPTION sub CONNECTION 'dbname=postgres port=5432'
> PUBLICATION
> > pub; SELECT * FROM tbl;
> >
> > -- publisher
> > postgres=# UPDATE tbl SET a=-a;
> > UPDATE 1
> > postgres=# SELECT * FROM tbl;
> > a | b
> > +---
> > -1 | 1
> > (1 row)
> >
> > -- subscriber
> > postgres=# SELECT * FROM tbl;
> > a | b
> > ---+---
> > 1 | 1
> > (1 row)
> >
> > (a in subscriber should be -1)
> >
>
> I don't understand the purpose of idx_b in the above test case, why is it
> required to reproduce the problem?
> @@ -15488,6 +15488,7 @@ relation_mark_replica_identity(Relation rel, char
> ri_type, Oid indexOid,
> CatalogTupleUpdate(pg_index, _index_tuple->t_self, pg_index_tuple);
> InvokeObjectPostAlterHookArg(IndexRelationId, thisIndexOid, 0,
> InvalidOid, is_internal);
> + CacheInvalidateRelcache(rel);
>
> CatalogTupleUpdate internally calls heap_update which calls
> CacheInvalidateHeapTuple(), why is that not sufficient for invalidation?
I think it's because the bug happens only when changing REPLICA IDENTITY index
from one(idx_a) to another one(idx_b).
When first time specify the REPLICA IDENTITY index, the code works well because
it will invoke 'CatalogTupleUpdate(pg_class,...' Which will invalidate the
target table's relcache.
if (pg_class_form->relreplident != ri_type)
{
pg_class_form->relreplident = ri_type;
CatalogTupleUpdate(pg_class, _class_tuple->t_self,
pg_class_tuple);
}
But when changing REPLICA IDENTITY index, the code only invoke
'CatalogTupleUpdate(pg_index,' which seems only invalidate the index's cache
not the
target table.
if (dirty)
{
CatalogTupleUpdate(pg_index, _index_tuple->t_self,
pg_index_tuple);
InvokeObjectPostAlterHookArg(IndexRelationId,
thisIndexOid, 0,
InvalidOid, is_internal);
}
Best regards,
Hou zj
Re: [BUG]Invalidate relcache when setting REPLICA IDENTITY
On Wed, Nov 10, 2021 at 12:42 PM tanghy.f...@fujitsu.com wrote: > > Hi > > I think I found a bug related to logical replication(REPLICA IDENTITY in > specific). > If I change REPLICA IDENTITY after creating publication, the DELETE/UPDATE > operations won't be replicated as expected. > > For example: > -- publisher > CREATE TABLE tbl(a int, b int); > ALTER TABLE tbl ALTER COLUMN a SET NOT NULL; > CREATE UNIQUE INDEX idx_a ON tbl(a); > ALTER TABLE tbl ALTER COLUMN b SET NOT NULL; > CREATE UNIQUE INDEX idx_b ON tbl(b); > ALTER TABLE tbl REPLICA IDENTITY USING INDEX idx_a; > CREATE PUBLICATION pub FOR TABLE tbl; > ALTER TABLE tbl REPLICA IDENTITY USING INDEX idx_b; > INSERT INTO tbl VALUES (1,1); > > -- subscriber > CREATE TABLE tbl(a int, b int); > ALTER TABLE tbl ALTER COLUMN b SET NOT NULL; > CREATE UNIQUE INDEX idx_b ON tbl(b); > ALTER TABLE tbl REPLICA IDENTITY USING INDEX idx_b; > CREATE SUBSCRIPTION sub CONNECTION 'dbname=postgres port=5432' PUBLICATION > pub; > SELECT * FROM tbl; > > -- publisher > postgres=# UPDATE tbl SET a=-a; > UPDATE 1 > postgres=# SELECT * FROM tbl; > a | b > +--- > -1 | 1 > (1 row) > > -- subscriber > postgres=# SELECT * FROM tbl; > a | b > ---+--- > 1 | 1 > (1 row) > > (a in subscriber should be -1) > I don't understand the purpose of idx_b in the above test case, why is it required to reproduce the problem? @@ -15488,6 +15488,7 @@ relation_mark_replica_identity(Relation rel, char ri_type, Oid indexOid, CatalogTupleUpdate(pg_index, _index_tuple->t_self, pg_index_tuple); InvokeObjectPostAlterHookArg(IndexRelationId, thisIndexOid, 0, InvalidOid, is_internal); + CacheInvalidateRelcache(rel); CatalogTupleUpdate internally calls heap_update which calls CacheInvalidateHeapTuple(), why is that not sufficient for invalidation? -- With Regards, Amit Kapila.
[BUG]Invalidate relcache when setting REPLICA IDENTITY
Hi I think I found a bug related to logical replication(REPLICA IDENTITY in specific). If I change REPLICA IDENTITY after creating publication, the DELETE/UPDATE operations won't be replicated as expected. For example: -- publisher CREATE TABLE tbl(a int, b int); ALTER TABLE tbl ALTER COLUMN a SET NOT NULL; CREATE UNIQUE INDEX idx_a ON tbl(a); ALTER TABLE tbl ALTER COLUMN b SET NOT NULL; CREATE UNIQUE INDEX idx_b ON tbl(b); ALTER TABLE tbl REPLICA IDENTITY USING INDEX idx_a; CREATE PUBLICATION pub FOR TABLE tbl; ALTER TABLE tbl REPLICA IDENTITY USING INDEX idx_b; INSERT INTO tbl VALUES (1,1); -- subscriber CREATE TABLE tbl(a int, b int); ALTER TABLE tbl ALTER COLUMN b SET NOT NULL; CREATE UNIQUE INDEX idx_b ON tbl(b); ALTER TABLE tbl REPLICA IDENTITY USING INDEX idx_b; CREATE SUBSCRIPTION sub CONNECTION 'dbname=postgres port=5432' PUBLICATION pub; SELECT * FROM tbl; -- publisher postgres=# UPDATE tbl SET a=-a; UPDATE 1 postgres=# SELECT * FROM tbl; a | b +--- -1 | 1 (1 row) -- subscriber postgres=# SELECT * FROM tbl; a | b ---+--- 1 | 1 (1 row) (a in subscriber should be -1) But if I restart a psql client before executing UPDATE operation in publication, it works well. So I think the problem is: when using "ALTER TABLE ... REPLICA IDENTITY USING INDEX ...", relcahe was not invalidated. Attach a patch to fix it, I also added a test case for it.(Hou helped me to write/review this patch, which is very kind of him) FYI: I also tested that same problem could be reproduced on PG10 ~ PG14. (Logical replication is introduced in PG10.) So I think backpatch is needed here. Regards Tang 0001-Invalidate-relcache-when-setting-REPLICA-IDENTITY.patch Description: 0001-Invalidate-relcache-when-setting-REPLICA-IDENTITY.patch
