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

Hi,

While reviewing one of the logical replication patches, I found that
we do not include hint messages to display the actual option which has
been specified more than once in case of redundant option error. I
felt including this will help in easily identifying the error, users
will not have to search through the statement to identify where the
actual error is present. Attached a patch for the same.
Thoughts?

Regards,
Vignesh

On Mon, Apr 26, 2021 at 5:29 PM vignesh C <vignesh21@gmail.com> wrote:
>
> Hi,
>
> While reviewing one of the logical replication patches, I found that
> we do not include hint messages to display the actual option which has
> been specified more than once in case of redundant option error. I
> felt including this will help in easily identifying the error, users
> will not have to search through the statement to identify where the
> actual error is present. Attached a patch for the same.
> Thoughts?

+1. A more detailed error will be useful in a rare scenario like users
have specified duplicate options along with a lot of other options,
they will know for which option the error is thrown by the server.

Instead of adding errhint or errdetail, how about just changing the
error message itself to something like "option \"%s\" specified more
than once" or "parameter \"%s\" specified more than once" like we have
in other places in the code?

Comments on the patch:

1) generally errhint and errdetail messages should end with a period
".", please see their usage in other places.
+                         errhint("Option \"streaming\" specified more
than once")));

2) I think it should be errdetail instead of errhint, because you are
giving more information about the error,  but not hinting user how to
overcome it. If you had to say something like "Remove duplicate
\"password\" option." or "The \"password\" option is specified more
than once. Remove all the duplicates.", then it makes sense to use
errhint.

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

On Mon, Apr 26, 2021 at 5:49 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Mon, Apr 26, 2021 at 5:29 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Hi,
> >
> > While reviewing one of the logical replication patches, I found that
> > we do not include hint messages to display the actual option which has
> > been specified more than once in case of redundant option error. I
> > felt including this will help in easily identifying the error, users
> > will not have to search through the statement to identify where the
> > actual error is present. Attached a patch for the same.
> > Thoughts?
>

+1 for improving the error

> Comments on the patch:
>
> 1) generally errhint and errdetail messages should end with a period
> ".", please see their usage in other places.
> +                         errhint("Option \"streaming\" specified more
> than once")));
>
> 2) I think it should be errdetail instead of errhint, because you are
> giving more information about the error,  but not hinting user how to
> overcome it. If you had to say something like "Remove duplicate
> \"password\" option." or "The \"password\" option is specified more
> than once. Remove all the duplicates.", then it makes sense to use
> errhint.

I agree this should be errdetail.


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Mon, Apr 26, 2021 at 5:49 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Mon, Apr 26, 2021 at 5:29 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Hi,
> >
> > While reviewing one of the logical replication patches, I found that
> > we do not include hint messages to display the actual option which has
> > been specified more than once in case of redundant option error. I
> > felt including this will help in easily identifying the error, users
> > will not have to search through the statement to identify where the
> > actual error is present. Attached a patch for the same.
> > Thoughts?
>
> +1. A more detailed error will be useful in a rare scenario like users
> have specified duplicate options along with a lot of other options,
> they will know for which option the error is thrown by the server.
>
> Instead of adding errhint or errdetail, how about just changing the
> error message itself to something like "option \"%s\" specified more
> than once" or "parameter \"%s\" specified more than once" like we have
> in other places in the code?
>

Both seemed fine but I preferred using errdetail as I felt it is
slightly better for the details to appear in a new line.

> Comments on the patch:
>
> 1) generally errhint and errdetail messages should end with a period
> ".", please see their usage in other places.
> +                         errhint("Option \"streaming\" specified more
> than once")));
>

Modified it.

> 2) I think it should be errdetail instead of errhint, because you are
> giving more information about the error,  but not hinting user how to
> overcome it. If you had to say something like "Remove duplicate
> \"password\" option." or "The \"password\" option is specified more
> than once. Remove all the duplicates.", then it makes sense to use
> errhint.

Modified it.

Attached patch for the same.

Regards,
Vignesh

On Mon, Apr 26, 2021 at 6:18 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Mon, Apr 26, 2021 at 5:49 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Mon, Apr 26, 2021 at 5:29 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > While reviewing one of the logical replication patches, I found that
> > > we do not include hint messages to display the actual option which has
> > > been specified more than once in case of redundant option error. I
> > > felt including this will help in easily identifying the error, users
> > > will not have to search through the statement to identify where the
> > > actual error is present. Attached a patch for the same.
> > > Thoughts?
> >
>
> +1 for improving the error
>
> > Comments on the patch:
> >
> > 1) generally errhint and errdetail messages should end with a period
> > ".", please see their usage in other places.
> > +                         errhint("Option \"streaming\" specified more
> > than once")));
> >
> > 2) I think it should be errdetail instead of errhint, because you are
> > giving more information about the error,  but not hinting user how to
> > overcome it. If you had to say something like "Remove duplicate
> > \"password\" option." or "The \"password\" option is specified more
> > than once. Remove all the duplicates.", then it makes sense to use
> > errhint.
>
> I agree this should be errdetail.

Thanks for the comment, I have modified and shared the v2 patch
attached in the previous mail.

Regards,
Vignesh

On Mon, Apr 26, 2021 at 7:02 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Mon, Apr 26, 2021 at 5:49 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Mon, Apr 26, 2021 at 5:29 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > While reviewing one of the logical replication patches, I found that
> > > we do not include hint messages to display the actual option which has
> > > been specified more than once in case of redundant option error. I
> > > felt including this will help in easily identifying the error, users
> > > will not have to search through the statement to identify where the
> > > actual error is present. Attached a patch for the same.
> > > Thoughts?
> >
> > +1. A more detailed error will be useful in a rare scenario like users
> > have specified duplicate options along with a lot of other options,
> > they will know for which option the error is thrown by the server.
> >
> > Instead of adding errhint or errdetail, how about just changing the
> > error message itself to something like "option \"%s\" specified more
> > than once" or "parameter \"%s\" specified more than once" like we have
> > in other places in the code?
> >
>
> Both seemed fine but I preferred using errdetail as I felt it is
> slightly better for the details to appear in a new line.

Thanks! IMO, it is better to change the error message to "option
\"%s\" specified more than once" instead of adding an error detail.
Let's hear other hackers' opinions.

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

On 2021-Apr-26, Bharath Rupireddy wrote:

> Thanks! IMO, it is better to change the error message to "option
> \"%s\" specified more than once" instead of adding an error detail.
> Let's hear other hackers' opinions.

Many other places have the message "conflicting or redundant options",
and then parser_errposition() shows the problem option.  That seems
pretty unhelpful, so whenever the problem is the redundancy I would have
the message be explicit about that, and about which option is the
problem:
  errmsg("option \"%s\" specified more than once", "someopt")
Do note that wording it this way means only one translatable message,
not dozens.

In some cases it is possible that you'd end up with two messages, one
for "redundant" and one for the other ways for options to conflict with
others; for example collationcmds.c has one that's not as obvious, and
force_quote/force_quote_all in COPY have their own thing too.

I think we should do parser_errposition() wherever possible, in
addition to the wording change.

-- 
Álvaro Herrera       Valdivia, Chile
<inflex> really, I see PHP as like a strange amalgamation of C, Perl, Shell
<crab> inflex: you know that "amalgam" means "mixture with mercury",
       more or less, right?
<crab> i.e., "deadly poison"

On Mon, Apr 26, 2021 at 8:06 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-Apr-26, Bharath Rupireddy wrote:
>
> > Thanks! IMO, it is better to change the error message to "option
> > \"%s\" specified more than once" instead of adding an error detail.
> > Let's hear other hackers' opinions.
>
> Many other places have the message "conflicting or redundant options",
> and then parser_errposition() shows the problem option.  That seems
> pretty unhelpful, so whenever the problem is the redundancy I would have
> the message be explicit about that, and about which option is the
> problem:
>   errmsg("option \"%s\" specified more than once", "someopt")
> Do note that wording it this way means only one translatable message,
> not dozens.

I agree that we can just be clear about the problem. Looks like the
majority of the errors "conflicting or redundant options" are for
redundant options. So, wherever "conflicting or redundant options"
exists: 1) change the message to "option \"%s\" specified more than
once" and remove parser_errposition if it's there because the option
name in the error message would give the info with which user can
point to the location 2) change the message to something like "option
\"%s\" is conflicting with option \"%s\"".

> In some cases it is possible that you'd end up with two messages, one
> for "redundant" and one for the other ways for options to conflict with
> others; for example collationcmds.c has one that's not as obvious, and

And yes, we need to divide up the message for conflicting and
redundant options on a case-to-case basis.

In createdb: we just need to modify the error message to "conflicting
options" or we could just get rid of errdetail and have the error
message directly saying "LOCALE cannot be specified together with
LC_COLLATE or LC_CTYPE". Redundant options are just caught in the
above for loop in createdb.
    if (dlocale && (dcollate || dctype))
        ereport(ERROR,
                (errcode(ERRCODE_SYNTAX_ERROR),
                 errmsg("conflicting or redundant options"),
                 errdetail("LOCALE cannot be specified together with
LC_COLLATE or LC_CTYPE.")));

In AlterDatabase: we can remove parser_errposition because the option
name in the error message would give the right information.
        if (list_length(stmt->options) != 1)
            ereport(ERROR,
                    (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                     errmsg("option \"%s\" cannot be specified with
other options",
                            dtablespace->defname),
                     parser_errposition(pstate, dtablespace->location)));

In compute_common_attribute: we can remove goto duplicate_error and
have the message change to "option \"%s\" specified more than once".

In DefineType: we need to rework for loop.
I found another problem with collationcmds.c is that it doesn't error
out if some of the options are specified more than once, something
like below. I think the option checking "for loop" in DefineCollation
needs to be reworked.
CREATE COLLATION case_insensitive (provider = icu, provider =
someother locale = '@colStrength=secondary', deterministic = false,
deterministic = true);

> force_quote/force_quote_all in COPY have their own thing too.

We can remove the errhint for force_not_null and force_null along with
the error message wording change to "option \"%s\" specified more than
once".

Upon looking at error "conflicting or redundant options" instances, to
do the above we need a lot of code changes, I'm not sure that will be
acceptable.

One thing is that all the option checking for loops are doing these
things in common: 1) fetching the values bool, int, float, string of
the options 2) redundant checking. I feel we need to invent a common
API to which we pass in 1) a list of allowed options for a particular
command, we can have these as static data structure
{allowed_option_name, data_type}, 2) a list of user specified options
3) the API will return a list of fetched i.e. parsed values
{user_specified_option_name, data_type, value}. Maybe the API can
return a hash table of these values so that the callers can look up
faster for the required option. The advantage of this API is that we
don't need to have many for-loops for options checking in the code.
I'm not sure it is worth doing though. Thoughts?

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

On 2021-Apr-26, Bharath Rupireddy wrote:

> I agree that we can just be clear about the problem. Looks like the
> majority of the errors "conflicting or redundant options" are for
> redundant options. So, wherever "conflicting or redundant options"
> exists: 1) change the message to "option \"%s\" specified more than
> once" and remove parser_errposition if it's there because the option
> name in the error message would give the info with which user can
> point to the location

Hmm, I would keep the parser_errposition() even if the option name is
mentioned in the error message.  There's no harm in being a little
redundant, with both the option name and the error cursor showing the
same thing.

> 2) change the message to something like "option \"%s\" is conflicting
> with option \"%s\"".

Maybe, but since these would all be special cases, I think we'd need to
discuss them individually.  I would suggest that in order not to stall
this patch, these cases should all stay as "redundant or conflicting
options" -- that is, avoid any further change apart from exactly the
thing you came here to change.  You can submit a 0002 patch to change
those other errors.  That way, even if those changes end up rejected for
whatever reason, you still got your 0001 done (which would change the
bulk of "conflicting or redundant" error to the "option %s already
specified" error).  Some progress is better than none.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
<Schwern> It does it in a really, really complicated way
<crab> why does it need to be complicated?
<Schwern> Because it's MakeMaker.

On Mon, Apr 26, 2021 at 9:24 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-Apr-26, Bharath Rupireddy wrote:
>
> > I agree that we can just be clear about the problem. Looks like the
> > majority of the errors "conflicting or redundant options" are for
> > redundant options. So, wherever "conflicting or redundant options"
> > exists: 1) change the message to "option \"%s\" specified more than
> > once" and remove parser_errposition if it's there because the option
> > name in the error message would give the info with which user can
> > point to the location
>
> Hmm, I would keep the parser_errposition() even if the option name is
> mentioned in the error message.  There's no harm in being a little
> redundant, with both the option name and the error cursor showing the
> same thing.

Agreed.

> > 2) change the message to something like "option \"%s\" is conflicting
> > with option \"%s\"".
>
> Maybe, but since these would all be special cases, I think we'd need to
> discuss them individually.  I would suggest that in order not to stall
> this patch, these cases should all stay as "redundant or conflicting
> options" -- that is, avoid any further change apart from exactly the
> thing you came here to change.  You can submit a 0002 patch to change
> those other errors.  That way, even if those changes end up rejected for
> whatever reason, you still got your 0001 done (which would change the
> bulk of "conflicting or redundant" error to the "option %s already
> specified" error).  Some progress is better than none.

+1 to have all the conflicting options error message changes as 0002
patch or I'm okay even if we discuss those changes after the 0001
patch goes in.

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

On Mon, Apr 26, 2021 at 9:10 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> I found another problem with collationcmds.c is that it doesn't error
> out if some of the options are specified more than once, something
> like below. I think the option checking "for loop" in DefineCollation
> needs to be reworked.
> CREATE COLLATION case_insensitive (provider = icu, provider =
> someother locale = '@colStrength=secondary', deterministic = false,
> deterministic = true);

I'm thinking that the above problem should be discussed separately. I
will start a new thread soon on this.

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

On Tue, Apr 27, 2021 at 6:23 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Mon, Apr 26, 2021 at 9:10 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > I found another problem with collationcmds.c is that it doesn't error
> > out if some of the options are specified more than once, something
> > like below. I think the option checking "for loop" in DefineCollation
> > needs to be reworked.
> > CREATE COLLATION case_insensitive (provider = icu, provider =
> > someother locale = '@colStrength=secondary', deterministic = false,
> > deterministic = true);
>
> I'm thinking that the above problem should be discussed separately. I
> will start a new thread soon on this.

I started a separate thread -
https://www.postgresql.org/message-id/CALj2ACWtL6fTLdyF4R_YkPtf1YEDb6FUoD5DGAki3rpD%2BsWqiA%40mail.gmail.com

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

On Mon, Apr 26, 2021 at 9:24 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-Apr-26, Bharath Rupireddy wrote:
>
> > I agree that we can just be clear about the problem. Looks like the
> > majority of the errors "conflicting or redundant options" are for
> > redundant options. So, wherever "conflicting or redundant options"
> > exists: 1) change the message to "option \"%s\" specified more than
> > once" and remove parser_errposition if it's there because the option
> > name in the error message would give the info with which user can
> > point to the location
>
> Hmm, I would keep the parser_errposition() even if the option name is
> mentioned in the error message.  There's no harm in being a little
> redundant, with both the option name and the error cursor showing the
> same thing.
>
> > 2) change the message to something like "option \"%s\" is conflicting
> > with option \"%s\"".
>
> Maybe, but since these would all be special cases, I think we'd need to
> discuss them individually.  I would suggest that in order not to stall
> this patch, these cases should all stay as "redundant or conflicting
> options" -- that is, avoid any further change apart from exactly the
> thing you came here to change.  You can submit a 0002 patch to change
> those other errors.  That way, even if those changes end up rejected for
> whatever reason, you still got your 0001 done (which would change the
> bulk of "conflicting or redundant" error to the "option %s already
> specified" error).  Some progress is better than none.

Thanks for the comments, please find the attached v3 patch which has
the change for the first part. I will make changes for 002 and post it
soon.
Thoughts?

Regards,
Vignesh

On 2021-Apr-29, vignesh C wrote:

> Thanks for the comments, please find the attached v3 patch which has
> the change for the first part.

Looks good to me.  I would only add parser_errposition() to the few
error sites missing that.



-- 
Álvaro Herrera                            39°49'30"S 73°17'W
"Uno puede defenderse de los ataques; contra los elogios se esta indefenso"

On Thu, Apr 29, 2021 at 10:44 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-Apr-29, vignesh C wrote:
>
> > Thanks for the comments, please find the attached v3 patch which has
> > the change for the first part.
>
> Looks good to me.  I would only add parser_errposition() to the few
> error sites missing that.

Yes, we need to add parser_errposition as agreed in [1].

I think we will have to make changes in compute_common_attribute as
well because the error in the duplicate_error goto statement is
actually for the duplicate option specified more than once, we can do
something like the attached. If it seems okay, it can be merged with
the main patch.

[1] -
https://www.postgresql.org/message-id/flat/CALj2ACUa%3DZM8QtOLPCHc7%3DWgFrx9P6-AgKQs8cmKLvNCvu7arQ%40mail.gmail.com

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

On Fri, Apr 30, 2021 at 8:16 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Thu, Apr 29, 2021 at 10:44 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > On 2021-Apr-29, vignesh C wrote:
> >
> > > Thanks for the comments, please find the attached v3 patch which has
> > > the change for the first part.
> >
> > Looks good to me.  I would only add parser_errposition() to the few
> > error sites missing that.
>
> Yes, we need to add parser_errposition as agreed in [1].
>
> I think we will have to make changes in compute_common_attribute as
> well because the error in the duplicate_error goto statement is
> actually for the duplicate option specified more than once, we can do
> something like the attached. If it seems okay, it can be merged with
> the main patch.

+ DefElem *duplicate_item = NULL;
+
  if (strcmp(defel->defname, "volatility") == 0)
  {
  if (is_procedure)
  goto procedure_error;
  if (*volatility_item)
- goto duplicate_error;
+ duplicate_item = defel;

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?

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

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.

Just for completion, I also attached Vignesh's latest patch v3 as-is,
in case anybody wants to review it.

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

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.  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.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

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.

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

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;


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

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

On Fri, Apr 30, 2021 at 5:06 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> 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.
>

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

On Fri, Apr 30, 2021 at 12:36 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > 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.

Looking into this again, why not as shown below?  IMHO, this way the
code will be logically the same as it was before the patch, basically
why to process an extra statement ( *volatility_item = defel;) if we
have already decided to error.

 if (is_procedure)
    is_procedure_error =  true;
else if (*volatility_item)
  is_duplicate_error = true;
else
  *volatility_item = defel;

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Fri, Apr 30, 2021 at 2:49 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> Looking into this again, why not as shown below?  IMHO, this way the
> code will be logically the same as it was before the patch, basically
> why to process an extra statement ( *volatility_item = defel;) if we
> have already decided to error.

I changed my mind given the concerns raised on removing the goto
statements. We could just do as below:

diff --git a/src/backend/commands/functioncmds.c
b/src/backend/commands/functioncmds.c
index 9548287217..1f1c74c379 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -575,7 +575,7 @@ compute_common_attribute(ParseState *pstate,
 duplicate_error:
     ereport(ERROR,
             (errcode(ERRCODE_SYNTAX_ERROR),
-             errmsg("conflicting or redundant options"),
+             errmsg("option \"%s\" specified more than once", defel->defname),
              parser_errposition(pstate, defel->location)));
     return false;                /* keep compiler quiet */

I'm not attaching above one line change as a patch, maybe Vignesh can
merge this into the main patch.

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

On Sat, May 1, 2021 at 10:43 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Fri, Apr 30, 2021 at 2:49 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > Looking into this again, why not as shown below?  IMHO, this way the
> > code will be logically the same as it was before the patch, basically
> > why to process an extra statement ( *volatility_item = defel;) if we
> > have already decided to error.
>
> I changed my mind given the concerns raised on removing the goto
> statements. We could just do as below:

Okay, that make sense.

> diff --git a/src/backend/commands/functioncmds.c
> b/src/backend/commands/functioncmds.c
> index 9548287217..1f1c74c379 100644
> --- a/src/backend/commands/functioncmds.c
> +++ b/src/backend/commands/functioncmds.c
> @@ -575,7 +575,7 @@ compute_common_attribute(ParseState *pstate,
>  duplicate_error:
>      ereport(ERROR,
>              (errcode(ERRCODE_SYNTAX_ERROR),
> -             errmsg("conflicting or redundant options"),
> +             errmsg("option \"%s\" specified more than once", defel->defname),
>               parser_errposition(pstate, defel->location)));
>      return false;                /* keep compiler quiet */
>
> I'm not attaching above one line change as a patch, maybe Vignesh can
> merge this into the main patch.

+1

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Sat, May 1, 2021 at 10:47 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Sat, May 1, 2021 at 10:43 AM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Fri, Apr 30, 2021 at 2:49 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > Looking into this again, why not as shown below?  IMHO, this way the
> > > code will be logically the same as it was before the patch, basically
> > > why to process an extra statement ( *volatility_item = defel;) if we
> > > have already decided to error.
> >
> > I changed my mind given the concerns raised on removing the goto
> > statements. We could just do as below:
>
> Okay, that make sense.
>
> > diff --git a/src/backend/commands/functioncmds.c
> > b/src/backend/commands/functioncmds.c
> > index 9548287217..1f1c74c379 100644
> > --- a/src/backend/commands/functioncmds.c
> > +++ b/src/backend/commands/functioncmds.c
> > @@ -575,7 +575,7 @@ compute_common_attribute(ParseState *pstate,
> >  duplicate_error:
> >      ereport(ERROR,
> >              (errcode(ERRCODE_SYNTAX_ERROR),
> > -             errmsg("conflicting or redundant options"),
> > +             errmsg("option \"%s\" specified more than once", defel->defname),
> >               parser_errposition(pstate, defel->location)));
> >      return false;                /* keep compiler quiet */
> >
> > I'm not attaching above one line change as a patch, maybe Vignesh can
> > merge this into the main patch.

Thanks for the comments. I have merged the change into the attached patch.
Thoughts?

Regards,
Vignesh

On Thu, Apr 29, 2021 at 10:44 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-Apr-29, vignesh C wrote:
>
> > Thanks for the comments, please find the attached v3 patch which has
> > the change for the first part.
>
> Looks good to me.  I would only add parser_errposition() to the few
> error sites missing that.

I have not included parser_errposition as ParseState was not available
for these errors.
Thoughts?

Regards,
Vignesh

On Sat, May 1, 2021 at 7:25 PM vignesh C <vignesh21@gmail.com> wrote:
> > > I'm not attaching above one line change as a patch, maybe Vignesh can
> > > merge this into the main patch.
>
> Thanks for the comments. I have merged the change into the attached patch.
> Thoughts?

Thanks! v4 basically LGTM. Can we park this in the current commitfest
if not done already?

Upon looking at the number of places where we have the "option \"%s\"
specified more than once" error, I, now strongly feel that we should
use goto duplicate_error approach like in compute_common_attribute, so
that we will have only one ereport(ERROR. We can change it in
following files: copy.c, dbcommands.c, extension.c,
compute_function_attributes, sequence.c, subscriptioncmds.c,
typecmds.c, user.c, walsender.c, pgoutput.c. This will reduce the LOC
greatly.

Thoughts?

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

On 2021-May-01, vignesh C wrote:

> On Thu, Apr 29, 2021 at 10:44 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > On 2021-Apr-29, vignesh C wrote:
> >
> > > Thanks for the comments, please find the attached v3 patch which has
> > > the change for the first part.
> >
> > Looks good to me.  I would only add parser_errposition() to the few
> > error sites missing that.
> 
> I have not included parser_errposition as ParseState was not available
> for these errors.

Yeah, it's tough to do that in a few of those such as validator
functions, and I don't think we'd want to do that.  However there are
some cases where we can easily add the parsestate as an argument -- for
example CreatePublication can get it in ProcessUtilitySlow and pass it
down to parse_publication_options; likewise for ExecuteDoStmt.  I didn't
check other places.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W

On 2021-May-01, Bharath Rupireddy wrote:

> IMO, it's not good to change the function API just for showing up
> parse_position (which is there for cosmetic reasons I feel) in an error
> which actually has the option name clearly mentioned in the error message.

Why not?  We've done it before, I'm sure you can find examples in the
git log.  The function API is not that critical -- these functions are
mostly only called from ProcessUtility and friends.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W

On Sun, May 2, 2021 at 4:27 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-May-01, Bharath Rupireddy wrote:
>
> > IMO, it's not good to change the function API just for showing up
> > parse_position (which is there for cosmetic reasons I feel) in an error
> > which actually has the option name clearly mentioned in the error message.
>
> Why not?  We've done it before, I'm sure you can find examples in the
> git log.  The function API is not that critical -- these functions are
> mostly only called from ProcessUtility and friends.

I feel it is better to include it wherever possible.

Regards,
Vignesh

On Sat, May 1, 2021 at 10:54 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-May-01, vignesh C wrote:
>
> > On Thu, Apr 29, 2021 at 10:44 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > >
> > > On 2021-Apr-29, vignesh C wrote:
> > >
> > > > Thanks for the comments, please find the attached v3 patch which has
> > > > the change for the first part.
> > >
> > > Looks good to me.  I would only add parser_errposition() to the few
> > > error sites missing that.
> >
> > I have not included parser_errposition as ParseState was not available
> > for these errors.
>
> Yeah, it's tough to do that in a few of those such as validator
> functions, and I don't think we'd want to do that.  However there are
> some cases where we can easily add the parsestate as an argument -- for
> example CreatePublication can get it in ProcessUtilitySlow and pass it
> down to parse_publication_options; likewise for ExecuteDoStmt.  I didn't
> check other places.

Thanks for the comments. I have changed in most of the places except
for a few places like plugin functions, internal commands and changes
that required changing more levels of function callers. Attached patch
has the changes for the same.
Thoughts?

Regards,
Vignesh

On Sat, May 1, 2021 at 9:02 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Sat, May 1, 2021 at 7:25 PM vignesh C <vignesh21@gmail.com> wrote:
> > > > I'm not attaching above one line change as a patch, maybe Vignesh can
> > > > merge this into the main patch.
> >
> > Thanks for the comments. I have merged the change into the attached patch.
> > Thoughts?
>
> Thanks! v4 basically LGTM. Can we park this in the current commitfest
> if not done already?
>
> Upon looking at the number of places where we have the "option \"%s\"
> specified more than once" error, I, now strongly feel that we should
> use goto duplicate_error approach like in compute_common_attribute, so
> that we will have only one ereport(ERROR. We can change it in
> following files: copy.c, dbcommands.c, extension.c,
> compute_function_attributes, sequence.c, subscriptioncmds.c,
> typecmds.c, user.c, walsender.c, pgoutput.c. This will reduce the LOC
> greatly.
>
> Thoughts?

I have made the changes for this, I have posted the same in the v5
patch posted in my earlier mail.

Regards,
Vignesh

On Sun, May 2, 2021 at 8:44 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Sat, May 1, 2021 at 9:02 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Sat, May 1, 2021 at 7:25 PM vignesh C <vignesh21@gmail.com> wrote:
> > > > > I'm not attaching above one line change as a patch, maybe Vignesh can
> > > > > merge this into the main patch.
> > >
> > > Thanks for the comments. I have merged the change into the attached patch.
> > > Thoughts?
> >
> > Thanks! v4 basically LGTM. Can we park this in the current commitfest
> > if not done already?
> >
> > Upon looking at the number of places where we have the "option \"%s\"
> > specified more than once" error, I, now strongly feel that we should
> > use goto duplicate_error approach like in compute_common_attribute, so
> > that we will have only one ereport(ERROR. We can change it in
> > following files: copy.c, dbcommands.c, extension.c,
> > compute_function_attributes, sequence.c, subscriptioncmds.c,
> > typecmds.c, user.c, walsender.c, pgoutput.c. This will reduce the LOC
> > greatly.
> >
> > Thoughts?
>
> I have made the changes for this, I have posted the same in the v5
> patch posted in my earlier mail.

Thanks! The v5 patch looks good to me. Let's see if all agree on the
goto duplicate_error approach which could reduce the LOC by ~80.

I don't see it in the current commitfest, can we park it there so that
the patch will get tested on cfbot systems?

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

On Mon, May 3, 2021 at 12:08 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Sun, May 2, 2021 at 8:44 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Sat, May 1, 2021 at 9:02 PM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > >
> > > On Sat, May 1, 2021 at 7:25 PM vignesh C <vignesh21@gmail.com> wrote:
> > > > > > I'm not attaching above one line change as a patch, maybe Vignesh can
> > > > > > merge this into the main patch.
> > > >
> > > > Thanks for the comments. I have merged the change into the attached patch.
> > > > Thoughts?
> > >
> > > Thanks! v4 basically LGTM. Can we park this in the current commitfest
> > > if not done already?
> > >
> > > Upon looking at the number of places where we have the "option \"%s\"
> > > specified more than once" error, I, now strongly feel that we should
> > > use goto duplicate_error approach like in compute_common_attribute, so
> > > that we will have only one ereport(ERROR. We can change it in
> > > following files: copy.c, dbcommands.c, extension.c,
> > > compute_function_attributes, sequence.c, subscriptioncmds.c,
> > > typecmds.c, user.c, walsender.c, pgoutput.c. This will reduce the LOC
> > > greatly.
> > >
> > > Thoughts?
> >
> > I have made the changes for this, I have posted the same in the v5
> > patch posted in my earlier mail.
>
> Thanks! The v5 patch looks good to me. Let's see if all agree on the
> goto duplicate_error approach which could reduce the LOC by ~80.

I think the "goto duplicate_error" approach looks good, it avoids
duplicating the same error code multiple times.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Mon, May 3, 2021 at 12:08 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Sun, May 2, 2021 at 8:44 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Sat, May 1, 2021 at 9:02 PM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > >
> > > On Sat, May 1, 2021 at 7:25 PM vignesh C <vignesh21@gmail.com> wrote:
> > > > > > I'm not attaching above one line change as a patch, maybe Vignesh can
> > > > > > merge this into the main patch.
> > > >
> > > > Thanks for the comments. I have merged the change into the attached patch.
> > > > Thoughts?
> > >
> > > Thanks! v4 basically LGTM. Can we park this in the current commitfest
> > > if not done already?
> > >
> > > Upon looking at the number of places where we have the "option \"%s\"
> > > specified more than once" error, I, now strongly feel that we should
> > > use goto duplicate_error approach like in compute_common_attribute, so
> > > that we will have only one ereport(ERROR. We can change it in
> > > following files: copy.c, dbcommands.c, extension.c,
> > > compute_function_attributes, sequence.c, subscriptioncmds.c,
> > > typecmds.c, user.c, walsender.c, pgoutput.c. This will reduce the LOC
> > > greatly.
> > >
> > > Thoughts?
> >
> > I have made the changes for this, I have posted the same in the v5
> > patch posted in my earlier mail.
>
> Thanks! The v5 patch looks good to me. Let's see if all agree on the
> goto duplicate_error approach which could reduce the LOC by ~80.
>
> I don't see it in the current commitfest, can we park it there so that
> the patch will get tested on cfbot systems?

I have added an entry in commitfest:
https://commitfest.postgresql.org/33/3103/

Regards,
Vignesh

On Mon, May 3, 2021 at 1:41 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Mon, May 3, 2021 at 12:08 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Sun, May 2, 2021 at 8:44 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Sat, May 1, 2021 at 9:02 PM Bharath Rupireddy
> > > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > > >
> > > > On Sat, May 1, 2021 at 7:25 PM vignesh C <vignesh21@gmail.com> wrote:
> > > > > > > I'm not attaching above one line change as a patch, maybe Vignesh can
> > > > > > > merge this into the main patch.
> > > > >
> > > > > Thanks for the comments. I have merged the change into the attached patch.
> > > > > Thoughts?
> > > >
> > > > Thanks! v4 basically LGTM. Can we park this in the current commitfest
> > > > if not done already?
> > > >
> > > > Upon looking at the number of places where we have the "option \"%s\"
> > > > specified more than once" error, I, now strongly feel that we should
> > > > use goto duplicate_error approach like in compute_common_attribute, so
> > > > that we will have only one ereport(ERROR. We can change it in
> > > > following files: copy.c, dbcommands.c, extension.c,
> > > > compute_function_attributes, sequence.c, subscriptioncmds.c,
> > > > typecmds.c, user.c, walsender.c, pgoutput.c. This will reduce the LOC
> > > > greatly.
> > > >
> > > > Thoughts?
> > >
> > > I have made the changes for this, I have posted the same in the v5
> > > patch posted in my earlier mail.
> >
> > Thanks! The v5 patch looks good to me. Let's see if all agree on the
> > goto duplicate_error approach which could reduce the LOC by ~80.
>
> I think the "goto duplicate_error" approach looks good, it avoids
> duplicating the same error code multiple times.

Thanks. I will mark the v5 patch "ready for committer" if no one has comments.

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

> > > Thanks! The v5 patch looks good to me. Let's see if all agree on the
> > > goto duplicate_error approach which could reduce the LOC by ~80.
> >
> > I think the "goto duplicate_error" approach looks good, it avoids
> > duplicating the same error code multiple times.
> 
> Thanks. I will mark the v5 patch "ready for committer" if no one has comments.

Hi,

I looked into the patch and noticed a minor thing.

+    return;                /* keep compiler quiet */
 }

I think we do not need the comment here.
The compiler seems not require "return" at the end of function
when function's return type is VOID.

In addition, it seems better to remove these "return;" like what
commit "3974c4" did.

Best regards,
houzj

On Sat, May 8, 2021 at 12:01 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> > > > Thanks! The v5 patch looks good to me. Let's see if all agree on the
> > > > goto duplicate_error approach which could reduce the LOC by ~80.
> > >
> > > I think the "goto duplicate_error" approach looks good, it avoids
> > > duplicating the same error code multiple times.
> >
> > Thanks. I will mark the v5 patch "ready for committer" if no one has comments.
>
> Hi,
>
> I looked into the patch and noticed a minor thing.
>
> +       return;                         /* keep compiler quiet */
>  }
>
> I think we do not need the comment here.
> The compiler seems not require "return" at the end of function
> when function's return type is VOID.
>
> In addition, it seems better to remove these "return;" like what
> commit "3974c4" did.

It looks like that commit removed the plain return statements for a
void returning functions. I see in the code that there are return
statements that are there right after ereport(ERROR, just to keep the
compiler quiet. Here in this patch also, we have return; statements
after ereport(ERROR, for void returning functions. I'm not sure
removing them would cause some compiler warnings on some platforms
with some other compilers. If we're not sure, I'm okay to keep those
return; statements. Thoughts?

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

On Sat, May 8, 2021 at 2:20 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Sat, May 8, 2021 at 12:01 PM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> > > > > Thanks! The v5 patch looks good to me. Let's see if all agree on the
> > > > > goto duplicate_error approach which could reduce the LOC by ~80.
> > > >
> > > > I think the "goto duplicate_error" approach looks good, it avoids
> > > > duplicating the same error code multiple times.
> > >
> > > Thanks. I will mark the v5 patch "ready for committer" if no one has comments.
> >
> > Hi,
> >
> > I looked into the patch and noticed a minor thing.
> >
> > +       return;                         /* keep compiler quiet */
> >  }
> >
> > I think we do not need the comment here.
> > The compiler seems not require "return" at the end of function
> > when function's return type is VOID.
> >
> > In addition, it seems better to remove these "return;" like what
> > commit "3974c4" did.
>
> It looks like that commit removed the plain return statements for a
> void returning functions. I see in the code that there are return
> statements that are there right after ereport(ERROR, just to keep the
> compiler quiet. Here in this patch also, we have return; statements
> after ereport(ERROR, for void returning functions. I'm not sure
> removing them would cause some compiler warnings on some platforms
> with some other compilers. If we're not sure, I'm okay to keep those
> return; statements. Thoughts?

I felt we could retain the return statement and remove the comments.
If you are ok with that I will modify and post a patch for it.
Thoughts?

Regards,
Vignesh

On Sat, May 8, 2021 at 7:06 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Sat, May 8, 2021 at 2:20 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Sat, May 8, 2021 at 12:01 PM houzj.fnst@fujitsu.com
> > <houzj.fnst@fujitsu.com> wrote:
> > >
> > > > > > Thanks! The v5 patch looks good to me. Let's see if all agree on the
> > > > > > goto duplicate_error approach which could reduce the LOC by ~80.
> > > > >
> > > > > I think the "goto duplicate_error" approach looks good, it avoids
> > > > > duplicating the same error code multiple times.
> > > >
> > > > Thanks. I will mark the v5 patch "ready for committer" if no one has comments.
> > >
> > > Hi,
> > >
> > > I looked into the patch and noticed a minor thing.
> > >
> > > +       return;                         /* keep compiler quiet */
> > >  }
> > >
> > > I think we do not need the comment here.
> > > The compiler seems not require "return" at the end of function
> > > when function's return type is VOID.
> > >
> > > In addition, it seems better to remove these "return;" like what
> > > commit "3974c4" did.
> >
> > It looks like that commit removed the plain return statements for a
> > void returning functions. I see in the code that there are return
> > statements that are there right after ereport(ERROR, just to keep the
> > compiler quiet. Here in this patch also, we have return; statements
> > after ereport(ERROR, for void returning functions. I'm not sure
> > removing them would cause some compiler warnings on some platforms
> > with some other compilers. If we're not sure, I'm okay to keep those
> > return; statements. Thoughts?
>
> I felt we could retain the return statement and remove the comments.
> If you are ok with that I will modify and post a patch for it.
> Thoughts?

I would like to keep it as is i.e. both return statement and /* keep
compiler quiet */ comment. Having said that, it's better to leave it
to the committer on whether to have the return statement at all.

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

> > > > > > > Thanks! The v5 patch looks good to me. Let's see if all
> > > > > > > agree on the goto duplicate_error approach which could reduce
> the LOC by ~80.
> > > > > >
> > > > > > I think the "goto duplicate_error" approach looks good, it
> > > > > > avoids duplicating the same error code multiple times.
> > > > >
> > > > > Thanks. I will mark the v5 patch "ready for committer" if no one has
> comments.
> > > >
> > > > Hi,
> > > >
> > > > I looked into the patch and noticed a minor thing.
> > > >
> > > > +       return;                         /* keep compiler quiet */
> > > >  }
> > > >
> > > > I think we do not need the comment here.
> > > > The compiler seems not require "return" at the end of function
> > > > when function's return type is VOID.
> > > >
> > > > In addition, it seems better to remove these "return;" like what
> > > > commit "3974c4" did.
> > >
> > > It looks like that commit removed the plain return statements for a
> > > void returning functions. I see in the code that there are return
> > > statements that are there right after ereport(ERROR, just to keep
> > > the compiler quiet. Here in this patch also, we have return;
> > > statements after ereport(ERROR, for void returning functions. I'm
> > > not sure removing them would cause some compiler warnings on some
> > > platforms with some other compilers. If we're not sure, I'm okay to
> > > keep those return; statements. Thoughts?
> >
> > I felt we could retain the return statement and remove the comments.
> > If you are ok with that I will modify and post a patch for it.
> > Thoughts?
> 
> I would like to keep it as is i.e. both return statement and /* keep compiler
> quiet */ comment. Having said that, it's better to leave it to the committer on
> whether to have the return statement at all.

Yes, it's better to leave it to the committer on whether to have the "return;".
But, I think at least removing "return;" which is at the *end* of the function will not cause any warning.
Such as:

+       return;                         /* keep compiler quiet */
}

So, I'd vote for at least removing the comment " keep the compiler quiet ".

Best regards,
houzj

On Mon, May 10, 2021 at 6:00 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> > > > > > > > Thanks! The v5 patch looks good to me. Let's see if all
> > > > > > > > agree on the goto duplicate_error approach which could reduce
> > the LOC by ~80.
> > > > > > >
> > > > > > > I think the "goto duplicate_error" approach looks good, it
> > > > > > > avoids duplicating the same error code multiple times.
> > > > > >
> > > > > > Thanks. I will mark the v5 patch "ready for committer" if no one has
> > comments.
> > > > >
> > > > > Hi,
> > > > >
> > > > > I looked into the patch and noticed a minor thing.
> > > > >
> > > > > +       return;                         /* keep compiler quiet */
> > > > >  }
> > > > >
> > > > > I think we do not need the comment here.
> > > > > The compiler seems not require "return" at the end of function
> > > > > when function's return type is VOID.
> > > > >
> > > > > In addition, it seems better to remove these "return;" like what
> > > > > commit "3974c4" did.
> > > >
> > > > It looks like that commit removed the plain return statements for a
> > > > void returning functions. I see in the code that there are return
> > > > statements that are there right after ereport(ERROR, just to keep
> > > > the compiler quiet. Here in this patch also, we have return;
> > > > statements after ereport(ERROR, for void returning functions. I'm
> > > > not sure removing them would cause some compiler warnings on some
> > > > platforms with some other compilers. If we're not sure, I'm okay to
> > > > keep those return; statements. Thoughts?
> > >
> > > I felt we could retain the return statement and remove the comments.
> > > If you are ok with that I will modify and post a patch for it.
> > > Thoughts?
> >
> > I would like to keep it as is i.e. both return statement and /* keep compiler
> > quiet */ comment. Having said that, it's better to leave it to the committer on
> > whether to have the return statement at all.
>
> Yes, it's better to leave it to the committer on whether to have the "return;".
> But, I think at least removing "return;" which is at the *end* of the function will not cause any warning.
> Such as:
>
> +       return;                         /* keep compiler quiet */
> }
>
> So, I'd vote for at least removing the comment " keep the compiler quiet ".

That sounds fine to me, Attached v6 patch which has the changes for the same.

Regards,
Vignesh

On 2021-May-10, vignesh C wrote:

> That sounds fine to me, Attached v6 patch which has the changes for the same.

What about defining a function (maybe a static inline function in
defrem.h) that is marked noreturn and receives the DefElem and
optionally pstate, and throws the error?  I think that would avoid the
patch's need to have half a dozen copies of the "duplicate_error:" label
and ereport stanza.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W

On Tue, May 11, 2021 at 2:47 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-May-10, vignesh C wrote:
>
> > That sounds fine to me, Attached v6 patch which has the changes for the same.
>
> What about defining a function (maybe a static inline function in
> defrem.h) that is marked noreturn and receives the DefElem and
> optionally pstate, and throws the error?  I think that would avoid the
> patch's need to have half a dozen copies of the "duplicate_error:" label
> and ereport stanza.

+1 to have a static inline function which just reports the error.

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

On Tue, May 11, 2021 at 2:47 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-May-10, vignesh C wrote:
>
> > That sounds fine to me, Attached v6 patch which has the changes for the same.
>
> What about defining a function (maybe a static inline function in
> defrem.h) that is marked noreturn and receives the DefElem and
> optionally pstate, and throws the error?  I think that would avoid the
> patch's need to have half a dozen copies of the "duplicate_error:" label
> and ereport stanza.

Thanks for the comment, this reduces a significant amount of code.
Attached patch has the changes incorporated.

Regards,
Vignesh

On Tue, May 11, 2021 at 6:54 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, May 11, 2021 at 2:47 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > On 2021-May-10, vignesh C wrote:
> >
> > > That sounds fine to me, Attached v6 patch which has the changes for the same.
> >
> > What about defining a function (maybe a static inline function in
> > defrem.h) that is marked noreturn and receives the DefElem and
> > optionally pstate, and throws the error?  I think that would avoid the
> > patch's need to have half a dozen copies of the "duplicate_error:" label
> > and ereport stanza.
>
> Thanks for the comment, this reduces a significant amount of code.

Yeah, the patch reduces more than 200 LOC which is a  pretty good thing.

 25 files changed, 239 insertions(+), 454 deletions(-)

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

You can avoid duplicating the ereport like this:

+               ereport(ERROR,
+                               (errcode(ERRCODE_SYNTAX_ERROR),
+                                errmsg("option \"%s\" specified more than once", defel->defname),
+                                parser ? parser_errposition(pstate, defel->location) : 0));

... also, since e3a87b4991cc you can now elide the parens around the
auxiliary function calls:

+        ereport(ERROR,
+                errcode(ERRCODE_SYNTAX_ERROR),
+                errmsg("option \"%s\" specified more than once", defel->defname),
+                parser ? parser_errposition(pstate, defel->location) : 0));

Please do add a pg_attribute_noreturn() decorator.  I'm not sure if any
compilers will complain about the code flow if you have that, but I
expect many (all?) will if you don't.

-- 
Álvaro Herrera       Valdivia, Chile
"Java is clearly an example of money oriented programming"  (A. Stepanov)

On Thu, May 13, 2021 at 4:58 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> You can avoid duplicating the ereport like this:
>
> +               ereport(ERROR,
> +                               (errcode(ERRCODE_SYNTAX_ERROR),
> +                                errmsg("option \"%s\" specified more than once", defel->defname),
> +                                parser ? parser_errposition(pstate, defel->location) : 0));
>
> ... also, since e3a87b4991cc you can now elide the parens around the
> auxiliary function calls:
>

Modified.

> +        ereport(ERROR,
> +                errcode(ERRCODE_SYNTAX_ERROR),
> +                errmsg("option \"%s\" specified more than once", defel->defname),
> +                parser ? parser_errposition(pstate, defel->location) : 0));
>
> Please do add a pg_attribute_noreturn() decorator.  I'm not sure if any
> compilers will complain about the code flow if you have that, but I
> expect many (all?) will if you don't.

Modified.

Thanks for the comments, Attached patch has the changes for the same.

Regards,
Vignesh

On Thu, May 13, 2021 at 8:09 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Thu, May 13, 2021 at 4:58 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
>
> Thanks for the comments, Attached patch has the changes for the same.
>

The Patch was not applying on Head, the attached patch is rebased on
top of Head.

Regards,
Vignesh

On Wed, Jun 30, 2021 at 7:48 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Thu, May 13, 2021 at 8:09 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Thu, May 13, 2021 at 4:58 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > >
> >
> > Thanks for the comments, Attached patch has the changes for the same.
> >
>
> The Patch was not applying on Head, the attached patch is rebased on
> top of Head.

The patch was not applying on the head because of the recent commit
"8aafb02616753f5c6c90bbc567636b73c0cbb9d4", attached patch which is
rebased on HEAD.

Regards,
Vignesh

> On 6 Jul 2021, at 17:08, vignesh C <vignesh21@gmail.com> wrote:

> The patch was not applying on the head because of the recent commit
> "8aafb02616753f5c6c90bbc567636b73c0cbb9d4", attached patch which is
> rebased on HEAD.

I sort of like the visual cue of seeing ereport(ERROR ..  since it makes it
clear it will break execution then and there, this will require a lookup for
anyone who don't know the function by heart.  That being said, reducing
duplicated boilerplate has clear value and this reduce the risk of introducing
strings which are complicated to translate.  On the whole I think this is a net
win, and the patch looks pretty good.

-               DefElem    *defel = (DefElem *) lfirst(option);
+               defel = (DefElem *) lfirst(option);
Any particular reason to include this in the patch?

--
Daniel Gustafsson        https://vmware.com/

On Thu, Jul 8, 2021 at 1:52 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 6 Jul 2021, at 17:08, vignesh C <vignesh21@gmail.com> wrote:
>
> > The patch was not applying on the head because of the recent commit
> > "8aafb02616753f5c6c90bbc567636b73c0cbb9d4", attached patch which is
> > rebased on HEAD.
>
> I sort of like the visual cue of seeing ereport(ERROR ..  since it makes it
> clear it will break execution then and there, this will require a lookup for
> anyone who don't know the function by heart.  That being said, reducing
> duplicated boilerplate has clear value and this reduce the risk of introducing
> strings which are complicated to translate.  On the whole I think this is a net
> win, and the patch looks pretty good.
>
> -               DefElem    *defel = (DefElem *) lfirst(option);
> +               defel = (DefElem *) lfirst(option);
> Any particular reason to include this in the patch?
>

Thanks for identifying this, this change is not needed, this was
required in my previous solution based on goto label. As we have made
these changes into a common function. This change is not required,
Attached v9 patch which removes these changes.

Regards,
Vignesh

On Thu, 8 Jul 2021 at 14:40, vignesh C <vignesh21@gmail.com> wrote:
>
> On Thu, Jul 8, 2021 at 1:52 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> >
> > I sort of like the visual cue of seeing ereport(ERROR ..  since it makes it
> > clear it will break execution then and there, this will require a lookup for
> > anyone who don't know the function by heart.  That being said, reducing
> > duplicated boilerplate has clear value and this reduce the risk of introducing
> > strings which are complicated to translate.  On the whole I think this is a net
> > win, and the patch looks pretty good.
> >

Bikeshedding the function name, there are several similar examples in
the existing code, but the closest analogs are probably
errorMissingColumn() and errorMissingRTE(). So I think
errorConflictingDefElem() would be better, since it's slightly more
obviously an error.

Also, I don't think this function should be marked inline -- using a
normal function ought to help make the compiled code smaller.

A bigger problem is that the patch replaces about 100 instances of the
error "conflicting or redundant options" with "option \"%s\" specified
more than once", but that's not always the appropriate thing to do.
For example, in the walsender code, the error isn't necessarily due to
the option being specified more than once.

Also, there are cases where def->defname isn't actually the name of
the option specified, so including it in the error is misleading. For
example:

CREATE OR REPLACE FUNCTION foo() RETURNS int
AS $$ SELECT 1 $$ STABLE IMMUTABLE;

ERROR:  option "volatility" specified more than once
LINE 2: AS $$ SELECT 1 $$ STABLE IMMUTABLE;
                                 ^

and in this case "volatility" is an internal string, so it won't get translated.

I'm inclined to think that it isn't worth the effort trying to
distinguish between conflicting options, options specified more than
once and faked-up options that weren't really specified. If we just
make errorConflictingDefElem() report "conflicting or redundant
options", then it's much easier to update calling code without making
mistakes. The benefit then comes from the reduced code size and the
fact that the patch includes pstate in more places, so the
parser_errposition() indicator helps the user identify the problem.

In file_fdw_validator(), where there is no pstate, it's already using
"specified more than once" as a hint to clarify the "conflicting or
redundant options" error, so I think we should leave that alone.

Regards,
Dean

On Sat, 10 Jul 2021 at 11:44, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> I'm inclined to think that it isn't worth the effort trying to
> distinguish between conflicting options, options specified more than
> once and faked-up options that weren't really specified. If we just
> make errorConflictingDefElem() report "conflicting or redundant
> options", then it's much easier to update calling code without making
> mistakes. The benefit then comes from the reduced code size and the
> fact that the patch includes pstate in more places, so the
> parser_errposition() indicator helps the user identify the problem.
>
> In file_fdw_validator(), where there is no pstate, it's already using
> "specified more than once" as a hint to clarify the "conflicting or
> redundant options" error, so I think we should leave that alone.
>

Another possibility would be to pass the option list to
errorConflictingDefElem() and it could work out whether or not to
include the "option \%s\" specified more than once" hint, since that
hint probably is useful, and working out whether to include it is
probably less error-prone if it's done there.

Regards,
Dean

On Sat, Jul 10, 2021 at 4:14 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Thu, 8 Jul 2021 at 14:40, vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Thu, Jul 8, 2021 at 1:52 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> > >
> > > I sort of like the visual cue of seeing ereport(ERROR ..  since it makes it
> > > clear it will break execution then and there, this will require a lookup for
> > > anyone who don't know the function by heart.  That being said, reducing
> > > duplicated boilerplate has clear value and this reduce the risk of introducing
> > > strings which are complicated to translate.  On the whole I think this is a net
> > > win, and the patch looks pretty good.
> > >
>
> Bikeshedding the function name, there are several similar examples in
> the existing code, but the closest analogs are probably
> errorMissingColumn() and errorMissingRTE(). So I think
> errorConflictingDefElem() would be better, since it's slightly more
> obviously an error.
>

Ok, I will change it to keep it similar.

> Also, I don't think this function should be marked inline -- using a
> normal function ought to help make the compiled code smaller.
>

inline instructs the compiler to attempt to embed the function content
into the calling code instead of executing an actual call. I think we
should keep it inline to reduce the function call.

> A bigger problem is that the patch replaces about 100 instances of the
> error "conflicting or redundant options" with "option \"%s\" specified
> more than once", but that's not always the appropriate thing to do.
> For example, in the walsender code, the error isn't necessarily due to
> the option being specified more than once.
>

This patch intended to change "conflicting or redundant options" to
"option \"%s\" specified more than once" only in case that error is
for option specified more than once. This change is not required. I
will remove it.

> Also, there are cases where def->defname isn't actually the name of
> the option specified, so including it in the error is misleading. For
> example:
>
> CREATE OR REPLACE FUNCTION foo() RETURNS int
> AS $$ SELECT 1 $$ STABLE IMMUTABLE;
>
> ERROR:  option "volatility" specified more than once
> LINE 2: AS $$ SELECT 1 $$ STABLE IMMUTABLE;
>                                  ^
>
> and in this case "volatility" is an internal string, so it won't get translated.
>
> I'm inclined to think that it isn't worth the effort trying to
> distinguish between conflicting options, options specified more than
> once and faked-up options that weren't really specified. If we just
> make errorConflictingDefElem() report "conflicting or redundant
> options", then it's much easier to update calling code without making
> mistakes. The benefit then comes from the reduced code size and the
> fact that the patch includes pstate in more places, so the
> parser_errposition() indicator helps the user identify the problem.
>
> In file_fdw_validator(), where there is no pstate, it's already using
> "specified more than once" as a hint to clarify the "conflicting or
> redundant options" error, so I think we should leave that alone.

This patch intended to change "conflicting or redundant options" to
"option \"%s\" specified more than once" only in case that error is
for option specified more than once. Thanks for pointing out a few
places where the actual error "conflicting or redundant options"
should be left as it is. I will post a new patch which will remove the
conflicting options error scenarios, which were not targeted in this
patch.

Regards,
Vignesh

On Sat, Jul 10, 2021 at 4:31 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Sat, 10 Jul 2021 at 11:44, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> >
> > I'm inclined to think that it isn't worth the effort trying to
> > distinguish between conflicting options, options specified more than
> > once and faked-up options that weren't really specified. If we just
> > make errorConflictingDefElem() report "conflicting or redundant
> > options", then it's much easier to update calling code without making
> > mistakes. The benefit then comes from the reduced code size and the
> > fact that the patch includes pstate in more places, so the
> > parser_errposition() indicator helps the user identify the problem.
> >
> > In file_fdw_validator(), where there is no pstate, it's already using
> > "specified more than once" as a hint to clarify the "conflicting or
> > redundant options" error, so I think we should leave that alone.
> >
>
> Another possibility would be to pass the option list to
> errorConflictingDefElem() and it could work out whether or not to
> include the "option \%s\" specified more than once" hint, since that
> hint probably is useful, and working out whether to include it is
> probably less error-prone if it's done there.

I'm planning to handle conflicting errors separately after this
current work is done, once the patch is changed to have just the valid
scenarios(removing the scenarios you have pointed out)  existing
function can work as is without any changes. Thoughts?

Regards,
Vignesh

On Sat, 10 Jul 2021 at 17:03, vignesh C <vignesh21@gmail.com> wrote:
>
> > Also, I don't think this function should be marked inline -- using a
> > normal function ought to help make the compiled code smaller.
>
> inline instructs the compiler to attempt to embed the function content
> into the calling code instead of executing an actual call. I think we
> should keep it inline to reduce the function call.

Hmm, I'd say that inline should generally be used sparingly, and only
for small functions that are called very often, to avoid the function
call overhead, and generate a faster and possibly smaller executable.
(Though I think sometimes it can still be faster if the executable is
larger.)

In this case, it's a function that is only called under error
conditions, so it's not commonly called, and we don't care so much
about performance when we're about to throw an error.

Also, if you look at an ereport() such as

    ereport(ERROR,
            errcode(ERRCODE_SYNTAX_ERROR),
            errmsg("conflicting or redundant options"),
            parser_errposition(pstate, defel->location)));

This is a macro that's actually expanded into 5 separate function calls:

 - errstart() / errstart_cold()
 - errcode()
 - errmsg()
 - parser_errposition()
 - errfinish()

so it's a non-trivial amount of code. Whereas, if it's not inlined, it
becomes just one function call at each call-site, making for smaller,
faster code in the typical case where an error is not being raised.

Of course, it's possible the compiler might still decide to inline the
function, if it thinks that's preferable. In some cases, we explicitly
mark this type of function with pg_noinline, to avoid that, and reduce
code bloat where it's used in lots of small, fast functions (see, for
example, float_overflow_error()).

In general though, I think inline and noinline should be reserved for
special cases where they give a clear, measurable benefit, and that in
general it's better to not mark the function and just let the compiler
decide.

Regards,
Dean

On Sat, 10 Jul 2021 at 18:09, vignesh C <vignesh21@gmail.com> wrote:
>
> I'm planning to handle conflicting errors separately after this
> current work is done, once the patch is changed to have just the valid
> scenarios(removing the scenarios you have pointed out)  existing
> function can work as is without any changes. Thoughts?

Ah OK, that might be reasonable. Perhaps, then errorDuplicateDefElem()
and errorConflictingDefElem() would be better than what I originally
suggested.

BTW, another case I spotted was this:

copy (select 1) to stdout csv csv header;
ERROR:  option "format" specified more than once
LINE 1: copy (select 1) to stdout csv csv header;
                                      ^

which isn't good because there is no option called "format".

Regards,
Dean

.On Sun, Jul 11, 2021 at 2:57 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Sat, 10 Jul 2021 at 17:03, vignesh C <vignesh21@gmail.com> wrote:
> >
> > > Also, I don't think this function should be marked inline -- using a
> > > normal function ought to help make the compiled code smaller.
> >
> > inline instructs the compiler to attempt to embed the function content
> > into the calling code instead of executing an actual call. I think we
> > should keep it inline to reduce the function call.
>
> Hmm, I'd say that inline should generally be used sparingly, and only
> for small functions that are called very often, to avoid the function
> call overhead, and generate a faster and possibly smaller executable.
> (Though I think sometimes it can still be faster if the executable is
> larger.)
>
> In this case, it's a function that is only called under error
> conditions, so it's not commonly called, and we don't care so much
> about performance when we're about to throw an error.
>
> Also, if you look at an ereport() such as
>
>     ereport(ERROR,
>             errcode(ERRCODE_SYNTAX_ERROR),
>             errmsg("conflicting or redundant options"),
>             parser_errposition(pstate, defel->location)));
>
> This is a macro that's actually expanded into 5 separate function calls:
>
>  - errstart() / errstart_cold()
>  - errcode()
>  - errmsg()
>  - parser_errposition()
>  - errfinish()
>
> so it's a non-trivial amount of code. Whereas, if it's not inlined, it
> becomes just one function call at each call-site, making for smaller,
> faster code in the typical case where an error is not being raised.
>
> Of course, it's possible the compiler might still decide to inline the
> function, if it thinks that's preferable. In some cases, we explicitly
> mark this type of function with pg_noinline, to avoid that, and reduce
> code bloat where it's used in lots of small, fast functions (see, for
> example, float_overflow_error()).
>
> In general though, I think inline and noinline should be reserved for
> special cases where they give a clear, measurable benefit, and that in
> general it's better to not mark the function and just let the compiler
> decide.

Ok, that makes sense. As this flow is mainly in the error part it is
ok. I will change it.

Regards,
Vignesh

On Sun, Jul 11, 2021 at 3:23 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Sat, 10 Jul 2021 at 18:09, vignesh C <vignesh21@gmail.com> wrote:
> >
> > I'm planning to handle conflicting errors separately after this
> > current work is done, once the patch is changed to have just the valid
> > scenarios(removing the scenarios you have pointed out)  existing
> > function can work as is without any changes. Thoughts?
>
> Ah OK, that might be reasonable. Perhaps, then errorDuplicateDefElem()
> and errorConflictingDefElem() would be better than what I originally
> suggested.
>
> BTW, another case I spotted was this:
>
> copy (select 1) to stdout csv csv header;
> ERROR:  option "format" specified more than once
> LINE 1: copy (select 1) to stdout csv csv header;
>                                       ^
>

Thanks for your comments, I have made the changes for the same in the
V10 patch attached.
Thoughts?

Regards,
Vignesh

On Mon, 12 Jul 2021 at 17:39, vignesh C <vignesh21@gmail.com> wrote:
>
> Thanks for your comments, I have made the changes for the same in the
> V10 patch attached.
> Thoughts?
>

I'm still not happy about changing so many error messages.

Some of the changes might be OK, but aren't strictly necessary. For example:

 COPY x from stdin (force_not_null (a), force_not_null (b));
-ERROR:  conflicting or redundant options
+ERROR:  option "force_not_null" specified more than once
 LINE 1: COPY x from stdin (force_not_null (a), force_not_null (b));
                                                ^

I actually prefer the original primary error message, for consistency
with other similar cases, and I think the error position indicator is
sufficient to identify the problem. If it were to include the
"specified more than once" text, I would put that in DETAIL.

Other changes are wrong though. For example:

 COPY x from stdin (format CSV, FORMAT CSV);
-ERROR:  conflicting or redundant options
+ERROR:  redundant options specified
 LINE 1: COPY x from stdin (format CSV, FORMAT CSV);
                                        ^

The problem here is that the code that throws this error throws the
same error if the second format is different, which would make it a
conflicting option, not a redundant one. And I don't think we should
add more code to test whether it's conflicting or redundant, so again,
I think we should just keep the original error message.

Similarly, this error is wrong:

CREATE OR REPLACE FUNCTION foo() RETURNS int AS $$ SELECT 1 $$ STABLE IMMUTABLE;
ERROR:  redundant options specified
LINE 1: ...NCTION foo() RETURNS int AS $$ SELECT 1 $$ STABLE IMMUTABLE;
                                                             ^

And even this error:

CREATE OR REPLACE FUNCTION foo() RETURNS int AS $$ SELECT 1 $$ STRICT STRICT;
ERROR:  redundant options specified
LINE 1: ... FUNCTION foo() RETURNS int AS $$ SELECT 1 $$ STRICT STRICT;
                                                                ^

which looks OK, is actually problematic because the same code also
handles the alternate syntax, which leads to this (which is now wrong
because it's conflicting not redundant):

CREATE OR REPLACE FUNCTION foo() RETURNS int AS $$ SELECT 1 $$ STRICT
CALLED ON NULL INPUT;
ERROR:  redundant options specified
LINE 1: ...NCTION foo() RETURNS int AS $$ SELECT 1 $$ STRICT CALLED ON ...
                                                             ^

The problem is it's actually quite hard to decide in each case whether
the option is redundant or conflicting. Sometimes, it might look
obvious in the code, but actually be much more subtle, due to an
earlier transformation of the grammar. Likewise redundant doesn't
necessarily mean literally specified more than once.

Also, most of these don't have regression test cases, and I'm very
reluctant to change them without proper testing, and that would make
the patch much bigger. To me, this patch is already attempting to
change too much in one go, which is causing problems.

So I suggest a more incremental approach, starting by keeping the
original error message, but improving it where possible with the error
position. Then maybe move on to look at specific cases that can be
further improved with additional detail (keeping the same primary
error message, for consistency).

Here is an updated version, following that approach. It does the following:

1). Keeps the same primary error message ("conflicting or redundant
options") in all cases.

2). Uses errorConflictingDefElem() to throw it, to ensure consistency
and reduce the executable size.

3). Includes your enhancements to make the ParseState available in
more places, so that the error position indicator is shown to indicate
the cause of the error.

IMO, this makes for a much safer incremental change, that is more committable.

As it turns out, there are 110 cases of this error that now use
errorConflictingDefElem(), and of those, just 10 (in 3 functions)
don't have a ParseState readily available to them:

- ATExecSetIdentity()
- parse_output_parameters() x5
- parseCreateReplSlotOptions() x4

It might be possible to improve those (and possibly some of the others
too) by adding some appropriate DETAIL to the error, but as I said, I
suggest doing that in a separate follow-on patch, and only with
careful analysis and testing of each case.

As it stands, the improvements from (3) seem quite worthwhile. Also,
the patch saves a couple of hundred lines of code, and for me an
optimised executable is around 30 kB smaller, which is more than I
expected.

Thoughts?

Regards,
Dean

On Tue, Jul 13, 2021 at 4:25 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Mon, 12 Jul 2021 at 17:39, vignesh C <vignesh21@gmail.com> wrote:
> >
> > Thanks for your comments, I have made the changes for the same in the
> > V10 patch attached.
> > Thoughts?
> >
>
> I'm still not happy about changing so many error messages.
>
> Some of the changes might be OK, but aren't strictly necessary. For example:
>
>  COPY x from stdin (force_not_null (a), force_not_null (b));
> -ERROR:  conflicting or redundant options
> +ERROR:  option "force_not_null" specified more than once
>  LINE 1: COPY x from stdin (force_not_null (a), force_not_null (b));
>                                                 ^
>
> I actually prefer the original primary error message, for consistency
> with other similar cases, and I think the error position indicator is
> sufficient to identify the problem. If it were to include the
> "specified more than once" text, I would put that in DETAIL.
>
> Other changes are wrong though. For example:
>
>  COPY x from stdin (format CSV, FORMAT CSV);
> -ERROR:  conflicting or redundant options
> +ERROR:  redundant options specified
>  LINE 1: COPY x from stdin (format CSV, FORMAT CSV);
>                                         ^
>
> The problem here is that the code that throws this error throws the
> same error if the second format is different, which would make it a
> conflicting option, not a redundant one. And I don't think we should
> add more code to test whether it's conflicting or redundant, so again,
> I think we should just keep the original error message.
>
> Similarly, this error is wrong:
>
> CREATE OR REPLACE FUNCTION foo() RETURNS int AS $$ SELECT 1 $$ STABLE IMMUTABLE;
> ERROR:  redundant options specified
> LINE 1: ...NCTION foo() RETURNS int AS $$ SELECT 1 $$ STABLE IMMUTABLE;
>                                                              ^
>
> And even this error:
>
> CREATE OR REPLACE FUNCTION foo() RETURNS int AS $$ SELECT 1 $$ STRICT STRICT;
> ERROR:  redundant options specified
> LINE 1: ... FUNCTION foo() RETURNS int AS $$ SELECT 1 $$ STRICT STRICT;
>                                                                 ^
>
> which looks OK, is actually problematic because the same code also
> handles the alternate syntax, which leads to this (which is now wrong
> because it's conflicting not redundant):
>
> CREATE OR REPLACE FUNCTION foo() RETURNS int AS $$ SELECT 1 $$ STRICT
> CALLED ON NULL INPUT;
> ERROR:  redundant options specified
> LINE 1: ...NCTION foo() RETURNS int AS $$ SELECT 1 $$ STRICT CALLED ON ...
>                                                              ^
>
> The problem is it's actually quite hard to decide in each case whether
> the option is redundant or conflicting. Sometimes, it might look
> obvious in the code, but actually be much more subtle, due to an
> earlier transformation of the grammar. Likewise redundant doesn't
> necessarily mean literally specified more than once.
>
> Also, most of these don't have regression test cases, and I'm very
> reluctant to change them without proper testing, and that would make
> the patch much bigger. To me, this patch is already attempting to
> change too much in one go, which is causing problems.
>
> So I suggest a more incremental approach, starting by keeping the
> original error message, but improving it where possible with the error
> position. Then maybe move on to look at specific cases that can be
> further improved with additional detail (keeping the same primary
> error message, for consistency).

I'm fine with this approach as we do not have tests to cover all the
error conditions, also I'm not sure if it is worth adding tests for
all the error conditions and as the patch changes a large number of
error conditions, an incremental approach is better.

> Here is an updated version, following that approach. It does the following:
>
> 1). Keeps the same primary error message ("conflicting or redundant
> options") in all cases.
>
> 2). Uses errorConflictingDefElem() to throw it, to ensure consistency
> and reduce the executable size.
>
> 3). Includes your enhancements to make the ParseState available in
> more places, so that the error position indicator is shown to indicate
> the cause of the error.
>
> IMO, this makes for a much safer incremental change, that is more committable.
>
> As it turns out, there are 110 cases of this error that now use
> errorConflictingDefElem(), and of those, just 10 (in 3 functions)
> don't have a ParseState readily available to them:
>
> - ATExecSetIdentity()
> - parse_output_parameters() x5
> - parseCreateReplSlotOptions() x4
>
> It might be possible to improve those (and possibly some of the others
> too) by adding some appropriate DETAIL to the error, but as I said, I
> suggest doing that in a separate follow-on patch, and only with
> careful analysis and testing of each case.
>
> As it stands, the improvements from (3) seem quite worthwhile. Also,
> the patch saves a couple of hundred lines of code, and for me an
> optimised executable is around 30 kB smaller, which is more than I
> expected.

Agreed, it can be handled as part of the 2nd patch. The changes you
made apply neatly and the test passes.

Regards,
Vignesh

On Tue, 13 Jul 2021 at 15:30, vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, Jul 13, 2021 at 4:25 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> >
> > As it stands, the improvements from (3) seem quite worthwhile. Also,
> > the patch saves a couple of hundred lines of code, and for me an
> > optimised executable is around 30 kB smaller, which is more than I
> > expected.
>
> Agreed, it can be handled as part of the 2nd patch. The changes you
> made apply neatly and the test passes.

Pushed.

I noticed that it's actually safe to call parser_errposition() with a
null ParseState, so I simplified the ereport() code to just call it
unconditionally. Also, I decided to not bother using the new function
in cases with a null ParseState anyway since it doesn't provide any
meaningful benefit in those cases, and those are the cases most likely
to targeted next, so it didn't seem sensible to change that code, only
for it to be changed again later.

Probably the thing to think about next is the few remaining cases that
throw this error directly and don't have any errdetail or errhint to
help the user identify the offending option. My preference remains to
leave the primary error text unchanged, but just add some suitable
errdetail. Also, it's probably not worth adding a new function for
those remaining errors, since there are only a handful of them.

Regards,
Dean

On Thu, Jul 15, 2021 at 1:40 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Tue, 13 Jul 2021 at 15:30, vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Tue, Jul 13, 2021 at 4:25 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> > >
> > > As it stands, the improvements from (3) seem quite worthwhile. Also,
> > > the patch saves a couple of hundred lines of code, and for me an
> > > optimised executable is around 30 kB smaller, which is more than I
> > > expected.
> >
> > Agreed, it can be handled as part of the 2nd patch. The changes you
> > made apply neatly and the test passes.
>
> Pushed.
>
> I noticed that it's actually safe to call parser_errposition() with a
> null ParseState, so I simplified the ereport() code to just call it
> unconditionally. Also, I decided to not bother using the new function
> in cases with a null ParseState anyway since it doesn't provide any
> meaningful benefit in those cases, and those are the cases most likely
> to targeted next, so it didn't seem sensible to change that code, only
> for it to be changed again later.
>
> Probably the thing to think about next is the few remaining cases that
> throw this error directly and don't have any errdetail or errhint to
> help the user identify the offending option. My preference remains to
> leave the primary error text unchanged, but just add some suitable
> errdetail. Also, it's probably not worth adding a new function for
> those remaining errors, since there are only a handful of them.

Thanks for pushing this patch. I have changed the commitfest status to
"waiting for author" till 0002 patch is posted.

Regards,
Vignesh

On Thu, Jul 15, 2021 at 5:12 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Thu, Jul 15, 2021 at 1:40 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> >
> > On Tue, 13 Jul 2021 at 15:30, vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Tue, Jul 13, 2021 at 4:25 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> > > >
> > > > As it stands, the improvements from (3) seem quite worthwhile. Also,
> > > > the patch saves a couple of hundred lines of code, and for me an
> > > > optimised executable is around 30 kB smaller, which is more than I
> > > > expected.
> > >
> > > Agreed, it can be handled as part of the 2nd patch. The changes you
> > > made apply neatly and the test passes.
> >
> > Pushed.
> >
> > I noticed that it's actually safe to call parser_errposition() with a
> > null ParseState, so I simplified the ereport() code to just call it
> > unconditionally. Also, I decided to not bother using the new function
> > in cases with a null ParseState anyway since it doesn't provide any
> > meaningful benefit in those cases, and those are the cases most likely
> > to targeted next, so it didn't seem sensible to change that code, only
> > for it to be changed again later.
> >
> > Probably the thing to think about next is the few remaining cases that
> > throw this error directly and don't have any errdetail or errhint to
> > help the user identify the offending option. My preference remains to
> > leave the primary error text unchanged, but just add some suitable
> > errdetail. Also, it's probably not worth adding a new function for
> > those remaining errors, since there are only a handful of them.
>
> Thanks for pushing this patch. I have changed the commitfest status to
> "waiting for author" till 0002 patch is posted.

I'm marking this entry in commitfest as committed, I'm planning to
work on the other comments later once I finish my current project
works.

Regards,
Vignesh