Re: Enhanced error message to include hint messages for redundant options error

On Fri, Apr 30, 2021 at 11:23 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Fri, Apr 30, 2021 at 11:09 AM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Fri, Apr 30, 2021 at 10:51 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > > On Fri, Apr 30, 2021 at 10:43 AM Bharath Rupireddy
> > > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > > >
> > > > On Fri, Apr 30, 2021 at 10:17 AM Dilip Kumar <dilipbalaut@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;

Thanks. Done that way, see the attached v3. Let's see what others has to say.

Also attaching Vignesh's v3 patch as-is, just for completion.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com