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.kapil...@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