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