On Fri, Mar 3, 2023 at 6:40 PM Önder Kalacı <onderkal...@gmail.com> wrote:
>
> Hi Vignesh,
>
> Thanks for the review
>
>>
>> 1) We are currently calling RelationGetIndexList twice, once in
>> FindUsableIndexForReplicaIdentityFull function and in the caller too,
>> we could avoid one of the calls by passing the indexlist to the
>> function or removing the check here, index list check can be handled
>> in FindUsableIndexForReplicaIdentityFull.
>> + if (remoterel->replident == REPLICA_IDENTITY_FULL &&
>> + RelationGetIndexList(localrel) != NIL)
>> + {
>> + /*
>> + * If we had a primary key or relation identity with a
>> unique index,
>> + * we would have already found and returned that oid.
>> At this point,
>> + * the remote relation has replica identity full and
>> we have at least
>> + * one local index defined.
>> + *
>> + * We are looking for one more opportunity for using
>> an index. If
>> + * there are any indexes defined on the local
>> relation, try to pick
>> + * a suitable index.
>> + *
>> + * The index selection safely assumes that all the
>> columns are going
>> + * to be available for the index scan given that
>> remote relation has
>> + * replica identity full.
>> + */
>> + return FindUsableIndexForReplicaIdentityFull(localrel);
>> + }
>> +
>
> makes sense, done
>
Today, I was looking at this comment and the fix for it. It seems to
me that it would be better to not add the check (indexlist != NIL)
here and rather get the indexlist in
FindUsableIndexForReplicaIdentityFull(). It will anyway return
InvalidOid, if there is no index and that way code will look a bit
cleaner.
--
With Regards,
Amit Kapila.
