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

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