On Fri, Apr 30, 2021 at 11:09 AM Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com> wrote: > > On Fri, Apr 30, 2021 at 10:51 AM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > On Fri, Apr 30, 2021 at 10:43 AM Bharath Rupireddy > > <bharath.rupireddyforpostg...@gmail.com> wrote: > > > > > > On Fri, Apr 30, 2021 at 10:17 AM Dilip Kumar <dilipbal...@gmail.com> > > > wrote: > > > > In this function, we already have the "defel" variable then I do not > > > > understand why you are using one extra variable and assigning defel to > > > > that? > > > > If the goal is to just improve the error message then you can simply > > > > use defel->defname? > > > > > > Yeah. I can do that. Thanks for the comment. > > > > > > While on this, I also removed the duplicate_error and procedure_error > > > goto statements, because IMHO, using goto statements is not an elegant > > > way. I used boolean flags to do the job instead. See the attached and > > > let me know what you think. > > > > Okay, but I see one side effect of this, basically earlier on > > procedure_error and duplicate_error we were not assigning anything to > > output parameters, e.g. volatility_item, but now those values will be > > assigned with defel even if there is an error. > > Yes, but on ereport(ERROR, we don't come back right? The txn gets > aborted and the control is not returned to the caller instead it will > go to sigjmp_buf of the backend. > > > So I think we should > > better avoid such change. But even if you want to do then better > > check for any impacts on the caller. > > AFAICS, there will not be any impact on the caller, as the control > doesn't return to the caller on error.
I see. other comments if (strcmp(defel->defname, "volatility") == 0) { if (is_procedure) - goto procedure_error; + is_procedure_error = true; if (*volatility_item) - goto duplicate_error; + is_duplicate_error = true; Another side effect I see is, in the above check earlier if is_procedure was true it was directly goto procedure_error, but now it will also check the if (*volatility_item) and it may set is_duplicate_error also true, which seems wrong to me. I think you can change it to if (is_procedure) is_procedure_error = true; else if (*volatility_item) is_duplicate_error = true; -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com