Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

On Fri, Mar 3, 2023 at 6:40 PM Önder Kalacı <onderkalaci@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.