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

On Tue, Mar 7, 2023 at 3:00 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Tue, Mar 7, 2023 at 8:01 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Mar 7, 2023 at 1:19 PM Peter Smith <smithpb2250@gmail.com> wrote:
> > >
> > > Let me give an example to demonstrate why I thought something is fishy here:
> > >
> > > Imagine rel has a (non-default) REPLICA IDENTITY with Oid=1111.
> > > Imagine the same rel has a PRIMARY KEY with Oid=2222.
> > >
> > > ---
> > >
> > > +/*
> > > + * Get replica identity index or if it is not defined a primary key.
> > > + *
> > > + * If neither is defined, returns InvalidOid
> > > + */
> > > +Oid
> > > +GetRelationIdentityOrPK(Relation rel)
> > > +{
> > > + Oid idxoid;
> > > +
> > > + idxoid = RelationGetReplicaIndex(rel);
> > > +
> > > + if (!OidIsValid(idxoid))
> > > + idxoid = RelationGetPrimaryKeyIndex(rel);
> > > +
> > > + return idxoid;
> > > +}
> > > +
> > > +/*
> > > + * 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;
> > > +}
> > > +
> > >
> > >
> > > So, according to the above function comment/name, I will expect
> > > calling IdxIsRelationIdentityOrPK passing Oid=1111 or Oid-2222 will
> > > both return true, right?
> > >
> > > But AFAICT
> > >
> > > IdxIsRelationIdentityOrPK(rel, 1111) --> GetRelationIdentityOrPK(rel)
> > > returns 1111 (the valid oid of the RI) --> 1111 == 1111 --> true;
> > >
> > > IdxIsRelationIdentityOrPK(rel, 2222) --> GetRelationIdentityOrPK(rel)
> > > returns 1111 (the valid oid of the RI) --> 1111 == 2222 --> false;
> > >
> > > ~
> > >
> > > Now two people are telling me this is OK, but I've been staring at it
> > > for too long and I just don't see how it can be. (??)
> > >
> >
> > The difference is that you are misunderstanding the intent of this
> > function. GetRelationIdentityOrPK() returns a "replica identity index
> > oid" if the same is defined, else return PK, if that is defined,
> > otherwise, return invalidOid. This is what is expected by its callers.
> > Now, one can argue to have a different function name and that may be a
> > valid argument but as far as I can see the function does what is
> > expected from it.
> >
>
> Sure, but I am questioning the function IdxIsRelationIdentityOrPK, not
> GetRelationIdentityOrPK.
>

The intent of IdxIsRelationIdentityOrPK() is as follows: Returns true
for the following conditions (a) if the given index OID is the same as
replica identity (when the same is defined); else (if replica identity
is not defined then (b)) (b) if the given OID is the same as PK.
Returns false otherwise. Feel free to propose any better function name
or if you think comments can be changed which makes it easier to
understand.

--
With Regards,
Amit Kapila.