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
