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

Hi Amit, all

Amit Kapila <amit.kapila16@gmail.com>, 12 Tem 2023 Çar, 13:09 tarihinde
şunu yazdı:

> On Wed, Jul 12, 2023 at 12:31 PM Masahiko Sawada <sawada.mshk@gmail.com>
> wrote:
> >
> > On Tue, Jul 11, 2023 at 5:31 PM Peter Smith <smithpb2250@gmail.com>
> wrote:
> > >
> >
> > I don't think we have concluded any action for it. I agree that
> > IsIndexOnlyOnExpression() is redundant. We don't need to check *all*
> > index fields actually. I've attached a draft patch. It removes
> > IsIndexOnlyOnExpression() and merges
> > RemoteRelContainsLeftMostColumnOnIdx() to
> > FindUsableIndexForReplicaIdentityFull. One concern is that we no
> > longer do the assertion check with
> > IsIndexUsableForReplicaIdentityFull(). What do you think?
> >
>
> 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?
>
>
>
I think such an approach is slightly better than the proposed changes on
remove_redundant_check.patch

I think one reason we ended up with IsIndexUsableForReplicaIdentityFull()
is that it
is a nice way for documenting the requirements in the code.

However, as you also alluded to in the
thread, RemoteRelContainsLeftMostColumnOnIdx()
breaks this documentation.

I agree that it is nice to have all the logic to be in the same place. I
think remove_redundant_check.patch
does that by inlining IsIndexUsableForReplicaIdentityFull
and RemoteRelContainsLeftMostColumnOnIdx
into FindUsableIndexForReplicaIdentityFull().

As Amit noted, the other way around might be more interesting. We expand
IsIndexUsableForReplicaIdentityFull() such that it also includes
RemoteRelContainsLeftMostColumnOnIdx(). With that, readers of
IsIndexUsableForReplicaIdentityFull() can follow the requirements slightly
easier.

Though, not sure yet if we can get all the necessary information for the
Assert
via ApplyExecutionData in FindReplTupleInLocalRel. Perhaps yes.

Thanks,
Onder