RE: [BUG]Invalidate relcache when setting REPLICA IDENTITY

On Wed, Nov 10, 2021 7:29 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Wed, Nov 10, 2021 at 12:42 PM tanghy(dot)fnst(at)fujitsu(dot)com
> <tanghy(dot)fnst(at)fujitsu(dot)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, &pg_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, &pg_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, &pg_index_tuple->t_self, pg_index_tuple);
InvokeObjectPostAlterHookArg(IndexRelationId, thisIndexOid, 0,
InvalidOid, is_internal);
}

Best regards,
Hou zj