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

Hi Amit, all

Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, 6 Mar 2023 Pzt, 12:40 tarihinde şunu
yazdı:

> On Fri, Mar 3, 2023 at 6:40 PM Önder Kalacı <onderkalaci(at)gmail(dot)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.
>
>
Yeah, seems easier to follow to me as well. Reflected it in the comment as
well.

Thanks,
Onder