Re: Synchronizing slots from primary to standby - Mailing list pgsql-hackers

From Bertrand Drouvot
Subject Re: Synchronizing slots from primary to standby
Date
Msg-id ZdyRFXu5wVvZY10T@ip-10-97-1-34.eu-west-3.compute.internal
Whole thread Raw
In response to Re: Synchronizing slots from primary to standby  (shveta malik <shveta.malik@gmail.com>)
List pgsql-hackers
Hi,

On Mon, Feb 26, 2024 at 05:52:40PM +0530, shveta malik wrote:
> On Mon, Feb 26, 2024 at 5:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > > > > +       if (!ok)
> > > > > +               GUC_check_errdetail("List syntax is invalid.");
> > > > > +
> > > > > +       /*
> > > > > +        * If there is a syntax error in the name or if the replication slots'
> > > > > +        * data is not initialized yet (i.e., we are in the startup process), skip
> > > > > +        * the slot verification.
> > > > > +        */
> > > > > +       if (!ok || !ReplicationSlotCtl)
> > > > > +       {
> > > > > +               pfree(rawname);
> > > > > +               list_free(elemlist);
> > > > > +               return ok;
> > > > > +       }
> > > > >
> > > > > we are testing the "ok" value twice, what about using if...else if... instead and
> > > > > test it once? If so, it might be worth to put the:
> > > > >
> > > > > "
> > > > > +       pfree(rawname);
> > > > > +       list_free(elemlist);
> > > > > +       return ok;
> > > > > "
> > > > >
> > > > > in a "goto".
> > > >
> > > > There were comments to remove the 'goto' statement and avoid
> > > > duplicate free code, so I prefer the current style.
> > >
> > > The duplicate free code would come from the if...else if... rewrite but then
> > > the "goto" would remove it, so I'm not sure to understand your point.
> > >
> >
> > I think Hou-San wants to say that there was previously a comment to
> > remove goto and now you are saying to introduce it. But, I think we
> > can avoid both code duplication and goto, if the first thing we check
> > in the function is ReplicationSlotCtl and return false if the same is
> > not set. Won't that be better?
> 
> I think we can not do that as we need to check atleast syntax before
> we return due to NULL ReplicationSlotCtl. We get NULL
> ReplicationSlotCtl during instance startup in
> check_standby_slot_names() as postmaster first loads GUC-table and
> then initializes shared-memory for replication slots. See calls of
> InitializeGUCOptions() and CreateSharedMemoryAndSemaphores() in
> PostmasterMain().  FWIW, I do not have any issue with current code as
> well, but if we have to change it, is [1] any better?
> 
> [1]:
> check_standby_slot_names()
> {
> ....
> if (!ok)
> {
>     GUC_check_errdetail("List syntax is invalid.");
> }
> else if (ReplicationSlotCtl)
> {
>    foreach-loop for slot validation
> }
> 
> pfree(rawname);
> list_free(elemlist);
> return ok;
> }
> 

Yeah thanks, it does not test the "ok" value twice and get rid of the goto
while checking the syntax first: I'd vote for it.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com



pgsql-hackers by date:

Previous
From: Andy Fan
Date:
Subject: Re: Shared detoast Datum proposal
Next
From: Amit Kapila
Date:
Subject: Re: Slow catchup of 2PC (twophase) transactions on replica in LR