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

On Mon, Mar 6, 2023 at 1:40 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Mon, Mar 6, 2023 at 5:44 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Mon, Mar 6, 2023 at 10:12 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > >
> > > 4. IdxIsRelationIdentityOrPK
> > >
> > > +/*
> > > + * Given a relation and OID of an index, returns true if the
> > > + * index is relation's replica identity index or relation's
> > > + * primary key's index.
> > > + *
> > > + * Returns false otherwise.
> > > + */
> > > +bool
> > > +IdxIsRelationIdentityOrPK(Relation rel, Oid idxoid)
> > > +{
> > > + Assert(OidIsValid(idxoid));
> > > +
> > > + return GetRelationIdentityOrPK(rel) == idxoid;
> > > +}
> > >
> > > I think you've "simplified" this function in v28 but AFAICT now it has
> > > a different logic to v27.
> > >
> > > PREVIOUSLY it was coded like
> > > + return RelationGetReplicaIndex(rel) == idxoid ||
> > > + RelationGetPrimaryKeyIndex(rel) == idxoid;
> > >
> > > You can see if 'idxoid' did NOT match RI but if it DID match PK
> > > previously it would return true. But now in that scenario, it won't
> > > even check the PK if there was a valid RI. So it might return false
> > > when previously it returned true. Is it deliberate?
> > >
> >
> > I don't see any problem with this because by default PK will be a
> > replica identity. So only if the user specifies the replica identity
> > full or changes the replica identity to some other index, we will try
> > to get PK which seems valid for this case. Am, I missing something
> > which makes this code do something bad?
>
> I don't know if there is anything bad; the point was that the function
> now seems to require a deeper understanding of the interrelationship
> of RelationGetReplicaIndex and RelationGetPrimaryKeyIndex, which is
> something the previous implementation did not require.
>

But the same understanding is required for the existing function
GetRelationIdentityOrPK(), so I feel it is better to be consistent
unless we see some problem here.

>
> Anyhow, if you feel those firstterm and FULL changes ought to be kept
> separate from this RI patch, please let me know and I will propose
> those changes in a new thread,
>

Personally, I would prefer to keep those separate. So, feel free to
propose them in a new thread.

--
With Regards,
Amit Kapila.