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

On Mon, Mar 6, 2023 at 5:44 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> 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?

I don't know if there is anything bad; the point was that the function
now seems to require a deeper understanding of the interrelationship
of RelationGetReplicaIndex and RelationGetPrimaryKeyIndex, which is
something the previous implementation did not require.

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

That comes from a previous suggestion of mine. Strictly speaking, it
is unrelated to this patch. But since we are modifying this paragraph
in a major way anyhow it seemed harmless to just fix this in passing
too. OTOH I could make another patch for this but it seemed like
unnecessary extra work.

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

That comes from a previous suggestion of mine too. The RI is specified
as FULL, not "full".
See https://www.postgresql.org/docs/current/sql-altertable.html#SQL-ALTERTABLE-REPLICA-IDENTITY

Sure, there was an existing <quote>full</quote> in this paragraph so
strictly speaking the RI patch could follow that style. But IMO that
style was wrong so all we are doing compounding the mistake instead of
just fixing everything in passing. OTOH, I could make a separate patch
to fix "full" to FULL, but again that seemed like unnecessary extra
work.

~

Anyhow, if you feel those firstterm and FULL changes ought to be kept
separate from this RI patch, please let me know and I will propose
those changes in a new thread,

------
Kind Regards,
Peter Smith.
Fujitsu Australia