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
