($INBOX_DIR/description missing)help / color / mirror / Atom feed
[PATCH v4 1/1] Adjust assorted hint messages that list all valid options. 8+ messages / 3 participants [nested] [flat]
* [PATCH v4 1/1] Adjust assorted hint messages that list all valid options. @ 2022-09-02 21:03 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Nathan Bossart @ 2022-09-02 21:03 UTC (permalink / raw) 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, escape, null, encoding -- 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..306d349656 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; @@ -214,27 +215,32 @@ file_fdw_validator(PG_FUNCTION_ARGS) if (!is_valid_option(def->defname, catalog)) { const struct FileFdwOption *opt; - StringInfoData buf; + const char *closest_match; + ClosestMatchState match_state; + bool has_valid_options = false; /* * Unknown option specified, complain about it. Provide a hint - * with list of valid options for the object. + * with a valid option that looks similar, if there is one. */ - initStringInfo(&buf); + initClosestMatch(&match_state, def->defname, 4); for (opt = valid_options; opt->optname; opt++) { if (catalog == opt->optcontext) - appendStringInfo(&buf, "%s%s", (buf.len > 0) ? ", " : "", - opt->optname); + { + has_valid_options = true; + updateClosestMatch(&match_state, opt->optname); + } } + closest_match = getClosestMatch(&match_state); ereport(ERROR, (errcode(ERRCODE_FDW_INVALID_OPTION_NAME), 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/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 7bf35602b0..ddc185b7d0 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -188,7 +188,6 @@ ALTER USER MAPPING FOR public SERVER testserver1 ALTER USER MAPPING FOR public SERVER testserver1 OPTIONS (ADD sslmode 'require'); ERROR: invalid option "sslmode" -HINT: Valid options in this context are: user, password, sslpassword, password_required, sslcert, sslkey -- But we can add valid ones fine ALTER USER MAPPING FOR public SERVER testserver1 OPTIONS (ADD sslpassword 'dummy'); @@ -9627,7 +9626,7 @@ DO $d$ END; $d$; 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 +HINT: Perhaps you meant to reference the option "passfile". 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 diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c index 95dde056eb..b1a14d204b 100644 --- a/contrib/postgres_fdw/option.c +++ b/contrib/postgres_fdw/option.c @@ -90,26 +90,31 @@ postgres_fdw_validator(PG_FUNCTION_ARGS) { /* * Unknown option specified, complain about it. Provide a hint - * with list of valid options for the object. + * with a valid option that looks similar, if there is one. */ PgFdwOption *opt; - StringInfoData buf; + const char *closest_match; + ClosestMatchState match_state; + bool has_valid_options = false; - initStringInfo(&buf); + initClosestMatch(&match_state, def->defname, 4); for (opt = postgres_fdw_options; opt->keyword; opt++) { if (catalog == opt->optcontext) - 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_INVALID_OPTION_NAME), 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/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c index cf222fc3e9..4ef4a765f4 100644 --- a/src/backend/foreign/foreign.c +++ b/src/backend/foreign/foreign.c @@ -27,6 +27,7 @@ #include "utils/memutils.h" #include "utils/rel.h" #include "utils/syscache.h" +#include "utils/varlena.h" /* @@ -621,25 +622,32 @@ postgresql_fdw_validator(PG_FUNCTION_ARGS) if (!is_conninfo_option(def->defname, catalog)) { const struct ConnectionOption *opt; - StringInfoData buf; + const char *closest_match; + ClosestMatchState match_state; + bool has_valid_options = false; /* * Unknown option specified, complain about it. Provide a hint - * with list of valid options for the object. + * with a valid option that looks similar, if there is one. */ - initStringInfo(&buf); + initClosestMatch(&match_state, def->defname, 4); for (opt = libpq_conninfo_options; opt->optname; opt++) + { if (catalog == opt->optcontext) - appendStringInfo(&buf, "%s%s", (buf.len > 0) ? ", " : "", - opt->optname); + { + has_valid_options = true; + updateClosestMatch(&match_state, opt->optname); + } + } + closest_match = getClosestMatch(&match_state); ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), 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."))); PG_RETURN_BOOL(false); } diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c index 816c66b7e7..1f6e090821 100644 --- a/src/backend/utils/adt/varlena.c +++ b/src/backend/utils/adt/varlena.c @@ -6197,6 +6197,88 @@ rest_of_char_same(const char *s1, const char *s2, int len) #include "levenshtein.c" +/* + * The following *ClosestMatch() functions can be used to determine whether a + * user-provided string resembles any known valid values, which is useful for + * providing hints in log messages, among other things. Use these functions + * like so: + * + * initClosestMatch(&state, source_string, max_distance); + * + * for (int i = 0; i < num_valid_strings; i++) + * updateClosestMatch(&state, valid_strings[i]); + * + * closestMatch = getClosestMatch(&state); + */ + +/* + * Initialize the given state with the source string and maximum Levenshtein + * distance to consider. + */ +void +initClosestMatch(ClosestMatchState *state, const char *source, int max_d) +{ + Assert(state); + Assert(max_d >= 0); + + state->source = source; + state->min_d = -1; + state->max_d = max_d; + state->match = NULL; +} + +/* + * If the candidate string is a closer match than the current one saved (or + * there is no match saved), save it as the closest match. + * + * If the source or candidate string is NULL, empty, or too long, this function + * takes no action. Likewise, if the Levenshtein distance exceeds the maximum + * allowed or more than half the characters are different, no action is taken. + */ +void +updateClosestMatch(ClosestMatchState *state, const char *candidate) +{ + int dist; + + Assert(state); + + if (state->source == NULL || state->source[0] == '\0' || + candidate == NULL || candidate[0] == '\0') + return; + + /* + * To avoid ERROR-ing, we check the lengths here instead of setting + * 'trusted' to false in the call to varstr_levenshtein_less_equal(). + */ + if (strlen(state->source) > MAX_LEVENSHTEIN_STRLEN || + strlen(candidate) > MAX_LEVENSHTEIN_STRLEN) + return; + + dist = varstr_levenshtein_less_equal(state->source, strlen(state->source), + candidate, strlen(candidate), 1, 1, 1, + state->max_d, true); + if (dist <= state->max_d && + dist <= strlen(state->source) / 2 && + (state->min_d == -1 || dist < state->min_d)) + { + state->min_d = dist; + state->match = candidate; + } +} + +/* + * Return the closest match. If no suitable candidates were provided via + * updateClosestMatch(), return NULL. + */ +const char * +getClosestMatch(ClosestMatchState *state) +{ + Assert(state); + + return state->match; +} + + /* * Unicode support */ diff --git a/src/include/utils/varlena.h b/src/include/utils/varlena.h index c45208a204..2bc3b6e519 100644 --- a/src/include/utils/varlena.h +++ b/src/include/utils/varlena.h @@ -38,4 +38,16 @@ extern text *replace_text_regexp(text *src_text, text *pattern_text, int cflags, Oid collation, int search_start, int n); +typedef struct ClosestMatchState +{ + const char *source; + int min_d; + int max_d; + const char *match; +} ClosestMatchState; + +extern void initClosestMatch(ClosestMatchState *state, const char *source, int max_d); +extern void updateClosestMatch(ClosestMatchState *state, const char *candidate); +extern const char *getClosestMatch(ClosestMatchState *state); + #endif diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out index 33505352cc..1b4c19a49b 100644 --- a/src/test/regress/expected/foreign_data.out +++ b/src/test/regress/expected/foreign_data.out @@ -329,7 +329,6 @@ CREATE SERVER s6 VERSION '16.0' FOREIGN DATA WRAPPER foo OPTIONS (host 'a', dbna CREATE SERVER s7 TYPE 'oracle' VERSION '17.0' FOREIGN DATA WRAPPER foo OPTIONS (host 'a', dbname 'b'); CREATE SERVER s8 FOREIGN DATA WRAPPER postgresql OPTIONS (foo '1'); -- ERROR ERROR: invalid option "foo" -HINT: Valid options in this context are: authtype, service, connect_timeout, dbname, host, hostaddr, port, tty, options, requiressl, sslmode, gsslib CREATE SERVER s8 FOREIGN DATA WRAPPER postgresql OPTIONS (host 'localhost', dbname 's8db'); \des+ List of foreign servers @@ -440,7 +439,6 @@ ERROR: permission denied for foreign-data wrapper foo RESET ROLE; ALTER SERVER s8 OPTIONS (foo '1'); -- ERROR option validation ERROR: invalid option "foo" -HINT: Valid options in this context are: authtype, service, connect_timeout, dbname, host, hostaddr, port, tty, options, requiressl, sslmode, gsslib ALTER SERVER s8 OPTIONS (connect_timeout '30', SET dbname 'db1', DROP host); SET ROLE regress_test_role; ALTER SERVER s1 OWNER TO regress_test_indirect; -- ERROR @@ -597,7 +595,7 @@ ERROR: user mapping for "regress_foreign_data_user" already exists for server " CREATE USER MAPPING FOR public SERVER s4 OPTIONS ("this mapping" 'is public'); CREATE USER MAPPING FOR user SERVER s8 OPTIONS (username 'test', password 'secret'); -- ERROR ERROR: invalid option "username" -HINT: Valid options in this context are: user, password +HINT: Perhaps you meant to reference the option "user". CREATE USER MAPPING FOR user SERVER s8 OPTIONS (user 'test', password 'secret'); ALTER SERVER s5 OWNER TO regress_test_role; ALTER SERVER s6 OWNER TO regress_test_indirect; @@ -636,7 +634,7 @@ ALTER USER MAPPING FOR public SERVER s5 OPTIONS (gotcha 'true'); -- E ERROR: user mapping for "public" does not exist for server "s5" ALTER USER MAPPING FOR current_user SERVER s8 OPTIONS (username 'test'); -- ERROR ERROR: invalid option "username" -HINT: Valid options in this context are: user, password +HINT: Perhaps you meant to reference the option "user". ALTER USER MAPPING FOR current_user SERVER s8 OPTIONS (DROP user, SET password 'public'); SET ROLE regress_test_role; ALTER USER MAPPING FOR current_user SERVER s5 OPTIONS (ADD modified '1'); -- 2.25.1 --Nq2Wo0NMKNjxTN9z-- ^ permalink raw reply [nested|flat] 8+ messages in thread
* [PATCH v2 1/1] Adjust assorted hint messages that list all valid options. @ 2022-09-02 21:03 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Nathan Bossart @ 2022-09-02 21:03 UTC (permalink / raw) 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 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, escape, null, encoding -- 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..e3b69ec125 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; @@ -214,27 +215,31 @@ file_fdw_validator(PG_FUNCTION_ARGS) if (!is_valid_option(def->defname, catalog)) { const struct FileFdwOption *opt; - StringInfoData buf; + const char *closest_match; + ClosestMatchState match_state; + bool has_valid_options = false; /* * Unknown option specified, complain about it. Provide a hint - * with list of valid options for the object. + * with a valid option that looks similar, if there is one. */ - initStringInfo(&buf); + initClosestMatch(&match_state, def->defname, 5); for (opt = valid_options; opt->optname; opt++) { if (catalog == opt->optcontext) - appendStringInfo(&buf, "%s%s", (buf.len > 0) ? ", " : "", - opt->optname); + { + has_valid_options = true; + updateClosestMatch(&match_state, opt->optname); + } } + closest_match = getClosestMatch(&match_state); ereport(ERROR, (errcode(ERRCODE_FDW_INVALID_OPTION_NAME), 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/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 7bf35602b0..036eaa63df 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -188,7 +188,7 @@ ALTER USER MAPPING FOR public SERVER testserver1 ALTER USER MAPPING FOR public SERVER testserver1 OPTIONS (ADD sslmode 'require'); ERROR: invalid option "sslmode" -HINT: Valid options in this context are: user, password, sslpassword, password_required, sslcert, sslkey +HINT: Did you mean "sslcert"? -- But we can add valid ones fine ALTER USER MAPPING FOR public SERVER testserver1 OPTIONS (ADD sslpassword 'dummy'); @@ -9627,7 +9627,7 @@ DO $d$ END; $d$; 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 +HINT: Did you mean "passfile"? 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 diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c index 95dde056eb..5ac0484c3e 100644 --- a/contrib/postgres_fdw/option.c +++ b/contrib/postgres_fdw/option.c @@ -90,26 +90,30 @@ postgres_fdw_validator(PG_FUNCTION_ARGS) { /* * Unknown option specified, complain about it. Provide a hint - * with list of valid options for the object. + * with a valid option that looks similar, if there is one. */ PgFdwOption *opt; - StringInfoData buf; + const char *closest_match; + ClosestMatchState match_state; + bool has_valid_options = false; - initStringInfo(&buf); + initClosestMatch(&match_state, def->defname, 5); for (opt = postgres_fdw_options; opt->keyword; opt++) { if (catalog == opt->optcontext) - 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_INVALID_OPTION_NAME), 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/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c index cf222fc3e9..0e2004ec8c 100644 --- a/src/backend/foreign/foreign.c +++ b/src/backend/foreign/foreign.c @@ -27,6 +27,7 @@ #include "utils/memutils.h" #include "utils/rel.h" #include "utils/syscache.h" +#include "utils/varlena.h" /* @@ -621,25 +622,31 @@ postgresql_fdw_validator(PG_FUNCTION_ARGS) if (!is_conninfo_option(def->defname, catalog)) { const struct ConnectionOption *opt; - StringInfoData buf; + const char *closest_match; + ClosestMatchState match_state; + bool has_valid_options = false; /* * Unknown option specified, complain about it. Provide a hint - * with list of valid options for the object. + * with a valid option that looks similar, if there is one. */ - initStringInfo(&buf); + initClosestMatch(&match_state, def->defname, 5); for (opt = libpq_conninfo_options; opt->optname; opt++) + { if (catalog == opt->optcontext) - appendStringInfo(&buf, "%s%s", (buf.len > 0) ? ", " : "", - opt->optname); + { + has_valid_options = true; + updateClosestMatch(&match_state, opt->optname); + } + } + closest_match = getClosestMatch(&match_state); ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), 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."))); PG_RETURN_BOOL(false); } diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c index 8539cef024..55905c5f5d 100644 --- a/src/backend/utils/adt/varlena.c +++ b/src/backend/utils/adt/varlena.c @@ -6196,6 +6196,88 @@ rest_of_char_same(const char *s1, const char *s2, int len) #include "levenshtein.c" +/* + * The following *ClosestMatch() functions can be used to determine whether a + * user-provided string resembles any known valid values, which is useful for + * providing hints in log messages, among other things. Use these functions + * like so: + * + * initClosestMatch(&state, source_string, max_distance); + * + * for (int i = 0; i < num_valid_strings; i++) + * updateClosestMatch(&state, valid_strings[i]); + * + * closestMatch = getClosestMatch(&state); + */ + +/* + * Initialize the given state with the source string and maximum Levenshtein + * distance to consider. + */ +void +initClosestMatch(ClosestMatchState *state, const char *source, int max_d) +{ + Assert(state); + Assert(max_d >= 0); + + state->source = source; + state->min_d = -1; + state->max_d = max_d; + state->match = NULL; +} + +/* + * If the candidate string is a closer match than the current one saved (or + * there is no match saved), save it as the closest match. + * + * If the source or candidate string is NULL, empty, or too long, this function + * takes no action. Likewise, if the Levenshtein distance between the source + * string and the candidate string exceeds the maximum distance allowed by the + * state, no action is taken. + */ +void +updateClosestMatch(ClosestMatchState *state, const char *candidate) +{ + int dist; + + Assert(state); + + if (state->source == NULL || state->source[0] == '\0' || + candidate == NULL || candidate[0] == '\0') + return; + + /* + * To avoid ERROR-ing, we check the lengths here instead of setting + * 'trusted' to false in the call to varstr_levenshtein_less_equal(). + */ + if (strlen(state->source) > MAX_LEVENSHTEIN_STRLEN || + strlen(candidate) > MAX_LEVENSHTEIN_STRLEN) + return; + + dist = varstr_levenshtein_less_equal(state->source, strlen(state->source), + candidate, strlen(candidate), 1, 1, 1, + state->max_d, true); + if (dist <= state->max_d && + (state->min_d == -1 || dist < state->min_d)) + { + state->min_d = dist; + state->match = candidate; + } +} + +/* + * Return the closest match. If no suitable candidates were provided via + * updateClosestMatch(), return NULL. + */ +const char * +getClosestMatch(ClosestMatchState *state) +{ + Assert(state); + + return state->match; +} + + /* * Unicode support */ diff --git a/src/include/utils/varlena.h b/src/include/utils/varlena.h index c45208a204..2bc3b6e519 100644 --- a/src/include/utils/varlena.h +++ b/src/include/utils/varlena.h @@ -38,4 +38,16 @@ extern text *replace_text_regexp(text *src_text, text *pattern_text, int cflags, Oid collation, int search_start, int n); +typedef struct ClosestMatchState +{ + const char *source; + int min_d; + int max_d; + const char *match; +} ClosestMatchState; + +extern void initClosestMatch(ClosestMatchState *state, const char *source, int max_d); +extern void updateClosestMatch(ClosestMatchState *state, const char *candidate); +extern const char *getClosestMatch(ClosestMatchState *state); + #endif diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out index 33505352cc..588ce62266 100644 --- a/src/test/regress/expected/foreign_data.out +++ b/src/test/regress/expected/foreign_data.out @@ -329,7 +329,7 @@ CREATE SERVER s6 VERSION '16.0' FOREIGN DATA WRAPPER foo OPTIONS (host 'a', dbna CREATE SERVER s7 TYPE 'oracle' VERSION '17.0' FOREIGN DATA WRAPPER foo OPTIONS (host 'a', dbname 'b'); CREATE SERVER s8 FOREIGN DATA WRAPPER postgresql OPTIONS (foo '1'); -- ERROR ERROR: invalid option "foo" -HINT: Valid options in this context are: authtype, service, connect_timeout, dbname, host, hostaddr, port, tty, options, requiressl, sslmode, gsslib +HINT: Did you mean "host"? CREATE SERVER s8 FOREIGN DATA WRAPPER postgresql OPTIONS (host 'localhost', dbname 's8db'); \des+ List of foreign servers @@ -440,7 +440,7 @@ ERROR: permission denied for foreign-data wrapper foo RESET ROLE; ALTER SERVER s8 OPTIONS (foo '1'); -- ERROR option validation ERROR: invalid option "foo" -HINT: Valid options in this context are: authtype, service, connect_timeout, dbname, host, hostaddr, port, tty, options, requiressl, sslmode, gsslib +HINT: Did you mean "host"? ALTER SERVER s8 OPTIONS (connect_timeout '30', SET dbname 'db1', DROP host); SET ROLE regress_test_role; ALTER SERVER s1 OWNER TO regress_test_indirect; -- ERROR @@ -597,7 +597,7 @@ ERROR: user mapping for "regress_foreign_data_user" already exists for server " CREATE USER MAPPING FOR public SERVER s4 OPTIONS ("this mapping" 'is public'); CREATE USER MAPPING FOR user SERVER s8 OPTIONS (username 'test', password 'secret'); -- ERROR ERROR: invalid option "username" -HINT: Valid options in this context are: user, password +HINT: Did you mean "user"? CREATE USER MAPPING FOR user SERVER s8 OPTIONS (user 'test', password 'secret'); ALTER SERVER s5 OWNER TO regress_test_role; ALTER SERVER s6 OWNER TO regress_test_indirect; @@ -636,7 +636,7 @@ ALTER USER MAPPING FOR public SERVER s5 OPTIONS (gotcha 'true'); -- E ERROR: user mapping for "public" does not exist for server "s5" ALTER USER MAPPING FOR current_user SERVER s8 OPTIONS (username 'test'); -- ERROR ERROR: invalid option "username" -HINT: Valid options in this context are: user, password +HINT: Did you mean "user"? ALTER USER MAPPING FOR current_user SERVER s8 OPTIONS (DROP user, SET password 'public'); SET ROLE regress_test_role; ALTER USER MAPPING FOR current_user SERVER s5 OPTIONS (ADD modified '1'); -- 2.25.1 --VbJkn9YxBvnuCH5J-- ^ permalink raw reply [nested|flat] 8+ messages in thread
* [PATCH v3 1/1] Adjust assorted hint messages that list all valid options. @ 2022-09-02 21:03 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Nathan Bossart @ 2022-09-02 21:03 UTC (permalink / raw) 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 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, escape, null, encoding -- 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..53d996bd7c 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; @@ -214,27 +215,31 @@ file_fdw_validator(PG_FUNCTION_ARGS) if (!is_valid_option(def->defname, catalog)) { const struct FileFdwOption *opt; - StringInfoData buf; + const char *closest_match; + ClosestMatchState match_state; + bool has_valid_options = false; /* * Unknown option specified, complain about it. Provide a hint - * with list of valid options for the object. + * with a valid option that looks similar, if there is one. */ - initStringInfo(&buf); + initClosestMatch(&match_state, def->defname, 4); for (opt = valid_options; opt->optname; opt++) { if (catalog == opt->optcontext) - appendStringInfo(&buf, "%s%s", (buf.len > 0) ? ", " : "", - opt->optname); + { + has_valid_options = true; + updateClosestMatch(&match_state, opt->optname); + } } + closest_match = getClosestMatch(&match_state); ereport(ERROR, (errcode(ERRCODE_FDW_INVALID_OPTION_NAME), 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/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 7bf35602b0..2588ad4aa6 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -188,7 +188,6 @@ ALTER USER MAPPING FOR public SERVER testserver1 ALTER USER MAPPING FOR public SERVER testserver1 OPTIONS (ADD sslmode 'require'); ERROR: invalid option "sslmode" -HINT: Valid options in this context are: user, password, sslpassword, password_required, sslcert, sslkey -- But we can add valid ones fine ALTER USER MAPPING FOR public SERVER testserver1 OPTIONS (ADD sslpassword 'dummy'); @@ -9627,7 +9626,7 @@ DO $d$ END; $d$; 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 +HINT: Did you mean "passfile"? 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 diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c index 95dde056eb..6971c88463 100644 --- a/contrib/postgres_fdw/option.c +++ b/contrib/postgres_fdw/option.c @@ -90,26 +90,30 @@ postgres_fdw_validator(PG_FUNCTION_ARGS) { /* * Unknown option specified, complain about it. Provide a hint - * with list of valid options for the object. + * with a valid option that looks similar, if there is one. */ PgFdwOption *opt; - StringInfoData buf; + const char *closest_match; + ClosestMatchState match_state; + bool has_valid_options = false; - initStringInfo(&buf); + initClosestMatch(&match_state, def->defname, 4); for (opt = postgres_fdw_options; opt->keyword; opt++) { if (catalog == opt->optcontext) - 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_INVALID_OPTION_NAME), 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/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c index cf222fc3e9..283ebb6a58 100644 --- a/src/backend/foreign/foreign.c +++ b/src/backend/foreign/foreign.c @@ -27,6 +27,7 @@ #include "utils/memutils.h" #include "utils/rel.h" #include "utils/syscache.h" +#include "utils/varlena.h" /* @@ -621,25 +622,31 @@ postgresql_fdw_validator(PG_FUNCTION_ARGS) if (!is_conninfo_option(def->defname, catalog)) { const struct ConnectionOption *opt; - StringInfoData buf; + const char *closest_match; + ClosestMatchState match_state; + bool has_valid_options = false; /* * Unknown option specified, complain about it. Provide a hint - * with list of valid options for the object. + * with a valid option that looks similar, if there is one. */ - initStringInfo(&buf); + initClosestMatch(&match_state, def->defname, 4); for (opt = libpq_conninfo_options; opt->optname; opt++) + { if (catalog == opt->optcontext) - appendStringInfo(&buf, "%s%s", (buf.len > 0) ? ", " : "", - opt->optname); + { + has_valid_options = true; + updateClosestMatch(&match_state, opt->optname); + } + } + closest_match = getClosestMatch(&match_state); ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), 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."))); PG_RETURN_BOOL(false); } diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c index 8539cef024..174f9c390d 100644 --- a/src/backend/utils/adt/varlena.c +++ b/src/backend/utils/adt/varlena.c @@ -6196,6 +6196,88 @@ rest_of_char_same(const char *s1, const char *s2, int len) #include "levenshtein.c" +/* + * The following *ClosestMatch() functions can be used to determine whether a + * user-provided string resembles any known valid values, which is useful for + * providing hints in log messages, among other things. Use these functions + * like so: + * + * initClosestMatch(&state, source_string, max_distance); + * + * for (int i = 0; i < num_valid_strings; i++) + * updateClosestMatch(&state, valid_strings[i]); + * + * closestMatch = getClosestMatch(&state); + */ + +/* + * Initialize the given state with the source string and maximum Levenshtein + * distance to consider. + */ +void +initClosestMatch(ClosestMatchState *state, const char *source, int max_d) +{ + Assert(state); + Assert(max_d >= 0); + + state->source = source; + state->min_d = -1; + state->max_d = max_d; + state->match = NULL; +} + +/* + * If the candidate string is a closer match than the current one saved (or + * there is no match saved), save it as the closest match. + * + * If the source or candidate string is NULL, empty, or too long, this function + * takes no action. Likewise, if the Levenshtein distance exceeds the maximum + * allowed or more than half the characters are different, no action is taken. + */ +void +updateClosestMatch(ClosestMatchState *state, const char *candidate) +{ + int dist; + + Assert(state); + + if (state->source == NULL || state->source[0] == '\0' || + candidate == NULL || candidate[0] == '\0') + return; + + /* + * To avoid ERROR-ing, we check the lengths here instead of setting + * 'trusted' to false in the call to varstr_levenshtein_less_equal(). + */ + if (strlen(state->source) > MAX_LEVENSHTEIN_STRLEN || + strlen(candidate) > MAX_LEVENSHTEIN_STRLEN) + return; + + dist = varstr_levenshtein_less_equal(state->source, strlen(state->source), + candidate, strlen(candidate), 1, 1, 1, + state->max_d, true); + if (dist <= state->max_d && + dist <= strlen(state->source) / 2 && + (state->min_d == -1 || dist < state->min_d)) + { + state->min_d = dist; + state->match = candidate; + } +} + +/* + * Return the closest match. If no suitable candidates were provided via + * updateClosestMatch(), return NULL. + */ +const char * +getClosestMatch(ClosestMatchState *state) +{ + Assert(state); + + return state->match; +} + + /* * Unicode support */ diff --git a/src/include/utils/varlena.h b/src/include/utils/varlena.h index c45208a204..2bc3b6e519 100644 --- a/src/include/utils/varlena.h +++ b/src/include/utils/varlena.h @@ -38,4 +38,16 @@ extern text *replace_text_regexp(text *src_text, text *pattern_text, int cflags, Oid collation, int search_start, int n); +typedef struct ClosestMatchState +{ + const char *source; + int min_d; + int max_d; + const char *match; +} ClosestMatchState; + +extern void initClosestMatch(ClosestMatchState *state, const char *source, int max_d); +extern void updateClosestMatch(ClosestMatchState *state, const char *candidate); +extern const char *getClosestMatch(ClosestMatchState *state); + #endif diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out index 33505352cc..97a0b9cda1 100644 --- a/src/test/regress/expected/foreign_data.out +++ b/src/test/regress/expected/foreign_data.out @@ -329,7 +329,6 @@ CREATE SERVER s6 VERSION '16.0' FOREIGN DATA WRAPPER foo OPTIONS (host 'a', dbna CREATE SERVER s7 TYPE 'oracle' VERSION '17.0' FOREIGN DATA WRAPPER foo OPTIONS (host 'a', dbname 'b'); CREATE SERVER s8 FOREIGN DATA WRAPPER postgresql OPTIONS (foo '1'); -- ERROR ERROR: invalid option "foo" -HINT: Valid options in this context are: authtype, service, connect_timeout, dbname, host, hostaddr, port, tty, options, requiressl, sslmode, gsslib CREATE SERVER s8 FOREIGN DATA WRAPPER postgresql OPTIONS (host 'localhost', dbname 's8db'); \des+ List of foreign servers @@ -440,7 +439,6 @@ ERROR: permission denied for foreign-data wrapper foo RESET ROLE; ALTER SERVER s8 OPTIONS (foo '1'); -- ERROR option validation ERROR: invalid option "foo" -HINT: Valid options in this context are: authtype, service, connect_timeout, dbname, host, hostaddr, port, tty, options, requiressl, sslmode, gsslib ALTER SERVER s8 OPTIONS (connect_timeout '30', SET dbname 'db1', DROP host); SET ROLE regress_test_role; ALTER SERVER s1 OWNER TO regress_test_indirect; -- ERROR @@ -597,7 +595,7 @@ ERROR: user mapping for "regress_foreign_data_user" already exists for server " CREATE USER MAPPING FOR public SERVER s4 OPTIONS ("this mapping" 'is public'); CREATE USER MAPPING FOR user SERVER s8 OPTIONS (username 'test', password 'secret'); -- ERROR ERROR: invalid option "username" -HINT: Valid options in this context are: user, password +HINT: Did you mean "user"? CREATE USER MAPPING FOR user SERVER s8 OPTIONS (user 'test', password 'secret'); ALTER SERVER s5 OWNER TO regress_test_role; ALTER SERVER s6 OWNER TO regress_test_indirect; @@ -636,7 +634,7 @@ ALTER USER MAPPING FOR public SERVER s5 OPTIONS (gotcha 'true'); -- E ERROR: user mapping for "public" does not exist for server "s5" ALTER USER MAPPING FOR current_user SERVER s8 OPTIONS (username 'test'); -- ERROR ERROR: invalid option "username" -HINT: Valid options in this context are: user, password +HINT: Did you mean "user"? ALTER USER MAPPING FOR current_user SERVER s8 OPTIONS (DROP user, SET password 'public'); SET ROLE regress_test_role; ALTER USER MAPPING FOR current_user SERVER s5 OPTIONS (ADD modified '1'); -- 2.25.1 --gBBFr7Ir9EOA20Yy-- ^ permalink raw reply [nested|flat] 8+ messages in thread
* [PATCH v2 1/1] Adjust assorted hint messages that list all valid options. @ 2022-09-02 21:03 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Nathan Bossart @ 2022-09-02 21:03 UTC (permalink / raw) 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 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, escape, null, encoding -- 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..e3b69ec125 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; @@ -214,27 +215,31 @@ file_fdw_validator(PG_FUNCTION_ARGS) if (!is_valid_option(def->defname, catalog)) { const struct FileFdwOption *opt; - StringInfoData buf; + const char *closest_match; + ClosestMatchState match_state; + bool has_valid_options = false; /* * Unknown option specified, complain about it. Provide a hint - * with list of valid options for the object. + * with a valid option that looks similar, if there is one. */ - initStringInfo(&buf); + initClosestMatch(&match_state, def->defname, 5); for (opt = valid_options; opt->optname; opt++) { if (catalog == opt->optcontext) - appendStringInfo(&buf, "%s%s", (buf.len > 0) ? ", " : "", - opt->optname); + { + has_valid_options = true; + updateClosestMatch(&match_state, opt->optname); + } } + closest_match = getClosestMatch(&match_state); ereport(ERROR, (errcode(ERRCODE_FDW_INVALID_OPTION_NAME), 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/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 7bf35602b0..036eaa63df 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -188,7 +188,7 @@ ALTER USER MAPPING FOR public SERVER testserver1 ALTER USER MAPPING FOR public SERVER testserver1 OPTIONS (ADD sslmode 'require'); ERROR: invalid option "sslmode" -HINT: Valid options in this context are: user, password, sslpassword, password_required, sslcert, sslkey +HINT: Did you mean "sslcert"? -- But we can add valid ones fine ALTER USER MAPPING FOR public SERVER testserver1 OPTIONS (ADD sslpassword 'dummy'); @@ -9627,7 +9627,7 @@ DO $d$ END; $d$; 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 +HINT: Did you mean "passfile"? 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 diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c index 95dde056eb..5ac0484c3e 100644 --- a/contrib/postgres_fdw/option.c +++ b/contrib/postgres_fdw/option.c @@ -90,26 +90,30 @@ postgres_fdw_validator(PG_FUNCTION_ARGS) { /* * Unknown option specified, complain about it. Provide a hint - * with list of valid options for the object. + * with a valid option that looks similar, if there is one. */ PgFdwOption *opt; - StringInfoData buf; + const char *closest_match; + ClosestMatchState match_state; + bool has_valid_options = false; - initStringInfo(&buf); + initClosestMatch(&match_state, def->defname, 5); for (opt = postgres_fdw_options; opt->keyword; opt++) { if (catalog == opt->optcontext) - 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_INVALID_OPTION_NAME), 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/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c index cf222fc3e9..0e2004ec8c 100644 --- a/src/backend/foreign/foreign.c +++ b/src/backend/foreign/foreign.c @@ -27,6 +27,7 @@ #include "utils/memutils.h" #include "utils/rel.h" #include "utils/syscache.h" +#include "utils/varlena.h" /* @@ -621,25 +622,31 @@ postgresql_fdw_validator(PG_FUNCTION_ARGS) if (!is_conninfo_option(def->defname, catalog)) { const struct ConnectionOption *opt; - StringInfoData buf; + const char *closest_match; + ClosestMatchState match_state; + bool has_valid_options = false; /* * Unknown option specified, complain about it. Provide a hint - * with list of valid options for the object. + * with a valid option that looks similar, if there is one. */ - initStringInfo(&buf); + initClosestMatch(&match_state, def->defname, 5); for (opt = libpq_conninfo_options; opt->optname; opt++) + { if (catalog == opt->optcontext) - appendStringInfo(&buf, "%s%s", (buf.len > 0) ? ", " : "", - opt->optname); + { + has_valid_options = true; + updateClosestMatch(&match_state, opt->optname); + } + } + closest_match = getClosestMatch(&match_state); ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), 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."))); PG_RETURN_BOOL(false); } diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c index 8539cef024..55905c5f5d 100644 --- a/src/backend/utils/adt/varlena.c +++ b/src/backend/utils/adt/varlena.c @@ -6196,6 +6196,88 @@ rest_of_char_same(const char *s1, const char *s2, int len) #include "levenshtein.c" +/* + * The following *ClosestMatch() functions can be used to determine whether a + * user-provided string resembles any known valid values, which is useful for + * providing hints in log messages, among other things. Use these functions + * like so: + * + * initClosestMatch(&state, source_string, max_distance); + * + * for (int i = 0; i < num_valid_strings; i++) + * updateClosestMatch(&state, valid_strings[i]); + * + * closestMatch = getClosestMatch(&state); + */ + +/* + * Initialize the given state with the source string and maximum Levenshtein + * distance to consider. + */ +void +initClosestMatch(ClosestMatchState *state, const char *source, int max_d) +{ + Assert(state); + Assert(max_d >= 0); + + state->source = source; + state->min_d = -1; + state->max_d = max_d; + state->match = NULL; +} + +/* + * If the candidate string is a closer match than the current one saved (or + * there is no match saved), save it as the closest match. + * + * If the source or candidate string is NULL, empty, or too long, this function + * takes no action. Likewise, if the Levenshtein distance between the source + * string and the candidate string exceeds the maximum distance allowed by the + * state, no action is taken. + */ +void +updateClosestMatch(ClosestMatchState *state, const char *candidate) +{ + int dist; + + Assert(state); + + if (state->source == NULL || state->source[0] == '\0' || + candidate == NULL || candidate[0] == '\0') + return; + + /* + * To avoid ERROR-ing, we check the lengths here instead of setting + * 'trusted' to false in the call to varstr_levenshtein_less_equal(). + */ + if (strlen(state->source) > MAX_LEVENSHTEIN_STRLEN || + strlen(candidate) > MAX_LEVENSHTEIN_STRLEN) + return; + + dist = varstr_levenshtein_less_equal(state->source, strlen(state->source), + candidate, strlen(candidate), 1, 1, 1, + state->max_d, true); + if (dist <= state->max_d && + (state->min_d == -1 || dist < state->min_d)) + { + state->min_d = dist; + state->match = candidate; + } +} + +/* + * Return the closest match. If no suitable candidates were provided via + * updateClosestMatch(), return NULL. + */ +const char * +getClosestMatch(ClosestMatchState *state) +{ + Assert(state); + + return state->match; +} + + /* * Unicode support */ diff --git a/src/include/utils/varlena.h b/src/include/utils/varlena.h index c45208a204..2bc3b6e519 100644 --- a/src/include/utils/varlena.h +++ b/src/include/utils/varlena.h @@ -38,4 +38,16 @@ extern text *replace_text_regexp(text *src_text, text *pattern_text, int cflags, Oid collation, int search_start, int n); +typedef struct ClosestMatchState +{ + const char *source; + int min_d; + int max_d; + const char *match; +} ClosestMatchState; + +extern void initClosestMatch(ClosestMatchState *state, const char *source, int max_d); +extern void updateClosestMatch(ClosestMatchState *state, const char *candidate); +extern const char *getClosestMatch(ClosestMatchState *state); + #endif diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out index 33505352cc..588ce62266 100644 --- a/src/test/regress/expected/foreign_data.out +++ b/src/test/regress/expected/foreign_data.out @@ -329,7 +329,7 @@ CREATE SERVER s6 VERSION '16.0' FOREIGN DATA WRAPPER foo OPTIONS (host 'a', dbna CREATE SERVER s7 TYPE 'oracle' VERSION '17.0' FOREIGN DATA WRAPPER foo OPTIONS (host 'a', dbname 'b'); CREATE SERVER s8 FOREIGN DATA WRAPPER postgresql OPTIONS (foo '1'); -- ERROR ERROR: invalid option "foo" -HINT: Valid options in this context are: authtype, service, connect_timeout, dbname, host, hostaddr, port, tty, options, requiressl, sslmode, gsslib +HINT: Did you mean "host"? CREATE SERVER s8 FOREIGN DATA WRAPPER postgresql OPTIONS (host 'localhost', dbname 's8db'); \des+ List of foreign servers @@ -440,7 +440,7 @@ ERROR: permission denied for foreign-data wrapper foo RESET ROLE; ALTER SERVER s8 OPTIONS (foo '1'); -- ERROR option validation ERROR: invalid option "foo" -HINT: Valid options in this context are: authtype, service, connect_timeout, dbname, host, hostaddr, port, tty, options, requiressl, sslmode, gsslib +HINT: Did you mean "host"? ALTER SERVER s8 OPTIONS (connect_timeout '30', SET dbname 'db1', DROP host); SET ROLE regress_test_role; ALTER SERVER s1 OWNER TO regress_test_indirect; -- ERROR @@ -597,7 +597,7 @@ ERROR: user mapping for "regress_foreign_data_user" already exists for server " CREATE USER MAPPING FOR public SERVER s4 OPTIONS ("this mapping" 'is public'); CREATE USER MAPPING FOR user SERVER s8 OPTIONS (username 'test', password 'secret'); -- ERROR ERROR: invalid option "username" -HINT: Valid options in this context are: user, password +HINT: Did you mean "user"? CREATE USER MAPPING FOR user SERVER s8 OPTIONS (user 'test', password 'secret'); ALTER SERVER s5 OWNER TO regress_test_role; ALTER SERVER s6 OWNER TO regress_test_indirect; @@ -636,7 +636,7 @@ ALTER USER MAPPING FOR public SERVER s5 OPTIONS (gotcha 'true'); -- E ERROR: user mapping for "public" does not exist for server "s5" ALTER USER MAPPING FOR current_user SERVER s8 OPTIONS (username 'test'); -- ERROR ERROR: invalid option "username" -HINT: Valid options in this context are: user, password +HINT: Did you mean "user"? ALTER USER MAPPING FOR current_user SERVER s8 OPTIONS (DROP user, SET password 'public'); SET ROLE regress_test_role; ALTER USER MAPPING FOR current_user SERVER s5 OPTIONS (ADD modified '1'); -- 2.25.1 --VbJkn9YxBvnuCH5J-- ^ permalink raw reply [nested|flat] 8+ messages in thread
* [PATCH v3 1/1] Adjust assorted hint messages that list all valid options. @ 2022-09-02 21:03 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Nathan Bossart @ 2022-09-02 21:03 UTC (permalink / raw) 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 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, escape, null, encoding -- 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..53d996bd7c 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; @@ -214,27 +215,31 @@ file_fdw_validator(PG_FUNCTION_ARGS) if (!is_valid_option(def->defname, catalog)) { const struct FileFdwOption *opt; - StringInfoData buf; + const char *closest_match; + ClosestMatchState match_state; + bool has_valid_options = false; /* * Unknown option specified, complain about it. Provide a hint - * with list of valid options for the object. + * with a valid option that looks similar, if there is one. */ - initStringInfo(&buf); + initClosestMatch(&match_state, def->defname, 4); for (opt = valid_options; opt->optname; opt++) { if (catalog == opt->optcontext) - appendStringInfo(&buf, "%s%s", (buf.len > 0) ? ", " : "", - opt->optname); + { + has_valid_options = true; + updateClosestMatch(&match_state, opt->optname); + } } + closest_match = getClosestMatch(&match_state); ereport(ERROR, (errcode(ERRCODE_FDW_INVALID_OPTION_NAME), 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/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 7bf35602b0..2588ad4aa6 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -188,7 +188,6 @@ ALTER USER MAPPING FOR public SERVER testserver1 ALTER USER MAPPING FOR public SERVER testserver1 OPTIONS (ADD sslmode 'require'); ERROR: invalid option "sslmode" -HINT: Valid options in this context are: user, password, sslpassword, password_required, sslcert, sslkey -- But we can add valid ones fine ALTER USER MAPPING FOR public SERVER testserver1 OPTIONS (ADD sslpassword 'dummy'); @@ -9627,7 +9626,7 @@ DO $d$ END; $d$; 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 +HINT: Did you mean "passfile"? 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 diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c index 95dde056eb..6971c88463 100644 --- a/contrib/postgres_fdw/option.c +++ b/contrib/postgres_fdw/option.c @@ -90,26 +90,30 @@ postgres_fdw_validator(PG_FUNCTION_ARGS) { /* * Unknown option specified, complain about it. Provide a hint - * with list of valid options for the object. + * with a valid option that looks similar, if there is one. */ PgFdwOption *opt; - StringInfoData buf; + const char *closest_match; + ClosestMatchState match_state; + bool has_valid_options = false; - initStringInfo(&buf); + initClosestMatch(&match_state, def->defname, 4); for (opt = postgres_fdw_options; opt->keyword; opt++) { if (catalog == opt->optcontext) - 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_INVALID_OPTION_NAME), 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/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c index cf222fc3e9..283ebb6a58 100644 --- a/src/backend/foreign/foreign.c +++ b/src/backend/foreign/foreign.c @@ -27,6 +27,7 @@ #include "utils/memutils.h" #include "utils/rel.h" #include "utils/syscache.h" +#include "utils/varlena.h" /* @@ -621,25 +622,31 @@ postgresql_fdw_validator(PG_FUNCTION_ARGS) if (!is_conninfo_option(def->defname, catalog)) { const struct ConnectionOption *opt; - StringInfoData buf; + const char *closest_match; + ClosestMatchState match_state; + bool has_valid_options = false; /* * Unknown option specified, complain about it. Provide a hint - * with list of valid options for the object. + * with a valid option that looks similar, if there is one. */ - initStringInfo(&buf); + initClosestMatch(&match_state, def->defname, 4); for (opt = libpq_conninfo_options; opt->optname; opt++) + { if (catalog == opt->optcontext) - appendStringInfo(&buf, "%s%s", (buf.len > 0) ? ", " : "", - opt->optname); + { + has_valid_options = true; + updateClosestMatch(&match_state, opt->optname); + } + } + closest_match = getClosestMatch(&match_state); ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), 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."))); PG_RETURN_BOOL(false); } diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c index 8539cef024..174f9c390d 100644 --- a/src/backend/utils/adt/varlena.c +++ b/src/backend/utils/adt/varlena.c @@ -6196,6 +6196,88 @@ rest_of_char_same(const char *s1, const char *s2, int len) #include "levenshtein.c" +/* + * The following *ClosestMatch() functions can be used to determine whether a + * user-provided string resembles any known valid values, which is useful for + * providing hints in log messages, among other things. Use these functions + * like so: + * + * initClosestMatch(&state, source_string, max_distance); + * + * for (int i = 0; i < num_valid_strings; i++) + * updateClosestMatch(&state, valid_strings[i]); + * + * closestMatch = getClosestMatch(&state); + */ + +/* + * Initialize the given state with the source string and maximum Levenshtein + * distance to consider. + */ +void +initClosestMatch(ClosestMatchState *state, const char *source, int max_d) +{ + Assert(state); + Assert(max_d >= 0); + + state->source = source; + state->min_d = -1; + state->max_d = max_d; + state->match = NULL; +} + +/* + * If the candidate string is a closer match than the current one saved (or + * there is no match saved), save it as the closest match. + * + * If the source or candidate string is NULL, empty, or too long, this function + * takes no action. Likewise, if the Levenshtein distance exceeds the maximum + * allowed or more than half the characters are different, no action is taken. + */ +void +updateClosestMatch(ClosestMatchState *state, const char *candidate) +{ + int dist; + + Assert(state); + + if (state->source == NULL || state->source[0] == '\0' || + candidate == NULL || candidate[0] == '\0') + return; + + /* + * To avoid ERROR-ing, we check the lengths here instead of setting + * 'trusted' to false in the call to varstr_levenshtein_less_equal(). + */ + if (strlen(state->source) > MAX_LEVENSHTEIN_STRLEN || + strlen(candidate) > MAX_LEVENSHTEIN_STRLEN) + return; + + dist = varstr_levenshtein_less_equal(state->source, strlen(state->source), + candidate, strlen(candidate), 1, 1, 1, + state->max_d, true); + if (dist <= state->max_d && + dist <= strlen(state->source) / 2 && + (state->min_d == -1 || dist < state->min_d)) + { + state->min_d = dist; + state->match = candidate; + } +} + +/* + * Return the closest match. If no suitable candidates were provided via + * updateClosestMatch(), return NULL. + */ +const char * +getClosestMatch(ClosestMatchState *state) +{ + Assert(state); + + return state->match; +} + + /* * Unicode support */ diff --git a/src/include/utils/varlena.h b/src/include/utils/varlena.h index c45208a204..2bc3b6e519 100644 --- a/src/include/utils/varlena.h +++ b/src/include/utils/varlena.h @@ -38,4 +38,16 @@ extern text *replace_text_regexp(text *src_text, text *pattern_text, int cflags, Oid collation, int search_start, int n); +typedef struct ClosestMatchState +{ + const char *source; + int min_d; + int max_d; + const char *match; +} ClosestMatchState; + +extern void initClosestMatch(ClosestMatchState *state, const char *source, int max_d); +extern void updateClosestMatch(ClosestMatchState *state, const char *candidate); +extern const char *getClosestMatch(ClosestMatchState *state); + #endif diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out index 33505352cc..97a0b9cda1 100644 --- a/src/test/regress/expected/foreign_data.out +++ b/src/test/regress/expected/foreign_data.out @@ -329,7 +329,6 @@ CREATE SERVER s6 VERSION '16.0' FOREIGN DATA WRAPPER foo OPTIONS (host 'a', dbna CREATE SERVER s7 TYPE 'oracle' VERSION '17.0' FOREIGN DATA WRAPPER foo OPTIONS (host 'a', dbname 'b'); CREATE SERVER s8 FOREIGN DATA WRAPPER postgresql OPTIONS (foo '1'); -- ERROR ERROR: invalid option "foo" -HINT: Valid options in this context are: authtype, service, connect_timeout, dbname, host, hostaddr, port, tty, options, requiressl, sslmode, gsslib CREATE SERVER s8 FOREIGN DATA WRAPPER postgresql OPTIONS (host 'localhost', dbname 's8db'); \des+ List of foreign servers @@ -440,7 +439,6 @@ ERROR: permission denied for foreign-data wrapper foo RESET ROLE; ALTER SERVER s8 OPTIONS (foo '1'); -- ERROR option validation ERROR: invalid option "foo" -HINT: Valid options in this context are: authtype, service, connect_timeout, dbname, host, hostaddr, port, tty, options, requiressl, sslmode, gsslib ALTER SERVER s8 OPTIONS (connect_timeout '30', SET dbname 'db1', DROP host); SET ROLE regress_test_role; ALTER SERVER s1 OWNER TO regress_test_indirect; -- ERROR @@ -597,7 +595,7 @@ ERROR: user mapping for "regress_foreign_data_user" already exists for server " CREATE USER MAPPING FOR public SERVER s4 OPTIONS ("this mapping" 'is public'); CREATE USER MAPPING FOR user SERVER s8 OPTIONS (username 'test', password 'secret'); -- ERROR ERROR: invalid option "username" -HINT: Valid options in this context are: user, password +HINT: Did you mean "user"? CREATE USER MAPPING FOR user SERVER s8 OPTIONS (user 'test', password 'secret'); ALTER SERVER s5 OWNER TO regress_test_role; ALTER SERVER s6 OWNER TO regress_test_indirect; @@ -636,7 +634,7 @@ ALTER USER MAPPING FOR public SERVER s5 OPTIONS (gotcha 'true'); -- E ERROR: user mapping for "public" does not exist for server "s5" ALTER USER MAPPING FOR current_user SERVER s8 OPTIONS (username 'test'); -- ERROR ERROR: invalid option "username" -HINT: Valid options in this context are: user, password +HINT: Did you mean "user"? ALTER USER MAPPING FOR current_user SERVER s8 OPTIONS (DROP user, SET password 'public'); SET ROLE regress_test_role; ALTER USER MAPPING FOR current_user SERVER s5 OPTIONS (ADD modified '1'); -- 2.25.1 --gBBFr7Ir9EOA20Yy-- ^ permalink raw reply [nested|flat] 8+ messages in thread
* [PATCH v4 1/1] Adjust assorted hint messages that list all valid options. @ 2022-09-02 21:03 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Nathan Bossart @ 2022-09-02 21:03 UTC (permalink / raw) 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, escape, null, encoding -- 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..306d349656 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; @@ -214,27 +215,32 @@ file_fdw_validator(PG_FUNCTION_ARGS) if (!is_valid_option(def->defname, catalog)) { const struct FileFdwOption *opt; - StringInfoData buf; + const char *closest_match; + ClosestMatchState match_state; + bool has_valid_options = false; /* * Unknown option specified, complain about it. Provide a hint - * with list of valid options for the object. + * with a valid option that looks similar, if there is one. */ - initStringInfo(&buf); + initClosestMatch(&match_state, def->defname, 4); for (opt = valid_options; opt->optname; opt++) { if (catalog == opt->optcontext) - appendStringInfo(&buf, "%s%s", (buf.len > 0) ? ", " : "", - opt->optname); + { + has_valid_options = true; + updateClosestMatch(&match_state, opt->optname); + } } + closest_match = getClosestMatch(&match_state); ereport(ERROR, (errcode(ERRCODE_FDW_INVALID_OPTION_NAME), 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/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 7bf35602b0..ddc185b7d0 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -188,7 +188,6 @@ ALTER USER MAPPING FOR public SERVER testserver1 ALTER USER MAPPING FOR public SERVER testserver1 OPTIONS (ADD sslmode 'require'); ERROR: invalid option "sslmode" -HINT: Valid options in this context are: user, password, sslpassword, password_required, sslcert, sslkey -- But we can add valid ones fine ALTER USER MAPPING FOR public SERVER testserver1 OPTIONS (ADD sslpassword 'dummy'); @@ -9627,7 +9626,7 @@ DO $d$ END; $d$; 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 +HINT: Perhaps you meant to reference the option "passfile". 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 diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c index 95dde056eb..b1a14d204b 100644 --- a/contrib/postgres_fdw/option.c +++ b/contrib/postgres_fdw/option.c @@ -90,26 +90,31 @@ postgres_fdw_validator(PG_FUNCTION_ARGS) { /* * Unknown option specified, complain about it. Provide a hint - * with list of valid options for the object. + * with a valid option that looks similar, if there is one. */ PgFdwOption *opt; - StringInfoData buf; + const char *closest_match; + ClosestMatchState match_state; + bool has_valid_options = false; - initStringInfo(&buf); + initClosestMatch(&match_state, def->defname, 4); for (opt = postgres_fdw_options; opt->keyword; opt++) { if (catalog == opt->optcontext) - 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_INVALID_OPTION_NAME), 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/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c index cf222fc3e9..4ef4a765f4 100644 --- a/src/backend/foreign/foreign.c +++ b/src/backend/foreign/foreign.c @@ -27,6 +27,7 @@ #include "utils/memutils.h" #include "utils/rel.h" #include "utils/syscache.h" +#include "utils/varlena.h" /* @@ -621,25 +622,32 @@ postgresql_fdw_validator(PG_FUNCTION_ARGS) if (!is_conninfo_option(def->defname, catalog)) { const struct ConnectionOption *opt; - StringInfoData buf; + const char *closest_match; + ClosestMatchState match_state; + bool has_valid_options = false; /* * Unknown option specified, complain about it. Provide a hint - * with list of valid options for the object. + * with a valid option that looks similar, if there is one. */ - initStringInfo(&buf); + initClosestMatch(&match_state, def->defname, 4); for (opt = libpq_conninfo_options; opt->optname; opt++) + { if (catalog == opt->optcontext) - appendStringInfo(&buf, "%s%s", (buf.len > 0) ? ", " : "", - opt->optname); + { + has_valid_options = true; + updateClosestMatch(&match_state, opt->optname); + } + } + closest_match = getClosestMatch(&match_state); ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), 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."))); PG_RETURN_BOOL(false); } diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c index 816c66b7e7..1f6e090821 100644 --- a/src/backend/utils/adt/varlena.c +++ b/src/backend/utils/adt/varlena.c @@ -6197,6 +6197,88 @@ rest_of_char_same(const char *s1, const char *s2, int len) #include "levenshtein.c" +/* + * The following *ClosestMatch() functions can be used to determine whether a + * user-provided string resembles any known valid values, which is useful for + * providing hints in log messages, among other things. Use these functions + * like so: + * + * initClosestMatch(&state, source_string, max_distance); + * + * for (int i = 0; i < num_valid_strings; i++) + * updateClosestMatch(&state, valid_strings[i]); + * + * closestMatch = getClosestMatch(&state); + */ + +/* + * Initialize the given state with the source string and maximum Levenshtein + * distance to consider. + */ +void +initClosestMatch(ClosestMatchState *state, const char *source, int max_d) +{ + Assert(state); + Assert(max_d >= 0); + + state->source = source; + state->min_d = -1; + state->max_d = max_d; + state->match = NULL; +} + +/* + * If the candidate string is a closer match than the current one saved (or + * there is no match saved), save it as the closest match. + * + * If the source or candidate string is NULL, empty, or too long, this function + * takes no action. Likewise, if the Levenshtein distance exceeds the maximum + * allowed or more than half the characters are different, no action is taken. + */ +void +updateClosestMatch(ClosestMatchState *state, const char *candidate) +{ + int dist; + + Assert(state); + + if (state->source == NULL || state->source[0] == '\0' || + candidate == NULL || candidate[0] == '\0') + return; + + /* + * To avoid ERROR-ing, we check the lengths here instead of setting + * 'trusted' to false in the call to varstr_levenshtein_less_equal(). + */ + if (strlen(state->source) > MAX_LEVENSHTEIN_STRLEN || + strlen(candidate) > MAX_LEVENSHTEIN_STRLEN) + return; + + dist = varstr_levenshtein_less_equal(state->source, strlen(state->source), + candidate, strlen(candidate), 1, 1, 1, + state->max_d, true); + if (dist <= state->max_d && + dist <= strlen(state->source) / 2 && + (state->min_d == -1 || dist < state->min_d)) + { + state->min_d = dist; + state->match = candidate; + } +} + +/* + * Return the closest match. If no suitable candidates were provided via + * updateClosestMatch(), return NULL. + */ +const char * +getClosestMatch(ClosestMatchState *state) +{ + Assert(state); + + return state->match; +} + + /* * Unicode support */ diff --git a/src/include/utils/varlena.h b/src/include/utils/varlena.h index c45208a204..2bc3b6e519 100644 --- a/src/include/utils/varlena.h +++ b/src/include/utils/varlena.h @@ -38,4 +38,16 @@ extern text *replace_text_regexp(text *src_text, text *pattern_text, int cflags, Oid collation, int search_start, int n); +typedef struct ClosestMatchState +{ + const char *source; + int min_d; + int max_d; + const char *match; +} ClosestMatchState; + +extern void initClosestMatch(ClosestMatchState *state, const char *source, int max_d); +extern void updateClosestMatch(ClosestMatchState *state, const char *candidate); +extern const char *getClosestMatch(ClosestMatchState *state); + #endif diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out index 33505352cc..1b4c19a49b 100644 --- a/src/test/regress/expected/foreign_data.out +++ b/src/test/regress/expected/foreign_data.out @@ -329,7 +329,6 @@ CREATE SERVER s6 VERSION '16.0' FOREIGN DATA WRAPPER foo OPTIONS (host 'a', dbna CREATE SERVER s7 TYPE 'oracle' VERSION '17.0' FOREIGN DATA WRAPPER foo OPTIONS (host 'a', dbname 'b'); CREATE SERVER s8 FOREIGN DATA WRAPPER postgresql OPTIONS (foo '1'); -- ERROR ERROR: invalid option "foo" -HINT: Valid options in this context are: authtype, service, connect_timeout, dbname, host, hostaddr, port, tty, options, requiressl, sslmode, gsslib CREATE SERVER s8 FOREIGN DATA WRAPPER postgresql OPTIONS (host 'localhost', dbname 's8db'); \des+ List of foreign servers @@ -440,7 +439,6 @@ ERROR: permission denied for foreign-data wrapper foo RESET ROLE; ALTER SERVER s8 OPTIONS (foo '1'); -- ERROR option validation ERROR: invalid option "foo" -HINT: Valid options in this context are: authtype, service, connect_timeout, dbname, host, hostaddr, port, tty, options, requiressl, sslmode, gsslib ALTER SERVER s8 OPTIONS (connect_timeout '30', SET dbname 'db1', DROP host); SET ROLE regress_test_role; ALTER SERVER s1 OWNER TO regress_test_indirect; -- ERROR @@ -597,7 +595,7 @@ ERROR: user mapping for "regress_foreign_data_user" already exists for server " CREATE USER MAPPING FOR public SERVER s4 OPTIONS ("this mapping" 'is public'); CREATE USER MAPPING FOR user SERVER s8 OPTIONS (username 'test', password 'secret'); -- ERROR ERROR: invalid option "username" -HINT: Valid options in this context are: user, password +HINT: Perhaps you meant to reference the option "user". CREATE USER MAPPING FOR user SERVER s8 OPTIONS (user 'test', password 'secret'); ALTER SERVER s5 OWNER TO regress_test_role; ALTER SERVER s6 OWNER TO regress_test_indirect; @@ -636,7 +634,7 @@ ALTER USER MAPPING FOR public SERVER s5 OPTIONS (gotcha 'true'); -- E ERROR: user mapping for "public" does not exist for server "s5" ALTER USER MAPPING FOR current_user SERVER s8 OPTIONS (username 'test'); -- ERROR ERROR: invalid option "username" -HINT: Valid options in this context are: user, password +HINT: Perhaps you meant to reference the option "user". ALTER USER MAPPING FOR current_user SERVER s8 OPTIONS (DROP user, SET password 'public'); SET ROLE regress_test_role; ALTER USER MAPPING FOR current_user SERVER s5 OPTIONS (ADD modified '1'); -- 2.25.1 --Nq2Wo0NMKNjxTN9z-- ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: File descriptors in exec'd subprocesses @ 2023-02-05 00:03 Thomas Munro <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Thomas Munro @ 2023-02-05 00:03 UTC (permalink / raw) To: pgsql-hackers On Sun, Feb 5, 2023 at 1:00 PM Thomas Munro <[email protected]> wrote: > SUSv3 (POSIX 2008) Oh, oops, 2008 actually corresponds to SUSv4. Hmm. ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: File descriptors in exec'd subprocesses @ 2023-02-05 05:15 Tom Lane <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Tom Lane @ 2023-02-05 05:15 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: pgsql-hackers Thomas Munro <[email protected]> writes: > On Sun, Feb 5, 2023 at 1:00 PM Thomas Munro <[email protected]> wrote: >> SUSv3 (POSIX 2008) > Oh, oops, 2008 actually corresponds to SUSv4. Hmm. Worst case, if we come across some allegedly-supported platform without O_CLOEXEC, we #define that to zero. Said platform is no worse off than it was before. regards, tom lane ^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2023-02-05 05:15 UTC | newest] Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2022-09-02 21:03 [PATCH v4 1/1] Adjust assorted hint messages that list all valid options. Nathan Bossart <[email protected]> 2022-09-02 21:03 [PATCH v4 1/1] Adjust assorted hint messages that list all valid options. Nathan Bossart <[email protected]> 2022-09-02 21:03 [PATCH v2 1/1] Adjust assorted hint messages that list all valid options. Nathan Bossart <[email protected]> 2022-09-02 21:03 [PATCH v3 1/1] Adjust assorted hint messages that list all valid options. Nathan Bossart <[email protected]> 2022-09-02 21:03 [PATCH v2 1/1] Adjust assorted hint messages that list all valid options. Nathan Bossart <[email protected]> 2022-09-02 21:03 [PATCH v3 1/1] Adjust assorted hint messages that list all valid options. Nathan Bossart <[email protected]> 2023-02-05 00:03 Re: File descriptors in exec'd subprocesses Thomas Munro <[email protected]> 2023-02-05 05:15 ` Re: File descriptors in exec'd subprocesses Tom Lane <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox