Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher - Mailing list pgsql-hackers

From Önder Kalacı
Subject Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher
Date
Msg-id CACawEhVM5Kd8ON_86Qozg=GWKd9mLBoJE+VNi7Zu=o6muvgvaQ@mail.gmail.com
Whole thread Raw
In response to Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher  (Amit Kapila <amit.kapila16@gmail.com>)
List pgsql-hackers
Hi Amit, all



>
> 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?

I also re-investigated the code, and I also don't see any issues with that.

See my comment to Peter's original review on this.
 

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?

As you suggest, I'm undoing this change.
 

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.

Alright, so changed all this section to two spaces after fullstop.
 


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?


 
Considering the discussion below, I'm switching all back to <quote>full</quote>. Let's
be consistent with the existing code. Peter already suggested to improve that with a follow-up
patch. If that lands in, I can reflect the changes on this patch as well. 

Given the changes are small, I'll incorporate the changes with v33 in my next e-mail.

Thanks,
Onder 

pgsql-hackers by date:

Previous
From: Amit Kapila
Date:
Subject: Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher
Next
From: Önder Kalacı
Date:
Subject: Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher