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

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.

And another good reason to remove the goto statements is that they
have return false; statements just to suppress the compiler and having
them after ereport(ERROR, doesn't make any sense to me.

duplicate_error:
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or redundant options"),
parser_errposition(pstate, defel->location)));
return false; /* keep compiler quiet */

procedure_error:
ereport(ERROR,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("invalid attribute in procedure definition"),
parser_errposition(pstate, defel->location)));
return false;

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