Thread
Commits
-
Improve reporting of "conflicting or redundant options" errors.
- 2bfb50b3df11 15.0 landed
-
Refactor function parse_subscription_options.
- 8aafb0261675 15.0 cited
-
Enhanced error message to include hint messages for redundant options error
vignesh C <vignesh21@gmail.com> — 2021-04-26T11:58:55Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-04-26T12:19:21Z
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 -
Re: Enhanced error message to include hint messages for redundant options error
Dilip Kumar <dilipbalaut@gmail.com> — 2021-04-26T12:48:29Z
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 -
Re: Enhanced error message to include hint messages for redundant options error
vignesh C <vignesh21@gmail.com> — 2021-04-26T13:31:55Z
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 -
Re: Enhanced error message to include hint messages for redundant options error
vignesh C <vignesh21@gmail.com> — 2021-04-26T13:33:06Z
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 -
Re: Enhanced error message to include hint messages for redundant options error
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-04-26T13:45:10Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-26T14:36:29Z
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" -
Re: Enhanced error message to include hint messages for redundant options error
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-04-26T15:40:25Z
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 -
Re: Enhanced error message to include hint messages for redundant options error
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-26T15:54:32Z
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.
-
Re: Enhanced error message to include hint messages for redundant options error
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-04-27T00:50:57Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-04-27T00:53:56Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-04-27T11:43:33Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
vignesh C <vignesh21@gmail.com> — 2021-04-29T16:47:58Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-29T17:14:17Z
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"
-
Re: Enhanced error message to include hint messages for redundant options error
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-04-30T02:46:15Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
Dilip Kumar <dilipbalaut@gmail.com> — 2021-04-30T04:47:00Z
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 -
Re: Enhanced error message to include hint messages for redundant options error
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-04-30T05:13:21Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
Dilip Kumar <dilipbalaut@gmail.com> — 2021-04-30T05:20:44Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-04-30T05:39:31Z
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 -
Re: Enhanced error message to include hint messages for redundant options error
Dilip Kumar <dilipbalaut@gmail.com> — 2021-04-30T05:53:32Z
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 -
Re: Enhanced error message to include hint messages for redundant options error
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-04-30T07:06:07Z
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 -
Re: Enhanced error message to include hint messages for redundant options error
Peter Smith <smithpb2250@gmail.com> — 2021-04-30T09:19:03Z
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 -
Re: Enhanced error message to include hint messages for redundant options error
Dilip Kumar <dilipbalaut@gmail.com> — 2021-04-30T09:19:34Z
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 -
Re: Enhanced error message to include hint messages for redundant options error
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-01T05:12:58Z
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 -
Re: Enhanced error message to include hint messages for redundant options error
Dilip Kumar <dilipbalaut@gmail.com> — 2021-05-01T05:17:25Z
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 -
Re: Enhanced error message to include hint messages for redundant options error
vignesh C <vignesh21@gmail.com> — 2021-05-01T13:55:15Z
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 -
Re: Enhanced error message to include hint messages for redundant options error
vignesh C <vignesh21@gmail.com> — 2021-05-01T13:56:44Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-01T15:31:48Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-05-01T17:24:09Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-01T17:55:44Z
On Sat, May 1, 2021, 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. > 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. Best Regards, Bharath Rupireddy. EnterpriseDB. >
-
Re: Enhanced error message to include hint messages for redundant options error
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-05-01T22:57:14Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
vignesh C <vignesh21@gmail.com> — 2021-05-02T10:30:59Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
Julien Rouhaud <rjuju123@gmail.com> — 2021-05-02T11:42:54Z
Le dim. 2 mai 2021 à 18:31, vignesh C <vignesh21@gmail.com> a écrit : > 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. > +1 >
-
Re: Enhanced error message to include hint messages for redundant options error
vignesh C <vignesh21@gmail.com> — 2021-05-02T15:12:30Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
vignesh C <vignesh21@gmail.com> — 2021-05-02T15:14:25Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-03T06:38:06Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
Dilip Kumar <dilipbalaut@gmail.com> — 2021-05-03T08:11:21Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
vignesh C <vignesh21@gmail.com> — 2021-05-03T13:11:15Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-04T07:19:23Z
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
-
RE: Enhanced error message to include hint messages for redundant options error
Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2021-05-08T06:31:22Z
> > > 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
-
Re: Enhanced error message to include hint messages for redundant options error
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-08T08:50:34Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
vignesh C <vignesh21@gmail.com> — 2021-05-08T13:36:42Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-09T12:39:38Z
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
-
RE: Enhanced error message to include hint messages for redundant options error
Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2021-05-10T00:30:09Z
> > > > > > > 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
-
Re: Enhanced error message to include hint messages for redundant options error
vignesh C <vignesh21@gmail.com> — 2021-05-10T13:27:47Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-05-10T21:17:14Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-11T04:08:22Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
vignesh C <vignesh21@gmail.com> — 2021-05-11T13:24:42Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-11T16:06:02Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-05-12T23:28:23Z
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) -
Re: Enhanced error message to include hint messages for redundant options error
vignesh C <vignesh21@gmail.com> — 2021-05-13T14:39:13Z
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 -
Re: Enhanced error message to include hint messages for redundant options error
vignesh C <vignesh21@gmail.com> — 2021-06-30T14:18:51Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
vignesh C <vignesh21@gmail.com> — 2021-07-06T15:08:35Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
Daniel Gustafsson <daniel@yesql.se> — 2021-07-07T20:22:09Z
> 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/
-
Re: Enhanced error message to include hint messages for redundant options error
vignesh C <vignesh21@gmail.com> — 2021-07-08T13:40:36Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
Dean Rasheed <dean.a.rasheed@gmail.com> — 2021-07-10T10:44:18Z
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 -
Re: Enhanced error message to include hint messages for redundant options error
Dean Rasheed <dean.a.rasheed@gmail.com> — 2021-07-10T11:00:49Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
vignesh C <vignesh21@gmail.com> — 2021-07-10T16:02:50Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
vignesh C <vignesh21@gmail.com> — 2021-07-10T17:08:53Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
Dean Rasheed <dean.a.rasheed@gmail.com> — 2021-07-11T09:27:00Z
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 -
Re: Enhanced error message to include hint messages for redundant options error
Dean Rasheed <dean.a.rasheed@gmail.com> — 2021-07-11T09:52:56Z
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 -
Re: Enhanced error message to include hint messages for redundant options error
vignesh C <vignesh21@gmail.com> — 2021-07-11T13:29:23Z
.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 -
Re: Enhanced error message to include hint messages for redundant options error
vignesh C <vignesh21@gmail.com> — 2021-07-12T16:39:22Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
Dean Rasheed <dean.a.rasheed@gmail.com> — 2021-07-13T10:54:54Z
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 -
Re: Enhanced error message to include hint messages for redundant options error
vignesh C <vignesh21@gmail.com> — 2021-07-13T14:30:19Z
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 -
Re: Enhanced error message to include hint messages for redundant options error
Dean Rasheed <dean.a.rasheed@gmail.com> — 2021-07-15T08:10:19Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
vignesh C <vignesh21@gmail.com> — 2021-07-15T11:42:39Z
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
-
Re: Enhanced error message to include hint messages for redundant options error
vignesh C <vignesh21@gmail.com> — 2021-07-22T06:09:23Z
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