Re: Synchronizing slots from primary to standby

On Mon, Feb 26, 2024 at 5:18 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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;
}

thanks
SHveta