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

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?

Few other comments on latest code:
============================
1.
<para>
- A published table must have a <quote>replica identity</quote> configured in
+ A published table must have a <firstterm>replica
identity</firstterm> configured in

How the above change is related to this patch?

2.
certain additional requirements) can also be set to be the replica
- identity. If the table does not have any suitable key, then it can be set
+ identity. If the table does not have any suitable key, then it can be set

I think we should change the spacing of existing docs (two spaces
after fullstop to one space) and that too inconsistently. I suggest to
add new changes with same spacing as existing doc. If you are adding
entirely new section then we can consider differently.

3.
to replica identity <quote>full</quote>, which means the entire row becomes
- the key. This, however, is very inefficient and should only be used as a
- fallback if no other solution is possible. If a replica identity other
- than <quote>full</quote> is set on the publisher side, a replica identity
- comprising the same or fewer columns must also be set on the subscriber
- side. See <xref linkend="sql-altertable-replica-identity"/> for details on
+ the key. When replica identity <literal>FULL</literal> is specified,
+ indexes can be used on the subscriber side for searching the rows. These

Shouldn't specifying <literal>FULL</literal> be consistent wih existing docs?

--
With Regards,
Amit Kapila.