Re: Replica Identity check of partition table on subscriber

From: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
To: Amit Langote <amitlangote09(at)gmail(dot)com>
Cc: "shiy(dot)fnst(at)fujitsu(dot)com" <shiy(dot)fnst(at)fujitsu(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com>
Subject: Re: Replica Identity check of partition table on subscriber
Date: 2022-06-16 08:30:12
Message-ID: CAA4eK1+dV8pDEp=zwgUJpRO3UD_We6aao5zZACFfff-Cpo7oUQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Thu, Jun 16, 2022 at 12:30 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>
> On Thu, Jun 16, 2022 at 3:45 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > On Thu, Jun 16, 2022 at 11:43 AM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > > + * Don't throw any error here just mark the relation entry as not updatable,
> > > + * as replica identity is only for updates and deletes but inserts can be
> > > + * replicated even without it.
> > >
> > > I know you're simply copying the old comment, but I think we can
> > > rewrite it to be slightly more useful:
> > >
> > > We just mark the relation entry as not updatable here if the local
> > > replica identity is found to be insufficient and leave it to
> > > check_relation_updatable() to throw the actual error if needed.
> >
> > I am fine with improving this comment but it would be better if in
> > some way we keep the following part of the comment: "as replica
> > identity is only for updates and deletes but inserts can be replicated
> > even without it." as that makes it more clear why it is okay to just
> > mark the entry as not updatable. One idea could be: "We just mark the
> > relation entry as not updatable here if the local replica identity is
> > found to be insufficient and leave it to check_relation_updatable() to
> > throw the actual error if needed. This is because replica identity is
> > only for updates and deletes but inserts can be replicated even
> > without it.". Feel free to suggest if you have any better ideas?
>
> I thought mentioning check_relation_updatable() would make it clear
> that only updates (and deletes) care about a valid local replica
> identity, because only apply_handle_{update|delete}() call that
> function. Anyway, how about this:
>
> We just mark the relation entry as not updatable here if the local
> replica identity is found to be insufficient for applying
> updates/deletes (inserts don't care!) and leave it to
> check_relation_updatable() to throw the actual error if needed.
>

This sounds better to me than the previous text.

--
With Regards,
Amit Kapila.

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2022-06-16 08:31:22 Re: Prevent writes on large objects in read-only transactions
Previous Message Kyotaro Horiguchi 2022-06-16 08:19:46 Re: fix stats_fetch_consistency value in postgresql.conf.sample