Re: postgres_fdw hint messages
On Fri, Sep 16, 2022 at 03:54:53PM +0200, Peter Eisentraut wrote: > I don't think we need a verb there at all. I committed it without a verb. Thanks! -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: postgres_fdw hint messages
On 13.09.22 21:02, Nathan Bossart wrote: On Tue, Sep 13, 2022 at 08:32:43AM +0200, Peter Eisentraut wrote: I notice that for column misspellings, the hint is phrased "Perhaps you meant X." whereas here we have "Did you mean X?". Let's make that uniform. Good point. I attached a new version of the patch that uses the column phrasing. I wasn't sure whether "reference" was the right word to use in this context, but I used it for now for consistency with the column hints. I think "specify" or "use" would work as well. I don't think we need a verb there at all. I committed it without a verb.
Re: postgres_fdw hint messages
On Tue, Sep 13, 2022 at 08:32:43AM +0200, Peter Eisentraut wrote: > I notice that for column misspellings, the hint is phrased "Perhaps you > meant X." whereas here we have "Did you mean X?". Let's make that uniform. Good point. I attached a new version of the patch that uses the column phrasing. I wasn't sure whether "reference" was the right word to use in this context, but I used it for now for consistency with the column hints. I think "specify" or "use" would work as well. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com >From f9e5d136214e2b55c0bcf0b7b8ec2485aab0e7ba Mon Sep 17 00:00:00 2001 From: Nathan Bossart Date: Fri, 2 Sep 2022 14:03:26 -0700 Subject: [PATCH v4 1/1] Adjust assorted hint messages that list all valid options. Instead of listing all valid options, we now try to provide one that looks similar. Since this may be useful elsewhere, this change introduces a new set of functions that can be reused for similar purposes. --- contrib/dblink/dblink.c | 27 +++--- contrib/dblink/expected/dblink.out| 1 - contrib/file_fdw/expected/file_fdw.out| 2 - contrib/file_fdw/file_fdw.c | 24 -- .../postgres_fdw/expected/postgres_fdw.out| 3 +- contrib/postgres_fdw/option.c | 23 -- src/backend/foreign/foreign.c | 26 -- src/backend/utils/adt/varlena.c | 82 +++ src/include/utils/varlena.h | 12 +++ src/test/regress/expected/foreign_data.out| 6 +- 10 files changed, 159 insertions(+), 47 deletions(-) diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c index 3df3f9bbe9..8c26adb56c 100644 --- a/contrib/dblink/dblink.c +++ b/contrib/dblink/dblink.c @@ -2008,27 +2008,32 @@ dblink_fdw_validator(PG_FUNCTION_ARGS) { /* * Unknown option, or invalid option for the context specified, so - * complain about it. Provide a hint with list of valid options - * for the context. + * complain about it. Provide a hint with a valid option that + * looks similar, if there is one. */ - StringInfoData buf; const PQconninfoOption *opt; + const char *closest_match; + ClosestMatchState match_state; + bool has_valid_options = false; - initStringInfo(&buf); + initClosestMatch(&match_state, def->defname, 4); for (opt = options; opt->keyword; opt++) { if (is_valid_dblink_option(options, opt->keyword, context)) - appendStringInfo(&buf, "%s%s", - (buf.len > 0) ? ", " : "", - opt->keyword); +{ + has_valid_options = true; + updateClosestMatch(&match_state, opt->keyword); +} } + + closest_match = getClosestMatch(&match_state); ereport(ERROR, (errcode(ERRCODE_FDW_OPTION_NAME_NOT_FOUND), errmsg("invalid option \"%s\"", def->defname), - buf.len > 0 - ? errhint("Valid options in this context are: %s", - buf.data) - : errhint("There are no valid options in this context."))); + has_valid_options ? closest_match ? + errhint("Perhaps you meant to reference the option \"%s\".", + closest_match) : 0 : + errhint("There are no valid options in this context."))); } } diff --git a/contrib/dblink/expected/dblink.out b/contrib/dblink/expected/dblink.out index c7bde6ad07..14d015e4d5 100644 --- a/contrib/dblink/expected/dblink.out +++ b/contrib/dblink/expected/dblink.out @@ -897,7 +897,6 @@ $d$; CREATE USER MAPPING FOR public SERVER fdtest OPTIONS (server 'localhost'); -- fail, can't specify server here ERROR: invalid option "server" -HINT: Valid options in this context are: user, password, sslpassword CREATE USER MAPPING FOR public SERVER fdtest OPTIONS (user :'USER'); GRANT USAGE ON FOREIGN SERVER fdtest TO regress_dblink_user; GRANT EXECUTE ON FUNCTION dblink_connect_u(text, text) TO regress_dblink_user; diff --git a/contrib/file_fdw/expected/file_fdw.out b/contrib/file_fdw/expected/file_fdw.out index 261af1a8b5..36d76ba26c 100644 --- a/contrib/file_fdw/expected/file_fdw.out +++ b/contrib/file_fdw/expected/file_fdw.out @@ -166,7 +166,6 @@ ERROR: invalid option "force_not_null" HINT: There are no valid options in this context. CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (force_not_null '*'); -- ERROR ERROR: invalid option "force_not_null" -HINT: Valid options in this context are: filename, program, format, header, delimiter, quote, escape, null, encoding -- force_null is not allowed to be specified at any foreign object level: ALTER FOREIGN DATA WRAPPER file_fdw OPTIONS (ADD force_null '*'); -- ERROR ERROR: invalid option "force_null" @@ -179,7 +178,6 @@ ERROR: invalid option "force_null" HINT: There are no valid options in this context. CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (force_null '*'); -- ERROR ERROR: invalid option "force_null" -HINT: Valid options in this context are: filename, program, format, header, delimiter, quote,
Re: postgres_fdw hint messages
On 03.09.22 06:30, Nathan Bossart wrote: On Fri, Sep 02, 2022 at 10:06:54PM -0400, Tom Lane wrote: I think the distance limit of 5 is too loose though. I see that it accommodates examples like "passfile" for "password", which seems great at first glance; but it also allows fundamentally silly suggestions like "user" for "server" or "host" for "foo". We'd need something smarter than Levenshtein if we want to offer "passfile" for "password" without looking stupid on a whole lot of other cases --- those words seem close, but they are close semantically not textually. Yeah, it's really only useful for simple misspellings, but IMO even that is rather handy. I noticed that the parse_relation.c stuff excludes matches where more than half the characters are different, so I added that here and lowered the distance limit to 4. This seems to prevent the silly suggestions (e.g., "host" for "foo") while retaining the more believable ones (e.g., "passfile" for "password"), at least for the small set of examples covered in the tests. I think this code is compact enough and the hints it produces are reasonable, so I think we could go with it. I notice that for column misspellings, the hint is phrased "Perhaps you meant X." whereas here we have "Did you mean X?". Let's make that uniform.
Re: postgres_fdw hint messages
On Fri, Sep 02, 2022 at 10:06:54PM -0400, Tom Lane wrote: > I think the distance limit of 5 is too loose though. I see that > it accommodates examples like "passfile" for "password", which > seems great at first glance; but it also allows fundamentally > silly suggestions like "user" for "server" or "host" for "foo". > We'd need something smarter than Levenshtein if we want to offer > "passfile" for "password" without looking stupid on a whole lot > of other cases --- those words seem close, but they are close > semantically not textually. Yeah, it's really only useful for simple misspellings, but IMO even that is rather handy. I noticed that the parse_relation.c stuff excludes matches where more than half the characters are different, so I added that here and lowered the distance limit to 4. This seems to prevent the silly suggestions (e.g., "host" for "foo") while retaining the more believable ones (e.g., "passfile" for "password"), at least for the small set of examples covered in the tests. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com >From 1ce0024357343cf4c1436cd74ed20e304d673762 Mon Sep 17 00:00:00 2001 From: Nathan Bossart Date: Fri, 2 Sep 2022 14:03:26 -0700 Subject: [PATCH v3 1/1] Adjust assorted hint messages that list all valid options. Instead of listing all valid options, we now try to provide one that looks similar. Since this may be useful elsewhere, this change introduces a new set of functions that can be reused for similar purposes. --- contrib/dblink/dblink.c | 26 +++--- contrib/dblink/expected/dblink.out| 1 - contrib/file_fdw/expected/file_fdw.out| 2 - contrib/file_fdw/file_fdw.c | 23 -- .../postgres_fdw/expected/postgres_fdw.out| 3 +- contrib/postgres_fdw/option.c | 22 +++-- src/backend/foreign/foreign.c | 25 -- src/backend/utils/adt/varlena.c | 82 +++ src/include/utils/varlena.h | 12 +++ src/test/regress/expected/foreign_data.out| 6 +- 10 files changed, 155 insertions(+), 47 deletions(-) diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c index e323fdd0e6..5e2d7b4c70 100644 --- a/contrib/dblink/dblink.c +++ b/contrib/dblink/dblink.c @@ -2008,27 +2008,31 @@ dblink_fdw_validator(PG_FUNCTION_ARGS) { /* * Unknown option, or invalid option for the context specified, so - * complain about it. Provide a hint with list of valid options - * for the context. + * complain about it. Provide a hint with a valid option that + * looks similar, if there is one. */ - StringInfoData buf; const PQconninfoOption *opt; + const char *closest_match; + ClosestMatchState match_state; + bool has_valid_options = false; - initStringInfo(&buf); + initClosestMatch(&match_state, def->defname, 4); for (opt = options; opt->keyword; opt++) { if (is_valid_dblink_option(options, opt->keyword, context)) - appendStringInfo(&buf, "%s%s", - (buf.len > 0) ? ", " : "", - opt->keyword); +{ + has_valid_options = true; + updateClosestMatch(&match_state, opt->keyword); +} } + + closest_match = getClosestMatch(&match_state); ereport(ERROR, (errcode(ERRCODE_FDW_OPTION_NAME_NOT_FOUND), errmsg("invalid option \"%s\"", def->defname), - buf.len > 0 - ? errhint("Valid options in this context are: %s", - buf.data) - : errhint("There are no valid options in this context."))); + has_valid_options ? closest_match ? + errhint("Did you mean \"%s\"?", closest_match) : 0 : + errhint("There are no valid options in this context."))); } } diff --git a/contrib/dblink/expected/dblink.out b/contrib/dblink/expected/dblink.out index c7bde6ad07..14d015e4d5 100644 --- a/contrib/dblink/expected/dblink.out +++ b/contrib/dblink/expected/dblink.out @@ -897,7 +897,6 @@ $d$; CREATE USER MAPPING FOR public SERVER fdtest OPTIONS (server 'localhost'); -- fail, can't specify server here ERROR: invalid option "server" -HINT: Valid options in this context are: user, password, sslpassword CREATE USER MAPPING FOR public SERVER fdtest OPTIONS (user :'USER'); GRANT USAGE ON FOREIGN SERVER fdtest TO regress_dblink_user; GRANT EXECUTE ON FUNCTION dblink_connect_u(text, text) TO regress_dblink_user; diff --git a/contrib/file_fdw/expected/file_fdw.out b/contrib/file_fdw/expected/file_fdw.out index 261af1a8b5..36d76ba26c 100644 --- a/contrib/file_fdw/expected/file_fdw.out +++ b/contrib/file_fdw/expected/file_fdw.out @@ -166,7 +166,6 @@ ERROR: invalid option "force_not_null" HINT: There are no valid options in this context. CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (force_not_null '*'); -- ERROR ERROR: invalid option "force_not_null" -HINT: Valid options in this context are: filename, program, format, header, delimiter, quote, escape, null, encoding -- force_null is not a
Re: postgres_fdw hint messages
Michael Paquier writes: > On Fri, Sep 02, 2022 at 02:26:09PM -0700, Nathan Bossart wrote: >> * I chose a Levenshtein distance of 5 as the threshold of closeness for the >> hint messages. This felt lenient, but it should hopefully still filter out >> some of the more ridiculous suggestions. However, it's still little more >> than a wild guess, so if folks think the threshold needs to be higher or >> lower, I'd readily change it. > Hmm. FWIW I would tend toward simplifying all this code and just drop > all the hints rather than increasing the dependency to more > levenshtein calculations in those error code paths, which is what > Peter E has posted. Personally I'm not a huge fan of this style of hint either. However, people seem to like the ones for misspelled column names, so I'm betting there will be a majority in favor of this one too. I think the distance limit of 5 is too loose though. I see that it accommodates examples like "passfile" for "password", which seems great at first glance; but it also allows fundamentally silly suggestions like "user" for "server" or "host" for "foo". We'd need something smarter than Levenshtein if we want to offer "passfile" for "password" without looking stupid on a whole lot of other cases --- those words seem close, but they are close semantically not textually. regards, tom lane
Re: postgres_fdw hint messages
On Fri, Sep 02, 2022 at 02:26:09PM -0700, Nathan Bossart wrote: > Here is a new patch. Two notes: > > * I considered whether to try to unite this new functionality with the > existing stuff in parse_relation.c, but the existing code looked a bit too > specialized. > > * I chose a Levenshtein distance of 5 as the threshold of closeness for the > hint messages. This felt lenient, but it should hopefully still filter out > some of the more ridiculous suggestions. However, it's still little more > than a wild guess, so if folks think the threshold needs to be higher or > lower, I'd readily change it. Hmm. FWIW I would tend toward simplifying all this code and just drop all the hints rather than increasing the dependency to more levenshtein calculations in those error code paths, which is what Peter E has posted. -- Michael signature.asc Description: PGP signature
Re: postgres_fdw hint messages
On Thu, Sep 01, 2022 at 04:31:20PM -0700, Nathan Bossart wrote: > On Thu, Sep 01, 2022 at 07:08:49PM -0400, Tom Lane wrote: >> (1) there probably needs to be some threshold of closeness, so we don't >> offer "foobar" when the user wrote "huh" > > Agreed. > >> (2) there are several places doing this now, and there will no doubt >> be more later, so we need to try to abstract the logic so it can be >> shared. > > Will do. Here is a new patch. Two notes: * I considered whether to try to unite this new functionality with the existing stuff in parse_relation.c, but the existing code looked a bit too specialized. * I chose a Levenshtein distance of 5 as the threshold of closeness for the hint messages. This felt lenient, but it should hopefully still filter out some of the more ridiculous suggestions. However, it's still little more than a wild guess, so if folks think the threshold needs to be higher or lower, I'd readily change it. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com >From c3c7793daf7e6b225605a44a1faead2f7678bca3 Mon Sep 17 00:00:00 2001 From: Nathan Bossart Date: Fri, 2 Sep 2022 14:03:26 -0700 Subject: [PATCH v2 1/1] Adjust assorted hint messages that list all valid options. Instead of listing all valid options, we now try to provide one that looks similar. Since this may be useful elsewhere, this change introduces a new set of functions that can be reused for similar purposes. --- contrib/dblink/dblink.c | 26 +++--- contrib/dblink/expected/dblink.out| 2 +- contrib/file_fdw/expected/file_fdw.out| 2 - contrib/file_fdw/file_fdw.c | 23 -- .../postgres_fdw/expected/postgres_fdw.out| 4 +- contrib/postgres_fdw/option.c | 22 +++-- src/backend/foreign/foreign.c | 25 -- src/backend/utils/adt/varlena.c | 82 +++ src/include/utils/varlena.h | 12 +++ src/test/regress/expected/foreign_data.out| 8 +- 10 files changed, 159 insertions(+), 47 deletions(-) diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c index e323fdd0e6..c134f73adb 100644 --- a/contrib/dblink/dblink.c +++ b/contrib/dblink/dblink.c @@ -2008,27 +2008,31 @@ dblink_fdw_validator(PG_FUNCTION_ARGS) { /* * Unknown option, or invalid option for the context specified, so - * complain about it. Provide a hint with list of valid options - * for the context. + * complain about it. Provide a hint with a valid option that + * looks similar, if there is one. */ - StringInfoData buf; const PQconninfoOption *opt; + const char *closest_match; + ClosestMatchState match_state; + bool has_valid_options = false; - initStringInfo(&buf); + initClosestMatch(&match_state, def->defname, 5); for (opt = options; opt->keyword; opt++) { if (is_valid_dblink_option(options, opt->keyword, context)) - appendStringInfo(&buf, "%s%s", - (buf.len > 0) ? ", " : "", - opt->keyword); +{ + has_valid_options = true; + updateClosestMatch(&match_state, opt->keyword); +} } + + closest_match = getClosestMatch(&match_state); ereport(ERROR, (errcode(ERRCODE_FDW_OPTION_NAME_NOT_FOUND), errmsg("invalid option \"%s\"", def->defname), - buf.len > 0 - ? errhint("Valid options in this context are: %s", - buf.data) - : errhint("There are no valid options in this context."))); + has_valid_options ? closest_match ? + errhint("Did you mean \"%s\"?", closest_match) : 0 : + errhint("There are no valid options in this context."))); } } diff --git a/contrib/dblink/expected/dblink.out b/contrib/dblink/expected/dblink.out index c7bde6ad07..2b874fc450 100644 --- a/contrib/dblink/expected/dblink.out +++ b/contrib/dblink/expected/dblink.out @@ -897,7 +897,7 @@ $d$; CREATE USER MAPPING FOR public SERVER fdtest OPTIONS (server 'localhost'); -- fail, can't specify server here ERROR: invalid option "server" -HINT: Valid options in this context are: user, password, sslpassword +HINT: Did you mean "user"? CREATE USER MAPPING FOR public SERVER fdtest OPTIONS (user :'USER'); GRANT USAGE ON FOREIGN SERVER fdtest TO regress_dblink_user; GRANT EXECUTE ON FUNCTION dblink_connect_u(text, text) TO regress_dblink_user; diff --git a/contrib/file_fdw/expected/file_fdw.out b/contrib/file_fdw/expected/file_fdw.out index 261af1a8b5..36d76ba26c 100644 --- a/contrib/file_fdw/expected/file_fdw.out +++ b/contrib/file_fdw/expected/file_fdw.out @@ -166,7 +166,6 @@ ERROR: invalid option "force_not_null" HINT: There are no valid options in this context. CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (force_not_null '*'); -- ERROR ERROR: invalid option "force_not_null" -HINT: Valid options in this context are: filename, program, format, header, delimiter, quote, escape, null, encoding -- force_null is not allowed to be specified at any for
Re: postgres_fdw hint messages
On Thu, Sep 01, 2022 at 07:08:49PM -0400, Tom Lane wrote: > (1) there probably needs to be some threshold of closeness, so we don't > offer "foobar" when the user wrote "huh" Agreed. > (2) there are several places doing this now, and there will no doubt > be more later, so we need to try to abstract the logic so it can be > shared. Will do. I'm also considering checking whether the user-provided string is longer than MAX_LEVENSHTEIN_STRLEN so that we can avoid the ERROR from varstr_levenshtein(). Or perhaps varstr_levenshtein() could indicate that the string is too long without ERROR-ing (e.g., by returning -1). If the user-provided string is too long, we'd just omit the hint. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: postgres_fdw hint messages
Nathan Bossart writes: > On Fri, Aug 26, 2022 at 12:35:38PM -0400, Tom Lane wrote: >> I vote for just dropping all these hints for now, while leaving the >> door open for anyone who wants to write closest-match-offering code. > Here is a quickly-hacked-together proof-of-concept for using Levenshtein > distances to determine which option to include in the hint. Would > something like this suffice? If so, I will work on polishing it up a bit. Seems reasonable to me, but (1) there probably needs to be some threshold of closeness, so we don't offer "foobar" when the user wrote "huh" (2) there are several places doing this now, and there will no doubt be more later, so we need to try to abstract the logic so it can be shared. regards, tom lane
Re: postgres_fdw hint messages
On Fri, Aug 26, 2022 at 12:35:38PM -0400, Tom Lane wrote: > Peter also mentioned the possibility of "did you mean" with a closest > match offered. That seems like a reasonable idea if someone > is motivated to create the code, which I'm not. > > I vote for just dropping all these hints for now, while leaving the > door open for anyone who wants to write closest-match-offering code. Here is a quickly-hacked-together proof-of-concept for using Levenshtein distances to determine which option to include in the hint. Would something like this suffice? If so, I will work on polishing it up a bit. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c index e323fdd0e6..7fe42cb732 100644 --- a/contrib/dblink/dblink.c +++ b/contrib/dblink/dblink.c @@ -2006,28 +2006,31 @@ dblink_fdw_validator(PG_FUNCTION_ARGS) if (!is_valid_dblink_option(options, def->defname, context)) { - /* - * Unknown option, or invalid option for the context specified, so - * complain about it. Provide a hint with list of valid options - * for the context. - */ - StringInfoData buf; - const PQconninfoOption *opt; + int min_dist = -1; + char *hint = NULL; - initStringInfo(&buf); - for (opt = options; opt->keyword; opt++) + for (const PQconninfoOption *opt = options; opt->keyword; opt++) { if (is_valid_dblink_option(options, opt->keyword, context)) - appendStringInfo(&buf, "%s%s", - (buf.len > 0) ? ", " : "", - opt->keyword); +{ + int dist; + + dist = varstr_levenshtein(def->defname, strlen(def->defname), + opt->keyword, strlen(opt->keyword), + 1, 1, 1, false); + if (min_dist == -1 || dist < min_dist) + { + min_dist = dist; + hint = opt->keyword; + } +} } + ereport(ERROR, (errcode(ERRCODE_FDW_OPTION_NAME_NOT_FOUND), errmsg("invalid option \"%s\"", def->defname), - buf.len > 0 - ? errhint("Valid options in this context are: %s", - buf.data) + hint != NULL + ? errhint("Did you mean \"%s\"?", hint) : errhint("There are no valid options in this context."))); } } diff --git a/contrib/dblink/expected/dblink.out b/contrib/dblink/expected/dblink.out index c7bde6ad07..2b874fc450 100644 --- a/contrib/dblink/expected/dblink.out +++ b/contrib/dblink/expected/dblink.out @@ -897,7 +897,7 @@ $d$; CREATE USER MAPPING FOR public SERVER fdtest OPTIONS (server 'localhost'); -- fail, can't specify server here ERROR: invalid option "server" -HINT: Valid options in this context are: user, password, sslpassword +HINT: Did you mean "user"? CREATE USER MAPPING FOR public SERVER fdtest OPTIONS (user :'USER'); GRANT USAGE ON FOREIGN SERVER fdtest TO regress_dblink_user; GRANT EXECUTE ON FUNCTION dblink_connect_u(text, text) TO regress_dblink_user; diff --git a/contrib/file_fdw/expected/file_fdw.out b/contrib/file_fdw/expected/file_fdw.out index 261af1a8b5..b780b1a13c 100644 --- a/contrib/file_fdw/expected/file_fdw.out +++ b/contrib/file_fdw/expected/file_fdw.out @@ -166,7 +166,7 @@ ERROR: invalid option "force_not_null" HINT: There are no valid options in this context. CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (force_not_null '*'); -- ERROR ERROR: invalid option "force_not_null" -HINT: Valid options in this context are: filename, program, format, header, delimiter, quote, escape, null, encoding +HINT: Did you mean "format"? -- force_null is not allowed to be specified at any foreign object level: ALTER FOREIGN DATA WRAPPER file_fdw OPTIONS (ADD force_null '*'); -- ERROR ERROR: invalid option "force_null" @@ -179,7 +179,7 @@ ERROR: invalid option "force_null" HINT: There are no valid options in this context. CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (force_null '*'); -- ERROR ERROR: invalid option "force_null" -HINT: Valid options in this context are: filename, program, format, header, delimiter, quote, escape, null, encoding +HINT: Did you mean "null"? -- basic query tests SELECT * FROM agg_text WHERE b > 10.0 ORDER BY a; a | b diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c index 4773cadec0..49dec5b7bf 100644 --- a/contrib/file_fdw/file_fdw.c +++ b/contrib/file_fdw/file_fdw.c @@ -37,6 +37,7 @@ #include "utils/memutils.h" #include "utils/rel.h" #include "utils/sampling.h" +#include "utils/varlena.h" PG_MODULE_MAGIC; @@ -213,27 +214,31 @@ file_fdw_validator(PG_FUNCTION_ARGS) if (!is_valid_option(def->defname, catalog)) { - const struct FileFdwOption *opt; - StringInfoData buf; + int min_dist = -1; + const char *hint = NULL; - /* - * Unknown option specified, complain about it. Provide a hint - * with list of valid options for the object. - */ - initStringInfo(&buf); - for (opt = valid_options; opt->optname; opt++) + for (const struct FileFdwOption *opt = valid_options; opt->
Re: postgres_fdw hint messages
On 25.08.22 15:42, Peter Eisentraut wrote: It's also questionable how useful this hint is in its current form, considering how long it is and that the options are in an implementation-dependent order. Possible changes: - Remove all the hints like this from foreign data commands. It appears that there was a strong preference toward this solution, so that's what I implemented in the updated patch set. (Considering the hints that are removed in the tests cases, I don't think this loses any value. What might be useful in practice instead is something like "the option you specified on this foreign server should actually be specified on a user mapping or foreign table", but this would take a fair amount of code to cover a reasonable set of cases, so I'll leave this as a future exercise.)From c079cf33ad8033875a6e18b9cff06e47330007ac Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Thu, 25 Aug 2022 15:31:10 +0200 Subject: [PATCH v2 1/2] postgres_fdw: Remove useless DO block in test --- contrib/postgres_fdw/expected/postgres_fdw.out | 8 +--- contrib/postgres_fdw/sql/postgres_fdw.sql | 6 +- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 7bf35602b0..a42c9720c3 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -9621,15 +9621,9 @@ ERROR: password is required DETAIL: Non-superusers must provide a password in the user mapping. -- If we add a password to the connstr it'll fail, because we don't allow passwords -- in connstrs only in user mappings. -DO $d$ -BEGIN -EXECUTE $$ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw')$$; -END; -$d$; +ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw'); ERROR: invalid option "password" HINT: Valid options in this context are: service, passfile, channel_binding, connect_timeout, dbname, host, hostaddr, port, options, application_name, keepalives, keepalives_idle, keepalives_interval, keepalives_count, tcp_user_timeout, sslmode, sslcompression, sslcert, sslkey, sslrootcert, sslcrl, sslcrldir, sslsni, requirepeer, ssl_min_protocol_version, ssl_max_protocol_version, gssencmode, krbsrvname, gsslib, target_session_attrs, use_remote_estimate, fdw_startup_cost, fdw_tuple_cost, extensions, updatable, truncatable, fetch_size, batch_size, async_capable, parallel_commit, keep_connections -CONTEXT: SQL statement "ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw')" -PL/pgSQL function inline_code_block line 3 at EXECUTE -- If we add a password for our user mapping instead, we should get a different -- error because the password wasn't actually *used* when we run with trust auth. -- diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 42735ae78a..63581a457d 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -2886,11 +2886,7 @@ CREATE FOREIGN TABLE pg_temp.ft1_nopw ( -- If we add a password to the connstr it'll fail, because we don't allow passwords -- in connstrs only in user mappings. -DO $d$ -BEGIN -EXECUTE $$ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw')$$; -END; -$d$; +ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw'); -- If we add a password for our user mapping instead, we should get a different -- error because the password wasn't actually *used* when we run with trust auth. -- 2.37.1 From eef24034a75e674e7e4a929557906395eab7673f Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Tue, 30 Aug 2022 09:08:53 +0200 Subject: [PATCH v2 2/2] Remove hints from FDW's invalid option errors The FDWs provided in tree all had code to list valid options in an error message hint if an invalid option was given to a forein-data-related command. These lists have in some cases gotten extremely bulky, and in general they don't really help the user correct their command. (They might help in case of typos, but not if there is a semantic misunderstanding about how the foreign-data setup should be configured.) So per discussion, remove these hints. Discussion: https://www.postgresql.org/message-id/flat/b1f9f399-3a1a-b554-283f-4ae7f34608e2%40enterprisedb.com --- contrib/dblink/dblink.c | 24 +-- contrib/dblink/expected/dblink.out| 2 -- contrib/file_fdw/expected/file_fdw.out| 8 --- contrib/file_fdw/file_fdw.c | 23 +- .../postgres_fdw/expected/postgres_fdw.out| 3 --- contrib/postgres_fdw/option.c | 23 +- src/backend/foreign/foreign.c | 19 +-- src/test/regress/expected/foreign_data.out| 5 8 files changed, 4 insertions(+), 103 deletions(-) diff --git a/contrib/dblink/dblink.c b/contrib/d
Re: postgres_fdw hint messages
Robert Haas writes: > On Thu, Aug 25, 2022 at 9:42 AM Peter Eisentraut > wrote: >> HINT: Valid options in this context are: service, passfile, >> channel_binding, connect_timeout, dbname, host, hostaddr, port, options, >> application_name, keepalives, keepalives_idle, keepalives_interval, >> keepalives_count, tcp_user_timeout, sslmode, sslcompression, sslcert, >> sslkey, sslrootcert, sslcrl, sslcrldir, sslsni, requirepeer, >> ssl_min_protocol_version, ssl_max_protocol_version, gssencmode, >> krbsrvname, gsslib, target_session_attrs, use_remote_estimate, >> fdw_startup_cost, fdw_tuple_cost, extensions, updatable, truncatable, >> fetch_size, batch_size, async_capable, parallel_commit, keep_connections >> >> This annoys developers who are working on libpq connection options, >> because any option added, removed, or changed causes this test to need >> to be updated. >> >> It's also questionable how useful this hint is in its current form, >> considering how long it is and that the options are in an >> implementation-dependent order. > I think the place to list the legal options is in the documentation, > not the HINT. I think listing them in a hint is reasonable as long as the hint doesn't get longer than a line or two. This one is entirely out of hand, so I agree with just dropping it. Note that there is essentially identical code in dblink, file_fdw, and backend/foreign/foreign.c. Do we want to nuke them all? Or maybe make a policy decision to suppress such HINTs when there are more than ~10 matches? (The latter policy would likely eventually end by always suppressing everything...) Peter also mentioned the possibility of "did you mean" with a closest match offered. That seems like a reasonable idea if someone is motivated to create the code, which I'm not. I vote for just dropping all these hints for now, while leaving the door open for anyone who wants to write closest-match-offering code. regards, tom lane
Re: postgres_fdw hint messages
On Thu, Aug 25, 2022 at 9:42 AM Peter Eisentraut wrote: > The postgres_fdw tests contain this (as amended by patch 0001): > > ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw'); > ERROR: invalid option "password" > HINT: Valid options in this context are: service, passfile, > channel_binding, connect_timeout, dbname, host, hostaddr, port, options, > application_name, keepalives, keepalives_idle, keepalives_interval, > keepalives_count, tcp_user_timeout, sslmode, sslcompression, sslcert, > sslkey, sslrootcert, sslcrl, sslcrldir, sslsni, requirepeer, > ssl_min_protocol_version, ssl_max_protocol_version, gssencmode, > krbsrvname, gsslib, target_session_attrs, use_remote_estimate, > fdw_startup_cost, fdw_tuple_cost, extensions, updatable, truncatable, > fetch_size, batch_size, async_capable, parallel_commit, keep_connections > > This annoys developers who are working on libpq connection options, > because any option added, removed, or changed causes this test to need > to be updated. > > It's also questionable how useful this hint is in its current form, > considering how long it is and that the options are in an > implementation-dependent order. I think the place to list the legal options is in the documentation, not the HINT. -- Robert Haas EDB: http://www.enterprisedb.com
Re: postgres_fdw hint messages
On Thu, Aug 25, 2022 at 6:42 PM Peter Eisentraut < peter.eisentr...@enterprisedb.com> wrote: > The postgres_fdw tests contain this (as amended by patch 0001): > > ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw'); > ERROR: invalid option "password" > HINT: Valid options in this context are: service, passfile, > channel_binding, connect_timeout, dbname, host, hostaddr, port, options, > application_name, keepalives, keepalives_idle, keepalives_interval, > keepalives_count, tcp_user_timeout, sslmode, sslcompression, sslcert, > sslkey, sslrootcert, sslcrl, sslcrldir, sslsni, requirepeer, > ssl_min_protocol_version, ssl_max_protocol_version, gssencmode, > krbsrvname, gsslib, target_session_attrs, use_remote_estimate, > fdw_startup_cost, fdw_tuple_cost, extensions, updatable, truncatable, > fetch_size, batch_size, async_capable, parallel_commit, keep_connections > > This annoys developers who are working on libpq connection options, > because any option added, removed, or changed causes this test to need > to be updated. > > It's also questionable how useful this hint is in its current form, > considering how long it is and that the options are in an > implementation-dependent order. > > Thanks Peter, for looking at that; this HINT message is growing over time. In my opinion, we should hide the complete message in case of an invalid option. But try to show dependent options; for example, if someone specify "sslcrl" and that option require some more options, then show the HINT of that options. Possible changes: > > - Hide the hint from this particular test (done in the attached patches). > > > - Keep the hint, but maybe make it sorted? > > - Remove all the hints like this from foreign data commands. > > - Don't show the hint when there are more than N valid options. > > - Do some kind of "did you mean" like we have for column names. > > Thoughts? -- Ibrar Ahmed
Re: postgres_fdw hint messages
Em qui., 25 de ago. de 2022 às 14:31, Ranier Vilela escreveu: > >The postgres_fdw tests contain this (as amended by patch 0001): > > >ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw'); > >ERROR: invalid option "password" > >HINT: Valid options in this context are: service, passfile, > >channel_binding, connect_timeout, dbname, host, hostaddr, port, options, > >application_name, keepalives, keepalives_idle, keepalives_interval, > >keepalives_count, tcp_user_timeout, sslmode, sslcompression, sslcert, > >sslkey, sslrootcert, sslcrl, sslcrldir, sslsni, requirepeer, > >ssl_min_protocol_version, ssl_max_protocol_version, gssencmode, > >krbsrvname, gsslib, target_session_attrs, use_remote_estimate, > >fdw_startup_cost, fdw_tuple_cost, extensions, updatable, truncatable, > >fetch_size, batch_size, async_capable, parallel_commit, keep_connections > > >This annoys developers who are working on libpq connection options, > >because any option added, removed, or changed causes this test to need > >to be updated. > > >It's also questionable how useful this hint is in its current form, > >considering how long it is and that the options are in an > >implementation-dependent order. > > >Possible changes: > > >- Hide the hint from this particular test (done in the attached patches). > +1 > I vote for this option. > Less work for future developers changes, I think worth the effort. > > Anyway, in alphabetical order, it's a lot easier for users to read. > > Patch attached. > Little tweak in the comments. regards, Ranier Vilela > > v1-0003-reorder-alphabetical-libpq-options.patch Description: Binary data
re: postgres_fdw hint messages
>The postgres_fdw tests contain this (as amended by patch 0001): >ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw'); >ERROR: invalid option "password" >HINT: Valid options in this context are: service, passfile, >channel_binding, connect_timeout, dbname, host, hostaddr, port, options, >application_name, keepalives, keepalives_idle, keepalives_interval, >keepalives_count, tcp_user_timeout, sslmode, sslcompression, sslcert, >sslkey, sslrootcert, sslcrl, sslcrldir, sslsni, requirepeer, >ssl_min_protocol_version, ssl_max_protocol_version, gssencmode, >krbsrvname, gsslib, target_session_attrs, use_remote_estimate, >fdw_startup_cost, fdw_tuple_cost, extensions, updatable, truncatable, >fetch_size, batch_size, async_capable, parallel_commit, keep_connections >This annoys developers who are working on libpq connection options, >because any option added, removed, or changed causes this test to need >to be updated. >It's also questionable how useful this hint is in its current form, >considering how long it is and that the options are in an >implementation-dependent order. >Possible changes: >- Hide the hint from this particular test (done in the attached patches). +1 I vote for this option. Less work for future developers changes, I think worth the effort. Anyway, in alphabetical order, it's a lot easier for users to read. Patch attached. regards, Ranier Vilela 0003-reorder-alphabetical-libpq-options.patch Description: Binary data