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

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.

--
With Regards,
Amit Kapila.