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

On Fri, Apr 30, 2021 at 5:06 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Fri, Apr 30, 2021 at 11:23 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > 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;
>
> Thanks. Done that way, see the attached v3. Let's see what others has to say.
>

Hmmm - I am not so sure about those goto replacements. I think the
poor goto has a bad reputation, but not all gotos are bad. I've met
some very nice gotos.

Each goto here was doing exactly what it looked like it was doing,
whereas all these boolean replacements have now introduced subtle
differences. e.g. now the *volatility_item = defel; assignment (and
all similar assignments) will happen which previously did not happen
at all. It leaves the reader wondering if assigning to those
references could have any side-effects at the caller. Probably there
are no problems at all....but can you be sure? Meanwhile, those
"inelegant" gotos did not give any cause for such doubts.

------
Kind Regards,
Peter Smith.
Fujitsu Australia