RE: [BUG]Invalidate relcache when setting REPLICA IDENTITY - Mailing list pgsql-hackers

On Wed, Nov 10, 2021 7:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Wed, Nov 10, 2021 at 12:42 PM tanghy.fnst@fujitsu.com
> <tanghy.fnst@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, &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