Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

On Thu, Jul 13, 2023 at 10:55 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Wed, Jul 12, 2023 at 11:15 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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.