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

On Mon, Mar 6, 2023 at 10:18 PM Önder Kalacı <onderkalaci(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;
>>
>> 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?
>>
>
> Thanks for detailed review/investigation on this. But, I also agree that
> there is no difference in terms of correctness. Also, it is probably better
> to be consistent with the existing code. So, making IdxIsRelationIdentityOrPK()
> relying on GetRelationIdentityOrPK() still sounds better to me.
>
>> You can see if 'idxoid' did NOT match RI but if it DID match PK
>> previously it would return true.
>
>
> Still, I cannot see how this check would yield a different result with how
> RI/PK works -- as Amit also noted in the next e-mail.
>
> Do you see any cases where this check would produce a different result?
> I cannot, but wanted to double check with you.
>
>

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. (??)

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