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.
Yeah, seems easier to follow to me as well. Reflected it in the comment as well.
