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

From: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
To: Peter Smith <smithpb2250(at)gmail(dot)com>
Cc: Önder Kalacı <onderkalaci(at)gmail(dot)com>, "shiy(dot)fnst(at)fujitsu(dot)com" <shiy(dot)fnst(at)fujitsu(dot)com>, "wangw(dot)fnst(at)fujitsu(dot)com" <wangw(dot)fnst(at)fujitsu(dot)com>, Marco Slot <marco(dot)slot(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Andres Freund <andres(at)anarazel(dot)de>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher
Date: 2023-03-06 08:40:29
Message-ID: CAA4eK1J8R-qS97cu27sF2=qzjhuQNkv+ZvgaTzFv7rs-LA4c2w@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Mon, Mar 6, 2023 at 1:40 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> 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.
>

But the same understanding is required for the existing function
GetRelationIdentityOrPK(), so I feel it is better to be consistent
unless we see some problem here.

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

Personally, I would prefer to keep those separate. So, feel free to
propose them in a new thread.

--
With Regards,
Amit Kapila.

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Julien Rouhaud 2023-03-06 08:52:18 Re: Combine pg_walinspect till_end_of_wal functions with others
Previous Message Alexander Kukushkin 2023-03-06 08:28:10 Re: pg_rewind: Skip log directory for file type check like pg_wal