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

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.

------
Kind Regards,
Peter Smith.
Fujitsu Australia