RE: [BUG]Invalidate relcache when setting REPLICA IDENTITY

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

Also attach the patches for back branch and remove some unnecessary
changes from pgindent.

Best regards,
Hou zj