Re: Enhanced error message to include hint messages for redundant options error
On Thu, Jul 15, 2021 at 5:12 PM vignesh C wrote: > > On Thu, Jul 15, 2021 at 1:40 PM Dean Rasheed wrote: > > > > On Tue, 13 Jul 2021 at 15:30, vignesh C wrote: > > > > > > On Tue, Jul 13, 2021 at 4:25 PM Dean Rasheed > > > 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
Re: Enhanced error message to include hint messages for redundant options error
On Thu, Jul 15, 2021 at 1:40 PM Dean Rasheed wrote: > > On Tue, 13 Jul 2021 at 15:30, vignesh C wrote: > > > > On Tue, Jul 13, 2021 at 4:25 PM Dean Rasheed > > 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
On Tue, 13 Jul 2021 at 15:30, vignesh C wrote: > > On Tue, Jul 13, 2021 at 4:25 PM Dean Rasheed 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
On Tue, Jul 13, 2021 at 4:25 PM Dean Rasheed wrote: > > On Mon, 12 Jul 2021 at 17:39, vignesh C 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
Re: Enhanced error message to include hint messages for redundant options error
On Mon, 12 Jul 2021 at 17:39, vignesh C 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 diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c new file mode 100644 index 5339241..89792b1 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -59,6 +59,7 @@ #include "catalog/pg_ts_template.h" #include "catalog/pg_type.h" #include "commands/dbcommands.h" +#include "commands/defrem.h" #include "commands/event_trigger.h" #include "commands/exte
Re: Enhanced error message to include hint messages for redundant options error
On Sun, Jul 11, 2021 at 3:23 PM Dean Rasheed wrote: > > On Sat, 10 Jul 2021 at 18:09, vignesh C 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 From c882bd3f8ebc7a8a21b8481234286aa8e2c9aee1 Mon Sep 17 00:00:00 2001 From: vignesh Date: Mon, 12 Jul 2021 09:12:34 +0530 Subject: [PATCH v10] Enhance error message to include option name in case of duplicate option error. Enhanced error message to include option name in case of duplication option error, so that the user can easily identify the error. --- contrib/file_fdw/file_fdw.c | 10 +- src/backend/catalog/aclchk.c| 11 +- src/backend/commands/copy.c | 54 ++ src/backend/commands/dbcommands.c | 70 +++- src/backend/commands/define.c | 20 src/backend/commands/extension.c| 20 +--- src/backend/commands/foreigncmds.c | 18 ++-- src/backend/commands/functioncmds.c | 53 +++-- src/backend/commands/publicationcmds.c | 27 +++-- src/backend/commands/sequence.c | 45 ++-- src/backend/commands/subscriptioncmds.c | 62 +-- src/backend/commands/tablecmds.c| 4 +- src/backend/commands/typecmds.c | 31 ++ src/backend/commands/user.c | 112 +--- src/backend/parser/parse_utilcmd.c | 4 +- src/backend/replication/pgoutput/pgoutput.c | 20 +--- src/backend/tcop/utility.c | 20 ++-- src/include/commands/defrem.h | 8 +- src/include/commands/publicationcmds.h | 4 +- src/include/commands/subscriptioncmds.h | 4 +- src/include/commands/typecmds.h | 2 +- src/include/commands/user.h | 2 +- src/test/regress/expected/copy2.out | 24 +++-- src/test/regress/expected/foreign_data.out | 8 +- src/test/regress/expected/publication.out | 4 +- 25 files changed, 209 insertions(+), 428 deletions(-) diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c index 2c2f149fb0..eb7edb695b 100644 --- a/contrib/file_fdw/file_fdw.c +++ b/contrib/file_fdw/file_fdw.c @@ -290,10 +290,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) else if (strcmp(def->defname, "force_not_null") == 0) { if (force_not_null) -ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), - errhint("Option \"force_not_null\" supplied more than once for a column."))); +errorDuplicateDefElem(def, NULL); force_not_null = def; /* Don't care what the value is, as long as it's a legal boolean */ (void) defGetBoolean(def); @@ -302,10 +299,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) else if (strcmp(def->defname, "force_null") == 0) { if (force_null) -ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), - errhint("Option \"force_null\" supplied more than once for a column."))); +errorDuplicateDefElem(def, NULL); force_null = def; (void) defGetBoolean(def); } diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index 53392414f1..c5b9f3cf70 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -59,6 +59,7 @@ #include "catalog/pg_ts_template.h" #include "catalog/pg_type.h" #include "commands/dbcommands.h" +#include "commands/defrem.h" #include "commands/event_trigger.h" #include "commands/extension.h" #include "commands/proclang.h" @@ -921,19 +922,13 @@ ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s if (strcmp(defel->defname, "schemas") == 0) { if (dnspnames) -ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), - parser_errposition(pstate, defel->location))); +errorDuplicateDefElem(defel, pstate); dnspnames = defel; } else if (strcmp(defel->defname, "roles") == 0) { if (drolespecs) -ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), - parser_errposition(pstate, defel->location))); +errorDuplicateDefElem(defel, pstate); drolespecs = defel;
Re: Enhanced error message to include hint messages for redundant options error
.On Sun, Jul 11, 2021 at 2:57 PM Dean Rasheed wrote: > > On Sat, 10 Jul 2021 at 17:03, vignesh C 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
On Sat, 10 Jul 2021 at 18:09, vignesh C 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
On Sat, 10 Jul 2021 at 17:03, vignesh C 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
On Sat, Jul 10, 2021 at 4:31 PM Dean Rasheed wrote: > > On Sat, 10 Jul 2021 at 11:44, Dean Rasheed 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
On Sat, Jul 10, 2021 at 4:14 PM Dean Rasheed wrote: > > On Thu, 8 Jul 2021 at 14:40, vignesh C wrote: > > > > On Thu, Jul 8, 2021 at 1:52 AM Daniel Gustafsson 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
On Sat, 10 Jul 2021 at 11:44, Dean Rasheed 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
On Thu, 8 Jul 2021 at 14:40, vignesh C wrote: > > On Thu, Jul 8, 2021 at 1:52 AM Daniel Gustafsson 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
On Thu, Jul 8, 2021 at 1:52 AM Daniel Gustafsson wrote: > > > On 6 Jul 2021, at 17:08, vignesh C 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 From a9935ebd58596772f2fdb7b641e90cfee071d98b Mon Sep 17 00:00:00 2001 From: vignesh Date: Tue, 6 Jul 2021 20:22:45 +0530 Subject: [PATCH v9] Enhance error message to include option name in case of duplicate option error. Enhanced error message to include option name in case of duplication option error, so that the user can easily identify the error. --- contrib/file_fdw/file_fdw.c | 10 +- src/backend/catalog/aclchk.c| 11 +- src/backend/commands/copy.c | 54 ++ src/backend/commands/dbcommands.c | 70 +++- src/backend/commands/extension.c| 20 +--- src/backend/commands/foreigncmds.c | 18 ++-- src/backend/commands/functioncmds.c | 53 +++-- src/backend/commands/publicationcmds.c | 27 +++-- src/backend/commands/sequence.c | 45 ++-- src/backend/commands/subscriptioncmds.c | 62 +-- src/backend/commands/tablecmds.c| 4 +- src/backend/commands/typecmds.c | 31 ++ src/backend/commands/user.c | 112 +--- src/backend/parser/parse_utilcmd.c | 4 +- src/backend/replication/pgoutput/pgoutput.c | 20 +--- src/backend/replication/walsender.c | 12 +-- src/backend/tcop/utility.c | 20 ++-- src/include/commands/defrem.h | 16 ++- src/include/commands/publicationcmds.h | 4 +- src/include/commands/subscriptioncmds.h | 4 +- src/include/commands/typecmds.h | 2 +- src/include/commands/user.h | 2 +- src/test/regress/expected/copy2.out | 24 +++-- src/test/regress/expected/foreign_data.out | 8 +- src/test/regress/expected/publication.out | 4 +- 25 files changed, 200 insertions(+), 437 deletions(-) diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c index 2c2f149fb0..0014a4d49d 100644 --- a/contrib/file_fdw/file_fdw.c +++ b/contrib/file_fdw/file_fdw.c @@ -290,10 +290,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) else if (strcmp(def->defname, "force_not_null") == 0) { if (force_not_null) -ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), - errhint("Option \"force_not_null\" supplied more than once for a column."))); +ReportDuplicateOptionError(def, NULL); force_not_null = def; /* Don't care what the value is, as long as it's a legal boolean */ (void) defGetBoolean(def); @@ -302,10 +299,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) else if (strcmp(def->defname, "force_null") == 0) { if (force_null) -ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), - errhint("Option \"force_null\" supplied more than once for a column."))); +ReportDuplicateOptionError(def, NULL); force_null = def; (void) defGetBoolean(def); } diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index 53392414f1..2968085916 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -59,6 +59,7 @@ #include "catalog/pg_ts_template.h" #include "catalog/pg_type.h" #include "commands/dbcommands.h" +#include "commands/defrem.h" #include "commands/event_trigger.h" #include "commands/extension.h" #include "commands/proclang.h" @@ -921,19 +922,13 @@ ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s if (strcmp(defel->defname, "schemas") == 0) { if (dnspnames) -ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), - parser_errposition(pstate, defel->location))); +ReportDuplicateOptionError(defel, pstate); dnspnames = defel; } else if (strcmp(defel->defna
Re: Enhanced error message to include hint messages for redundant options error
> On 6 Jul 2021, at 17:08, vignesh C 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
On Wed, Jun 30, 2021 at 7:48 PM vignesh C wrote: > > On Thu, May 13, 2021 at 8:09 PM vignesh C wrote: > > > > On Thu, May 13, 2021 at 4:58 AM Alvaro Herrera > > 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 From 5ccc262d895d688fbca42d795ea01c7c16c09b54 Mon Sep 17 00:00:00 2001 From: vignesh Date: Tue, 6 Jul 2021 20:22:45 +0530 Subject: [PATCH v8] Enhance error message to include option name in case of duplicate option error. Enhanced error message to include option name in case of duplication option error, so that the user can easily identify the error. --- contrib/file_fdw/file_fdw.c | 13 +-- src/backend/catalog/aclchk.c| 14 +-- src/backend/commands/copy.c | 57 +++--- src/backend/commands/dbcommands.c | 76 +++-- src/backend/commands/extension.c| 23 +--- src/backend/commands/foreigncmds.c | 21 ++-- src/backend/commands/functioncmds.c | 59 -- src/backend/commands/publicationcmds.c | 30 ++--- src/backend/commands/sequence.c | 48 ++-- src/backend/commands/subscriptioncmds.c | 62 +- src/backend/commands/tablecmds.c| 4 +- src/backend/commands/typecmds.c | 34 ++ src/backend/commands/user.c | 118 +--- src/backend/parser/parse_utilcmd.c | 4 +- src/backend/replication/pgoutput/pgoutput.c | 23 ++-- src/backend/replication/walsender.c | 15 +-- src/backend/tcop/utility.c | 20 ++-- src/include/commands/defrem.h | 16 ++- src/include/commands/publicationcmds.h | 4 +- src/include/commands/subscriptioncmds.h | 4 +- src/include/commands/typecmds.h | 2 +- src/include/commands/user.h | 2 +- src/test/regress/expected/copy2.out | 24 ++-- src/test/regress/expected/foreign_data.out | 8 +- src/test/regress/expected/publication.out | 4 +- 25 files changed, 232 insertions(+), 453 deletions(-) diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c index 2c2f149fb0..f49dd47930 100644 --- a/contrib/file_fdw/file_fdw.c +++ b/contrib/file_fdw/file_fdw.c @@ -200,6 +200,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) char *filename = NULL; DefElem*force_not_null = NULL; DefElem*force_null = NULL; + DefElem*def; List *other_options = NIL; ListCell *cell; @@ -209,7 +210,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) */ foreach(cell, options_list) { - DefElem*def = (DefElem *) lfirst(cell); + def = (DefElem *) lfirst(cell); if (!is_valid_option(def->defname, catalog)) { @@ -290,10 +291,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) else if (strcmp(def->defname, "force_not_null") == 0) { if (force_not_null) -ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), - errhint("Option \"force_not_null\" supplied more than once for a column."))); +ReportDuplicateOptionError(def, NULL); force_not_null = def; /* Don't care what the value is, as long as it's a legal boolean */ (void) defGetBoolean(def); @@ -302,10 +300,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) else if (strcmp(def->defname, "force_null") == 0) { if (force_null) -ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), - errhint("Option \"force_null\" supplied more than once for a column."))); +ReportDuplicateOptionError(def, NULL); force_null = def; (void) defGetBoolean(def); } diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index 53392414f1..264cdc2730 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -59,6 +59,7 @@ #include "catalog/pg_ts_template.h" #include "catalog/pg_type.h" #include "commands/dbcommands.h" +#include "commands/defrem.h" #include "commands/event_trigger.h" #include "commands/extension.h" #include "commands/proclang.h" @@ -910,30 +911,25 @@ ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s List *nspnames = NIL; DefElem*drolespecs = NULL; DefElem*dnspnames = NULL; + DefElem*defel; AclMode all_privileges; const char *errormsg; /* Deconstruct the "options" part of the statement */ foreach(cell, stmt->options) { - DefElem*defel = (DefElem *) lfirst(cell); + defel = (DefElem *) lfirst(cell); if (strcmp(defel->defname, "schemas") == 0) { if (dnspnames) -ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant op
Re: Enhanced error message to include hint messages for redundant options error
On Thu, May 13, 2021 at 8:09 PM vignesh C wrote: > > On Thu, May 13, 2021 at 4:58 AM Alvaro Herrera > 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 From fde4cfbe59064c890ac1c6c3342cc10398092c3c Mon Sep 17 00:00:00 2001 From: vignesh Date: Wed, 30 Jun 2021 19:18:32 +0530 Subject: [PATCH v8] Enhance error message. Enhanced error message, so that the user can easily identify the error. --- contrib/file_fdw/file_fdw.c | 13 +-- src/backend/catalog/aclchk.c| 14 +-- src/backend/commands/copy.c | 57 +++--- src/backend/commands/dbcommands.c | 76 +++-- src/backend/commands/extension.c| 23 +--- src/backend/commands/foreigncmds.c | 21 ++-- src/backend/commands/functioncmds.c | 59 -- src/backend/commands/publicationcmds.c | 30 ++--- src/backend/commands/sequence.c | 48 ++-- src/backend/commands/subscriptioncmds.c | 66 +-- src/backend/commands/tablecmds.c| 4 +- src/backend/commands/typecmds.c | 34 ++ src/backend/commands/user.c | 118 +--- src/backend/parser/parse_utilcmd.c | 4 +- src/backend/replication/pgoutput/pgoutput.c | 23 ++-- src/backend/replication/walsender.c | 15 +-- src/backend/tcop/utility.c | 20 ++-- src/include/commands/defrem.h | 16 ++- src/include/commands/publicationcmds.h | 4 +- src/include/commands/subscriptioncmds.h | 4 +- src/include/commands/typecmds.h | 2 +- src/include/commands/user.h | 2 +- src/test/regress/expected/copy2.out | 24 ++-- src/test/regress/expected/foreign_data.out | 8 +- src/test/regress/expected/publication.out | 4 +- 25 files changed, 235 insertions(+), 454 deletions(-) diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c index 2c2f149fb0..f49dd47930 100644 --- a/contrib/file_fdw/file_fdw.c +++ b/contrib/file_fdw/file_fdw.c @@ -200,6 +200,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) char *filename = NULL; DefElem*force_not_null = NULL; DefElem*force_null = NULL; + DefElem*def; List *other_options = NIL; ListCell *cell; @@ -209,7 +210,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) */ foreach(cell, options_list) { - DefElem*def = (DefElem *) lfirst(cell); + def = (DefElem *) lfirst(cell); if (!is_valid_option(def->defname, catalog)) { @@ -290,10 +291,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) else if (strcmp(def->defname, "force_not_null") == 0) { if (force_not_null) -ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), - errhint("Option \"force_not_null\" supplied more than once for a column."))); +ReportDuplicateOptionError(def, NULL); force_not_null = def; /* Don't care what the value is, as long as it's a legal boolean */ (void) defGetBoolean(def); @@ -302,10 +300,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) else if (strcmp(def->defname, "force_null") == 0) { if (force_null) -ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), - errhint("Option \"force_null\" supplied more than once for a column."))); +ReportDuplicateOptionError(def, NULL); force_null = def; (void) defGetBoolean(def); } diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index 53392414f1..264cdc2730 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -59,6 +59,7 @@ #include "catalog/pg_ts_template.h" #include "catalog/pg_type.h" #include "commands/dbcommands.h" +#include "commands/defrem.h" #include "commands/event_trigger.h" #include "commands/extension.h" #include "commands/proclang.h" @@ -910,30 +911,25 @@ ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s List *nspnames = NIL; DefElem*drolespecs = NULL; DefElem*dnspnames = NULL; + DefElem*defel; AclMode all_privileges; const char *errormsg; /* Deconstruct the "options" part of the statement */ foreach(cell, stmt->options) { - DefElem*defel = (DefElem *) lfirst(cell); + defel = (DefElem *) lfirst(cell); if (strcmp(defel->defname, "schemas") == 0) { if (dnspnames) -ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), - parser_errposition(pstate, defel->location))); +ReportDuplicateOptionError(defel, pstate); dnspnames = defel; } else if (strcmp(defel->defname, "roles") == 0) { if (drolespecs) -ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), - parser_e
Re: Enhanced error message to include hint messages for redundant options error
On Thu, May 13, 2021 at 4:58 AM Alvaro Herrera 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 From c2362f0c7c75675639cba8603b8fb7acd38c506d Mon Sep 17 00:00:00 2001 From: vignesh Date: Mon, 26 Apr 2021 18:40:36 +0530 Subject: [PATCH v8] Enhance error message. Enhanced error message, so that the user can easily identify the error. --- contrib/file_fdw/file_fdw.c | 13 +-- src/backend/catalog/aclchk.c| 14 +-- src/backend/commands/copy.c | 57 +++--- src/backend/commands/dbcommands.c | 76 +++-- src/backend/commands/extension.c| 23 +--- src/backend/commands/foreigncmds.c | 21 ++-- src/backend/commands/functioncmds.c | 59 -- src/backend/commands/publicationcmds.c | 30 ++--- src/backend/commands/sequence.c | 48 ++-- src/backend/commands/subscriptioncmds.c | 66 +-- src/backend/commands/tablecmds.c| 4 +- src/backend/commands/typecmds.c | 34 ++ src/backend/commands/user.c | 118 +--- src/backend/parser/parse_utilcmd.c | 4 +- src/backend/replication/pgoutput/pgoutput.c | 23 ++-- src/backend/replication/walsender.c | 15 +-- src/backend/tcop/utility.c | 20 ++-- src/include/commands/defrem.h | 16 ++- src/include/commands/publicationcmds.h | 4 +- src/include/commands/subscriptioncmds.h | 4 +- src/include/commands/typecmds.h | 2 +- src/include/commands/user.h | 2 +- src/test/regress/expected/copy2.out | 24 ++-- src/test/regress/expected/foreign_data.out | 8 +- src/test/regress/expected/publication.out | 4 +- 25 files changed, 235 insertions(+), 454 deletions(-) diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c index 2c2f149fb0..f49dd47930 100644 --- a/contrib/file_fdw/file_fdw.c +++ b/contrib/file_fdw/file_fdw.c @@ -200,6 +200,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) char *filename = NULL; DefElem*force_not_null = NULL; DefElem*force_null = NULL; + DefElem*def; List *other_options = NIL; ListCell *cell; @@ -209,7 +210,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) */ foreach(cell, options_list) { - DefElem*def = (DefElem *) lfirst(cell); + def = (DefElem *) lfirst(cell); if (!is_valid_option(def->defname, catalog)) { @@ -290,10 +291,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) else if (strcmp(def->defname, "force_not_null") == 0) { if (force_not_null) -ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), - errhint("Option \"force_not_null\" supplied more than once for a column."))); +ReportDuplicateOptionError(def, NULL); force_not_null = def; /* Don't care what the value is, as long as it's a legal boolean */ (void) defGetBoolean(def); @@ -302,10 +300,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) else if (strcmp(def->defname, "force_null") == 0) { if (force_null) -ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), - errhint("Option \"force_null\" supplied more than once for a column."))); +ReportDuplicateOptionError(def, NULL); force_null = def; (void) defGetBoolean(def); } diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index 53392414f1..264cdc2730 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -59,6 +59,7 @@ #include "catalog/pg_ts_template.h" #include "catalog/pg_type.h" #include "commands/dbcommands.h" +#include "commands/defrem.h" #include "commands/event_trigger.h" #include "commands/extension.h" #include "commands/proclang.h" @@ -910,30 +911,25 @@ ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s List *nspnames = NIL; DefElem*drolespecs = NULL; DefElem*dnspnames = NULL; + DefE
Re: Enhanced error message to include hint messages for redundant options error
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
On Tue, May 11, 2021 at 6:54 PM vignesh C wrote: > > On Tue, May 11, 2021 at 2:47 AM Alvaro Herrera > 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
On Tue, May 11, 2021 at 2:47 AM Alvaro Herrera 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 From 9b325d6d85fca39ec0ff3f39735281f8a9fa647c Mon Sep 17 00:00:00 2001 From: vignesh Date: Mon, 26 Apr 2021 18:40:36 +0530 Subject: [PATCH v7] Enhance error message. Enhanced error message, so that the user can easily identify the error. --- contrib/file_fdw/file_fdw.c | 13 +-- src/backend/catalog/aclchk.c| 14 +-- src/backend/commands/copy.c | 57 +++--- src/backend/commands/dbcommands.c | 76 +++-- src/backend/commands/extension.c| 23 +--- src/backend/commands/foreigncmds.c | 21 ++-- src/backend/commands/functioncmds.c | 59 -- src/backend/commands/publicationcmds.c | 30 ++--- src/backend/commands/sequence.c | 48 ++-- src/backend/commands/subscriptioncmds.c | 66 +-- src/backend/commands/tablecmds.c| 4 +- src/backend/commands/typecmds.c | 34 ++ src/backend/commands/user.c | 118 +--- src/backend/parser/parse_utilcmd.c | 4 +- src/backend/replication/pgoutput/pgoutput.c | 23 ++-- src/backend/replication/walsender.c | 15 +-- src/backend/tcop/utility.c | 20 ++-- src/include/commands/defrem.h | 20 +++- src/include/commands/publicationcmds.h | 4 +- src/include/commands/subscriptioncmds.h | 4 +- src/include/commands/typecmds.h | 2 +- src/include/commands/user.h | 2 +- src/test/regress/expected/copy2.out | 24 ++-- src/test/regress/expected/foreign_data.out | 8 +- src/test/regress/expected/publication.out | 4 +- 25 files changed, 239 insertions(+), 454 deletions(-) diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c index 2c2f149fb0..f49dd47930 100644 --- a/contrib/file_fdw/file_fdw.c +++ b/contrib/file_fdw/file_fdw.c @@ -200,6 +200,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) char *filename = NULL; DefElem*force_not_null = NULL; DefElem*force_null = NULL; + DefElem*def; List *other_options = NIL; ListCell *cell; @@ -209,7 +210,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) */ foreach(cell, options_list) { - DefElem*def = (DefElem *) lfirst(cell); + def = (DefElem *) lfirst(cell); if (!is_valid_option(def->defname, catalog)) { @@ -290,10 +291,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) else if (strcmp(def->defname, "force_not_null") == 0) { if (force_not_null) -ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), - errhint("Option \"force_not_null\" supplied more than once for a column."))); +ReportDuplicateOptionError(def, NULL); force_not_null = def; /* Don't care what the value is, as long as it's a legal boolean */ (void) defGetBoolean(def); @@ -302,10 +300,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) else if (strcmp(def->defname, "force_null") == 0) { if (force_null) -ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), - errhint("Option \"force_null\" supplied more than once for a column."))); +ReportDuplicateOptionError(def, NULL); force_null = def; (void) defGetBoolean(def); } diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index e1573eb398..3933aba716 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -59,6 +59,7 @@ #include "catalog/pg_ts_template.h" #include "catalog/pg_type.h" #include "commands/dbcommands.h" +#include "commands/defrem.h" #include "commands/event_trigger.h" #include "commands/extension.h" #include "commands/proclang.h" @@ -910,30 +911,25 @@ ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s List *nspnames = NIL; DefElem*drolespecs = NULL; DefElem*dnspnames = NULL; + DefElem*defel; AclMode all_privileges; const char *errormsg; /* Deconstruct the "options" part of the statement */ foreach(cell, stmt->options) { - DefElem*defel = (DefElem *) lfirst(cell); + defel = (DefElem *) lfirst(cell); if (strcmp(defel->defname, "schemas") == 0) { if (dnspnames) -ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), -
Re: Enhanced error message to include hint messages for redundant options error
On Tue, May 11, 2021 at 2:47 AM Alvaro Herrera 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
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 Herrera39°49'30"S 73°17'W
Re: Enhanced error message to include hint messages for redundant options error
On Mon, May 10, 2021 at 6:00 AM houzj.f...@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 From e4db135ebba067c5ae0a53489acf687e4c6a6f33 Mon Sep 17 00:00:00 2001 From: vignesh Date: Mon, 26 Apr 2021 18:40:36 +0530 Subject: [PATCH v6] Enhance error message. Enhanced error message, so that the user can easily identify the error. --- contrib/file_fdw/file_fdw.c | 20 +-- src/backend/catalog/aclchk.c| 22 ++-- src/backend/commands/copy.c | 66 -- src/backend/commands/dbcommands.c | 90 +- src/backend/commands/extension.c| 27 ++-- src/backend/commands/foreigncmds.c | 30 +++-- src/backend/commands/functioncmds.c | 57 + src/backend/commands/publicationcmds.c | 39 +++--- src/backend/commands/sequence.c | 57 +++-- src/backend/commands/subscriptioncmds.c | 75 +-- src/backend/commands/tablecmds.c| 2 +- src/backend/commands/typecmds.c | 38 +++--- src/backend/commands/user.c | 131 +++- src/backend/parser/parse_utilcmd.c | 2 +- src/backend/replication/pgoutput/pgoutput.c | 31 +++-- src/backend/replication/walsender.c | 23 ++-- src/backend/tcop/utility.c | 20 +-- src/include/commands/defrem.h | 6 +- src/include/commands/publicationcmds.h | 4 +- src/include/commands/subscriptioncmds.h | 4 +- src/include/commands/typecmds.h | 2 +- src/include/commands/user.h | 2 +- src/test/regress/expected/copy2.out | 24 ++-- src/test/regress/expected/foreign_data.out | 8 +- src/test/regress/expected/publication.out | 4 +- 25 files changed, 353 insertions(+), 431 deletions(-) diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c index 2c2f149fb0..32398c227f 100644 --- a/contrib/file_fdw/file_fdw.c +++ b/contrib/file_fdw/file_fdw.c @@ -200,6 +200,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) char *filename = NULL; DefElem*force_not_null = NULL; DefElem*force_null = NULL; + DefElem*def; List *other_options = NIL; ListCell *cell; @@ -209,7 +210,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) */ foreach(cell, options_list) { - DefElem*def = (DefElem *) lfirst(cell); + def = (DefElem *) lfirst(cell); if (!is_valid_option(def->defname, catalog)) { @@ -290,10 +291,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) else if (strcmp(def->defname, "force_not_null") == 0) { if (force_not_null) -ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), -
RE: Enhanced error message to include hint messages for redundant options error
> > > > > > > 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
On Sat, May 8, 2021 at 7:06 PM vignesh C wrote: > > On Sat, May 8, 2021 at 2:20 PM Bharath Rupireddy > wrote: > > > > On Sat, May 8, 2021 at 12:01 PM houzj.f...@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
On Sat, May 8, 2021 at 2:20 PM Bharath Rupireddy wrote: > > On Sat, May 8, 2021 at 12:01 PM houzj.f...@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
On Sat, May 8, 2021 at 12:01 PM houzj.f...@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
> > > 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
On Mon, May 3, 2021 at 1:41 PM Dilip Kumar wrote: > > On Mon, May 3, 2021 at 12:08 PM Bharath Rupireddy > wrote: > > > > On Sun, May 2, 2021 at 8:44 PM vignesh C wrote: > > > > > > On Sat, May 1, 2021 at 9:02 PM Bharath Rupireddy > > > wrote: > > > > > > > > On Sat, May 1, 2021 at 7:25 PM vignesh C 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
On Mon, May 3, 2021 at 12:08 PM Bharath Rupireddy wrote: > > On Sun, May 2, 2021 at 8:44 PM vignesh C wrote: > > > > On Sat, May 1, 2021 at 9:02 PM Bharath Rupireddy > > wrote: > > > > > > On Sat, May 1, 2021 at 7:25 PM vignesh C 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
On Mon, May 3, 2021 at 12:08 PM Bharath Rupireddy wrote: > > On Sun, May 2, 2021 at 8:44 PM vignesh C wrote: > > > > On Sat, May 1, 2021 at 9:02 PM Bharath Rupireddy > > wrote: > > > > > > On Sat, May 1, 2021 at 7:25 PM vignesh C 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
On Sun, May 2, 2021 at 8:44 PM vignesh C wrote: > > On Sat, May 1, 2021 at 9:02 PM Bharath Rupireddy > wrote: > > > > On Sat, May 1, 2021 at 7:25 PM vignesh C 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
On Sat, May 1, 2021 at 9:02 PM Bharath Rupireddy wrote: > > On Sat, May 1, 2021 at 7:25 PM vignesh C 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
On Sat, May 1, 2021 at 10:54 PM Alvaro Herrera wrote: > > On 2021-May-01, vignesh C wrote: > > > On Thu, Apr 29, 2021 at 10:44 PM Alvaro Herrera > > 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 From 7c56cea92947b82107e2298f6d48d934df6fd7d8 Mon Sep 17 00:00:00 2001 From: vignesh Date: Mon, 26 Apr 2021 18:40:36 +0530 Subject: [PATCH v5] Enhance error message. Enhanced error message, so that the user can easily identify the error. --- contrib/file_fdw/file_fdw.c | 20 +-- src/backend/catalog/aclchk.c| 22 ++-- src/backend/commands/copy.c | 66 -- src/backend/commands/dbcommands.c | 90 +- src/backend/commands/extension.c| 27 ++-- src/backend/commands/foreigncmds.c | 30 +++-- src/backend/commands/functioncmds.c | 55 src/backend/commands/publicationcmds.c | 39 +++--- src/backend/commands/sequence.c | 57 +++-- src/backend/commands/subscriptioncmds.c | 75 +-- src/backend/commands/tablecmds.c| 2 +- src/backend/commands/typecmds.c | 38 +++--- src/backend/commands/user.c | 131 +++- src/backend/parser/parse_utilcmd.c | 2 +- src/backend/replication/pgoutput/pgoutput.c | 31 +++-- src/backend/replication/walsender.c | 23 ++-- src/backend/tcop/utility.c | 20 +-- src/include/commands/defrem.h | 6 +- src/include/commands/publicationcmds.h | 4 +- src/include/commands/subscriptioncmds.h | 4 +- src/include/commands/typecmds.h | 2 +- src/include/commands/user.h | 2 +- src/test/regress/expected/copy2.out | 24 ++-- src/test/regress/expected/foreign_data.out | 8 +- src/test/regress/expected/publication.out | 4 +- 25 files changed, 352 insertions(+), 430 deletions(-) diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c index 2c2f149fb0..f857d5af97 100644 --- a/contrib/file_fdw/file_fdw.c +++ b/contrib/file_fdw/file_fdw.c @@ -200,6 +200,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) char *filename = NULL; DefElem*force_not_null = NULL; DefElem*force_null = NULL; + DefElem*def; List *other_options = NIL; ListCell *cell; @@ -209,7 +210,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) */ foreach(cell, options_list) { - DefElem*def = (DefElem *) lfirst(cell); + def = (DefElem *) lfirst(cell); if (!is_valid_option(def->defname, catalog)) { @@ -290,10 +291,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) else if (strcmp(def->defname, "force_not_null") == 0) { if (force_not_null) -ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), - errhint("Option \"force_not_null\" supplied more than once for a column."))); +goto duplicate_error; force_not_null = def; /* Don't care what the value is, as long as it's a legal boolean */ (void) defGetBoolean(def); @@ -302,10 +300,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) else if (strcmp(def->defname, "force_null") == 0) { if (force_null) -ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), - errhint("Option \"force_null\" supplied more than once for a column."))); +goto duplicate_error; force_null = def; (void) defGetBoolean(def); } @@ -328,6 +323,13 @@ file_fdw_validator(PG_FUNCTION_ARGS) errmsg("either filename or program is required for file_fdw foreign tables"))); PG_RETURN_VOID(); + +duplicate_error: + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("option \"%s\" specified more than once", def->defname))); + PG_RETURN_VOID();/* keep compiler quiet */ + } /* diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index e1573eb398..be38488934 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/b
Re: Enhanced error message to include hint messages for redundant options error
Le dim. 2 mai 2021 à 18:31, vignesh C a écrit : > On Sun, May 2, 2021 at 4:27 AM Alvaro Herrera > 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
On Sun, May 2, 2021 at 4:27 AM Alvaro Herrera 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
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 Herrera39°49'30"S 73°17'W
Re: Enhanced error message to include hint messages for redundant options error
On Sat, May 1, 2021, 10:54 PM Alvaro Herrera wrote: > On 2021-May-01, vignesh C wrote: > On Thu, Apr 29, 2021 at 10:44 PM Alvaro Herrera > 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
On 2021-May-01, vignesh C wrote: > On Thu, Apr 29, 2021 at 10:44 PM Alvaro Herrera > 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 Herrera39°49'30"S 73°17'W
Re: Enhanced error message to include hint messages for redundant options error
On Sat, May 1, 2021 at 7:25 PM vignesh C 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
On Thu, Apr 29, 2021 at 10:44 PM Alvaro Herrera 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
On Sat, May 1, 2021 at 10:47 AM Dilip Kumar wrote: > > On Sat, May 1, 2021 at 10:43 AM Bharath Rupireddy > wrote: > > > > On Fri, Apr 30, 2021 at 2:49 PM Dilip Kumar 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 From b76a67be6c162d064f0a9cecd3ebc66d6c8fcbd9 Mon Sep 17 00:00:00 2001 From: vignesh Date: Mon, 26 Apr 2021 18:40:36 +0530 Subject: [PATCH v4] Enhance error message. Enhanced error message, so that the user can easily identify the error. --- contrib/file_fdw/file_fdw.c | 6 +-- src/backend/catalog/aclchk.c| 4 +- src/backend/commands/copy.c | 23 +- src/backend/commands/dbcommands.c | 28 ++-- src/backend/commands/extension.c| 8 ++-- src/backend/commands/foreigncmds.c | 4 +- src/backend/commands/functioncmds.c | 14 +++--- src/backend/commands/publicationcmds.c | 4 +- src/backend/commands/sequence.c | 18 src/backend/commands/subscriptioncmds.c | 18 src/backend/commands/tablecmds.c| 2 +- src/backend/commands/typecmds.c | 14 +++--- src/backend/commands/user.c | 48 ++--- src/backend/parser/parse_utilcmd.c | 2 +- src/backend/replication/pgoutput/pgoutput.c | 10 ++--- src/backend/replication/walsender.c | 6 +-- src/test/regress/expected/copy2.out | 24 ++- src/test/regress/expected/foreign_data.out | 4 +- src/test/regress/expected/publication.out | 2 +- 19 files changed, 120 insertions(+), 119 deletions(-) diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c index 2c2f149fb0..10aa2fca28 100644 --- a/contrib/file_fdw/file_fdw.c +++ b/contrib/file_fdw/file_fdw.c @@ -292,8 +292,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) if (force_not_null) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), - errhint("Option \"force_not_null\" supplied more than once for a column."))); + errmsg("option \"%s\" specified more than once", def->defname))); force_not_null = def; /* Don't care what the value is, as long as it's a legal boolean */ (void) defGetBoolean(def); @@ -304,8 +303,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) if (force_null) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), - errhint("Option \"force_null\" supplied more than once for a column."))); + errmsg("option \"%s\" specified more than once", def->defname))); force_null = def; (void) defGetBoolean(def); } diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index e1573eb398..7885587bfc 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -923,7 +923,7 @@ ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s if (dnspnames) 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))); dnspnames = defel; } @@ -932,7 +932,7 @@ ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s if (drolespecs) 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))); drolespecs = defel; } diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index 8265b981eb..2860f36f91 100644 --- a/src/backend/comman
Re: Enhanced error message to include hint messages for redundant options error
On Sat, May 1, 2021 at 10:43 AM Bharath Rupireddy wrote: > > On Fri, Apr 30, 2021 at 2:49 PM Dilip Kumar 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
On Fri, Apr 30, 2021 at 2:49 PM Dilip Kumar 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
On Fri, Apr 30, 2021 at 12:36 PM Bharath Rupireddy 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
On Fri, Apr 30, 2021 at 5:06 PM Bharath Rupireddy wrote: > > On Fri, Apr 30, 2021 at 11:23 AM Dilip Kumar wrote: > > > > On Fri, Apr 30, 2021 at 11:09 AM Bharath Rupireddy > > wrote: > > > > > > On Fri, Apr 30, 2021 at 10:51 AM Dilip Kumar > > > wrote: > > > > > > > > On Fri, Apr 30, 2021 at 10:43 AM Bharath Rupireddy > > > > wrote: > > > > > > > > > > On Fri, Apr 30, 2021 at 10:17 AM Dilip Kumar > > > > > 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 allbut 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
On Fri, Apr 30, 2021 at 11:23 AM Dilip Kumar wrote: > > On Fri, Apr 30, 2021 at 11:09 AM Bharath Rupireddy > wrote: > > > > On Fri, Apr 30, 2021 at 10:51 AM Dilip Kumar wrote: > > > > > > On Fri, Apr 30, 2021 at 10:43 AM Bharath Rupireddy > > > wrote: > > > > > > > > On Fri, Apr 30, 2021 at 10:17 AM Dilip Kumar > > > > 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 From 6ed153cdb45a0d41d3889dc7d80b3a743eb66725 Mon Sep 17 00:00:00 2001 From: vignesh Date: Mon, 26 Apr 2021 18:40:36 +0530 Subject: [PATCH v3] Enhance error message. Enhanced error message, so that the user can easily identify the error. --- contrib/file_fdw/file_fdw.c | 6 +-- src/backend/catalog/aclchk.c| 4 +- src/backend/commands/copy.c | 23 +- src/backend/commands/dbcommands.c | 28 ++-- src/backend/commands/extension.c| 8 ++-- src/backend/commands/foreigncmds.c | 4 +- src/backend/commands/functioncmds.c | 12 +++--- src/backend/commands/publicationcmds.c | 4 +- src/backend/commands/sequence.c | 18 src/backend/commands/subscriptioncmds.c | 18 src/backend/commands/tablecmds.c| 2 +- src/backend/commands/typecmds.c | 14 +++--- src/backend/commands/user.c | 48 ++--- src/backend/parser/parse_utilcmd.c | 2 +- src/backend/replication/pgoutput/pgoutput.c | 10 ++--- src/backend/replication/walsender.c | 6 +-- src/test/regress/expected/copy2.out | 24 ++- src/test/regress/expected/foreign_data.out | 4 +- src/test/regress/expected/publication.out | 2 +- 19 files changed, 119 insertions(+), 118 deletions(-) diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c index 2c2f149fb0..10aa2fca28 100644 --- a/contrib/file_fdw/file_fdw.c +++ b/contrib/file_fdw/file_fdw.c @@ -292,8 +292,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) if (force_not_null) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), - errhint("Option \"force_not_null\" supplied more than once for a column."))); + errmsg("option \"%s\" specified more than once", def->defname))); force_not_null = def; /* Don't care what the value is, as long as it's a legal boolean */ (void) defGetBoolean(def); @@ -304,8 +303,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) if (force_null) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), - errhint("Option \"force_null\" supplied more than once for a column."))); + errmsg("option \"%s\" specified more than once", def->defname))); force_null = def; (void) defGetBoolean(def); } dif
Re: Enhanced error message to include hint messages for redundant options error
On Fri, Apr 30, 2021 at 11:09 AM Bharath Rupireddy wrote: > > On Fri, Apr 30, 2021 at 10:51 AM Dilip Kumar wrote: > > > > On Fri, Apr 30, 2021 at 10:43 AM Bharath Rupireddy > > wrote: > > > > > > On Fri, Apr 30, 2021 at 10:17 AM Dilip Kumar > > > 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
On Fri, Apr 30, 2021 at 10:51 AM Dilip Kumar wrote: > > On Fri, Apr 30, 2021 at 10:43 AM Bharath Rupireddy > wrote: > > > > On Fri, Apr 30, 2021 at 10:17 AM Dilip Kumar 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
On Fri, Apr 30, 2021 at 10:43 AM Bharath Rupireddy wrote: > > On Fri, Apr 30, 2021 at 10:17 AM Dilip Kumar 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
On Fri, Apr 30, 2021 at 10:17 AM Dilip Kumar 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 v3-0001-Enhance-error-message.patch Description: Binary data v2-0001-compute_common_attribute.patch Description: Binary data
Re: Enhanced error message to include hint messages for redundant options error
On Fri, Apr 30, 2021 at 8:16 AM Bharath Rupireddy wrote: > > On Thu, Apr 29, 2021 at 10:44 PM Alvaro Herrera > 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
On Thu, Apr 29, 2021 at 10:44 PM Alvaro Herrera 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 v1-0001-compute_common_attribute.patch Description: Binary data
Re: Enhanced error message to include hint messages for redundant options error
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 Herrera39°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
On Mon, Apr 26, 2021 at 9:24 PM Alvaro Herrera 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 From 6ed153cdb45a0d41d3889dc7d80b3a743eb66725 Mon Sep 17 00:00:00 2001 From: vignesh Date: Mon, 26 Apr 2021 18:40:36 +0530 Subject: [PATCH v3] Enhance error message. Enhanced error message, so that the user can easily identify the error. --- contrib/file_fdw/file_fdw.c | 6 +-- src/backend/catalog/aclchk.c| 4 +- src/backend/commands/copy.c | 23 +- src/backend/commands/dbcommands.c | 28 ++-- src/backend/commands/extension.c| 8 ++-- src/backend/commands/foreigncmds.c | 4 +- src/backend/commands/functioncmds.c | 12 +++--- src/backend/commands/publicationcmds.c | 4 +- src/backend/commands/sequence.c | 18 src/backend/commands/subscriptioncmds.c | 18 src/backend/commands/tablecmds.c| 2 +- src/backend/commands/typecmds.c | 14 +++--- src/backend/commands/user.c | 48 ++--- src/backend/parser/parse_utilcmd.c | 2 +- src/backend/replication/pgoutput/pgoutput.c | 10 ++--- src/backend/replication/walsender.c | 6 +-- src/test/regress/expected/copy2.out | 24 ++- src/test/regress/expected/foreign_data.out | 4 +- src/test/regress/expected/publication.out | 2 +- 19 files changed, 119 insertions(+), 118 deletions(-) diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c index 2c2f149fb0..10aa2fca28 100644 --- a/contrib/file_fdw/file_fdw.c +++ b/contrib/file_fdw/file_fdw.c @@ -292,8 +292,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) if (force_not_null) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), - errhint("Option \"force_not_null\" supplied more than once for a column."))); + errmsg("option \"%s\" specified more than once", def->defname))); force_not_null = def; /* Don't care what the value is, as long as it's a legal boolean */ (void) defGetBoolean(def); @@ -304,8 +303,7 @@ file_fdw_validator(PG_FUNCTION_ARGS) if (force_null) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), - errhint("Option \"force_null\" supplied more than once for a column."))); + errmsg("option \"%s\" specified more than once", def->defname))); force_null = def; (void) defGetBoolean(def); } diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index e1573eb398..7885587bfc 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -923,7 +923,7 @@ ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s if (dnspnames) 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))); dnspnames = defel; } @@ -932,7 +932,7 @@ ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s if (drolespecs) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"), + errmsg("option \"%s\" specified more than once", defel->d
Re: Enhanced error message to include hint messages for redundant options error
On Tue, Apr 27, 2021 at 6:23 AM Bharath Rupireddy wrote: > > On Mon, Apr 26, 2021 at 9:10 PM Bharath Rupireddy > 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
On Mon, Apr 26, 2021 at 9:10 PM Bharath Rupireddy 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
On Mon, Apr 26, 2021 at 9:24 PM Alvaro Herrera 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
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 Herrera39°49'30"S 73°17'W It does it in a really, really complicated way why does it need to be complicated? Because it's MakeMaker.
Re: Enhanced error message to include hint messages for redundant options error
On Mon, Apr 26, 2021 at 8:06 PM Alvaro Herrera 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
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 really, I see PHP as like a strange amalgamation of C, Perl, Shell inflex: you know that "amalgam" means "mixture with mercury", more or less, right? i.e., "deadly poison"
Re: Enhanced error message to include hint messages for redundant options error
On Mon, Apr 26, 2021 at 7:02 PM vignesh C wrote: > > On Mon, Apr 26, 2021 at 5:49 PM Bharath Rupireddy > wrote: > > > > On Mon, Apr 26, 2021 at 5:29 PM vignesh C 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
On Mon, Apr 26, 2021 at 6:18 PM Dilip Kumar wrote: > > On Mon, Apr 26, 2021 at 5:49 PM Bharath Rupireddy > wrote: > > > > On Mon, Apr 26, 2021 at 5:29 PM vignesh C 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
On Mon, Apr 26, 2021 at 5:49 PM Bharath Rupireddy wrote: > > On Mon, Apr 26, 2021 at 5:29 PM vignesh C 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 From 19b33693873a9c6201a139370649f605978863f8 Mon Sep 17 00:00:00 2001 From: vignesh Date: Mon, 26 Apr 2021 18:40:36 +0530 Subject: [PATCH v2] Enhance error message. Enhanced error message, so that the user can easily identify the error. --- src/backend/commands/copy.c | 3 +- src/backend/commands/foreigncmds.c | 6 ++-- src/backend/commands/functioncmds.c | 6 ++-- src/backend/commands/publicationcmds.c | 6 ++-- src/backend/commands/subscriptioncmds.c | 28 +++-- src/backend/commands/tablecmds.c| 3 +- src/backend/commands/typecmds.c | 18 +++ src/backend/commands/user.c | 33 ++--- src/backend/parser/parse_utilcmd.c | 3 +- src/backend/replication/pgoutput/pgoutput.c | 15 ++ src/backend/replication/walsender.c | 9 -- src/test/regress/expected/copy2.out | 2 ++ src/test/regress/expected/foreign_data.out | 2 ++ src/test/regress/expected/publication.out | 1 + 14 files changed, 92 insertions(+), 43 deletions(-) diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index 8265b981eb..77a50652cc 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -469,7 +469,8 @@ ProcessCopyOptions(ParseState *pstate, if (opts_out->force_null) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"))); + errmsg("conflicting or redundant options"), + parser_errposition(pstate, defel->location))); if (defel->arg && IsA(defel->arg, List)) opts_out->force_null = castNode(List, defel->arg); else diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c index eb7103fd3b..83b969a5f8 100644 --- a/src/backend/commands/foreigncmds.c +++ b/src/backend/commands/foreigncmds.c @@ -536,7 +536,8 @@ parse_func_options(List *func_options, if (*handler_given) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"))); + errmsg("conflicting or redundant options"), + errdetail("Option \"handler\" specified more than once."))); *handler_given = true; *fdwhandler = lookup_fdw_handler_func(def); } @@ -545,7 +546,8 @@ parse_func_options(List *func_options, if (*validator_given) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"))); + errmsg("conflicting or redundant options"), + errdetail("Option \"validator\" specified more than once."))); *validator_given = true; *fdwvalidator = lookup_fdw_validator_func(def); } diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c index 9548287217..5508fad48f 100644 --- a/src/backend/commands/functioncmds.c +++ b/src/backend/commands/functioncmds.c @@ -2065,7 +2065,8 @@ ExecuteDoStmt(DoStmt *stmt, bool atomic) if (as_item) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"))); + errmsg("conflicting or redundant options"), + errdetail("
Re: Enhanced error message to include hint messages for redundant options error
On Mon, Apr 26, 2021 at 5:49 PM Bharath Rupireddy wrote: > > On Mon, Apr 26, 2021 at 5:29 PM vignesh C 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
On Mon, Apr 26, 2021 at 5:29 PM vignesh C 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
Enhanced error message to include hint messages for redundant options error
Hi, While reviewing one of the logical replication patches, I found that we do not include hint messages to display the actual option which has been specified more than once in case of redundant option error. I felt including this will help in easily identifying the error, users will not have to search through the statement to identify where the actual error is present. Attached a patch for the same. Thoughts? Regards, Vignesh From 73f670b9206e3f0240d56435f838c67f3e9fb681 Mon Sep 17 00:00:00 2001 From: vignesh Date: Mon, 26 Apr 2021 09:44:23 +0530 Subject: [PATCH] Enhance error message. Enhanced error message, so that the user can easily identify the error. --- src/backend/commands/copy.c | 3 +- src/backend/commands/foreigncmds.c | 6 ++-- src/backend/commands/functioncmds.c | 6 ++-- src/backend/commands/publicationcmds.c | 6 ++-- src/backend/commands/subscriptioncmds.c | 28 +++-- src/backend/commands/tablecmds.c| 3 +- src/backend/commands/typecmds.c | 18 +++ src/backend/commands/user.c | 33 ++--- src/backend/parser/parse_utilcmd.c | 3 +- src/backend/replication/pgoutput/pgoutput.c | 15 ++ src/backend/replication/walsender.c | 9 -- src/test/regress/expected/copy2.out | 2 ++ src/test/regress/expected/foreign_data.out | 2 ++ src/test/regress/expected/publication.out | 1 + 14 files changed, 92 insertions(+), 43 deletions(-) diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index 8265b981eb..77a50652cc 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -469,7 +469,8 @@ ProcessCopyOptions(ParseState *pstate, if (opts_out->force_null) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"))); + errmsg("conflicting or redundant options"), + parser_errposition(pstate, defel->location))); if (defel->arg && IsA(defel->arg, List)) opts_out->force_null = castNode(List, defel->arg); else diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c index eb7103fd3b..1239b0cafd 100644 --- a/src/backend/commands/foreigncmds.c +++ b/src/backend/commands/foreigncmds.c @@ -536,7 +536,8 @@ parse_func_options(List *func_options, if (*handler_given) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"))); + errmsg("conflicting or redundant options"), + errhint("Option \"handler\" specified more than once"))); *handler_given = true; *fdwhandler = lookup_fdw_handler_func(def); } @@ -545,7 +546,8 @@ parse_func_options(List *func_options, if (*validator_given) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"))); + errmsg("conflicting or redundant options"), + errhint("Option \"validator\" specified more than once"))); *validator_given = true; *fdwvalidator = lookup_fdw_validator_func(def); } diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c index 9548287217..6772bdbce9 100644 --- a/src/backend/commands/functioncmds.c +++ b/src/backend/commands/functioncmds.c @@ -2065,7 +2065,8 @@ ExecuteDoStmt(DoStmt *stmt, bool atomic) if (as_item) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"))); + errmsg("conflicting or redundant options"), + errhint("Option \"as\" specified more than once"))); as_item = defel; } else if (strcmp(defel->defname, "language") == 0) @@ -2073,7 +2074,8 @@ ExecuteDoStmt(DoStmt *stmt, bool atomic) if (language_item) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"))); + errmsg("conflicting or redundant options"), + errhint("Option \"language\" specified more than once"))); language_item = defel; } else diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c index 95c253c8e0..804082116b 100644 --- a/src/backend/commands/publicationcmds.c +++ b/src/backend/commands/publicationcmds.c @@ -87,7 +87,8 @@ parse_publication_options(List *options, if (*publish_given) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"))); + errmsg("conflicting or redundant options"), + errhint("Option \"publish\" specified more than once"))); /* * If publish option was given only the explicitly listed actions @@ -130,7 +131,8 @@ parse_publication_options(List *options, if (*publish_via_partition_root_given) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("conflicting or redundant options"))); + errmsg("conflicting or redundant options"), + errhint("Option \"publish_via_parti