On Thu, Jul 13, 2023 at 10:55 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > On Wed, Jul 12, 2023 at 11:15 PM Masahiko Sawada <sawada.m...@gmail.com> > wrote: > > > > > I think this is a valid concern. Can't we move all the checks > > > (including the remote attrs check) inside > > > IsIndexUsableForReplicaIdentityFull() and then call it from both > > > places? Won't we have attrmap information available in the callers of > > > FindReplTupleInLocalRel() via ApplyExecutionData? > > > > You mean to pass ApplyExecutionData or attrmap down to > > RelationFindReplTupleByIndex()? I think it would be better to call it > > from FindReplTupleInLocalRel() instead. > > I've attached a draft patch for this idea. >
Looks reasonable to me. However, I am not very sure if we need to change the prototype of RelationFindReplTupleByIndex(). Few other minor comments: 1. - * has been implemented as a tri-state with values DISABLED, PENDING, and +n * has been implemented as a tri-state with values DISABLED, PENDING, and * ENABLED. * The above seems like a spurious change. 2. + /* And must reference the remote relation column */ + if (attrmap->maplen <= AttrNumberGetAttrOffset(keycol) || + attrmap->attnums[AttrNumberGetAttrOffset(keycol)] < 0) + return false; + I think we should specify the reason for this. I see that in the commit fd48a86c62 [1], the reason for this is removed. Shouldn't we retain that in some form? [1] - - * We also skip indexes if the remote relation does not contain the leftmost - * column of the index. This is because in most such cases sequential scan is - * favorable over index scan. -- With Regards, Amit Kapila.
