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

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