RE: [BUG]Invalidate relcache when setting REPLICA IDENTITY

On Thur, Nov 11, 2021 12:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Nov 11, 2021 at 7:07 AM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote:
> >
> > On Wed, Nov 10, 2021 7:29 PM Amit Kapila <amit.kapila16@gmail.com> 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, &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).
> >
> 
> 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