On Fri, Aug 26, 2022 at 12:35:38PM -0400, Tom Lane wrote: > Peter also mentioned the possibility of "did you mean" with a closest > match offered. That seems like a reasonable idea if someone > is motivated to create the code, which I'm not. > > I vote for just dropping all these hints for now, while leaving the > door open for anyone who wants to write closest-match-offering code.
Here is a quickly-hacked-together proof-of-concept for using Levenshtein distances to determine which option to include in the hint. Would something like this suffice? If so, I will work on polishing it up a bit. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c index e323fdd0e6..7fe42cb732 100644 --- a/contrib/dblink/dblink.c +++ b/contrib/dblink/dblink.c @@ -2006,28 +2006,31 @@ dblink_fdw_validator(PG_FUNCTION_ARGS) if (!is_valid_dblink_option(options, def->defname, context)) { - /* - * Unknown option, or invalid option for the context specified, so - * complain about it. Provide a hint with list of valid options - * for the context. - */ - StringInfoData buf; - const PQconninfoOption *opt; + int min_dist = -1; + char *hint = NULL; - initStringInfo(&buf); - for (opt = options; opt->keyword; opt++) + for (const PQconninfoOption *opt = options; opt->keyword; opt++) { if (is_valid_dblink_option(options, opt->keyword, context)) - appendStringInfo(&buf, "%s%s", - (buf.len > 0) ? ", " : "", - opt->keyword); + { + int dist; + + dist = varstr_levenshtein(def->defname, strlen(def->defname), + opt->keyword, strlen(opt->keyword), + 1, 1, 1, false); + if (min_dist == -1 || dist < min_dist) + { + min_dist = dist; + hint = opt->keyword; + } + } } + ereport(ERROR, (errcode(ERRCODE_FDW_OPTION_NAME_NOT_FOUND), errmsg("invalid option \"%s\"", def->defname), - buf.len > 0 - ? errhint("Valid options in this context are: %s", - buf.data) + hint != NULL + ? errhint("Did you mean \"%s\"?", hint) : errhint("There are no valid options in this context."))); } } diff --git a/contrib/dblink/expected/dblink.out b/contrib/dblink/expected/dblink.out index c7bde6ad07..2b874fc450 100644 --- a/contrib/dblink/expected/dblink.out +++ b/contrib/dblink/expected/dblink.out @@ -897,7 +897,7 @@ $d$; CREATE USER MAPPING FOR public SERVER fdtest OPTIONS (server 'localhost'); -- fail, can't specify server here ERROR: invalid option "server" -HINT: Valid options in this context are: user, password, sslpassword +HINT: Did you mean "user"? CREATE USER MAPPING FOR public SERVER fdtest OPTIONS (user :'USER'); GRANT USAGE ON FOREIGN SERVER fdtest TO regress_dblink_user; GRANT EXECUTE ON FUNCTION dblink_connect_u(text, text) TO regress_dblink_user; diff --git a/contrib/file_fdw/expected/file_fdw.out b/contrib/file_fdw/expected/file_fdw.out index 261af1a8b5..b780b1a13c 100644 --- a/contrib/file_fdw/expected/file_fdw.out +++ b/contrib/file_fdw/expected/file_fdw.out @@ -166,7 +166,7 @@ ERROR: invalid option "force_not_null" HINT: There are no valid options in this context. CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (force_not_null '*'); -- ERROR ERROR: invalid option "force_not_null" -HINT: Valid options in this context are: filename, program, format, header, delimiter, quote, escape, null, encoding +HINT: Did you mean "format"? -- force_null is not allowed to be specified at any foreign object level: ALTER FOREIGN DATA WRAPPER file_fdw OPTIONS (ADD force_null '*'); -- ERROR ERROR: invalid option "force_null" @@ -179,7 +179,7 @@ ERROR: invalid option "force_null" HINT: There are no valid options in this context. CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (force_null '*'); -- ERROR ERROR: invalid option "force_null" -HINT: Valid options in this context are: filename, program, format, header, delimiter, quote, escape, null, encoding +HINT: Did you mean "null"? -- basic query tests SELECT * FROM agg_text WHERE b > 10.0 ORDER BY a; a | b diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c index 4773cadec0..49dec5b7bf 100644 --- a/contrib/file_fdw/file_fdw.c +++ b/contrib/file_fdw/file_fdw.c @@ -37,6 +37,7 @@ #include "utils/memutils.h" #include "utils/rel.h" #include "utils/sampling.h" +#include "utils/varlena.h" PG_MODULE_MAGIC; @@ -213,27 +214,31 @@ file_fdw_validator(PG_FUNCTION_ARGS) if (!is_valid_option(def->defname, catalog)) { - const struct FileFdwOption *opt; - StringInfoData buf; + int min_dist = -1; + const char *hint = NULL; - /* - * Unknown option specified, complain about it. Provide a hint - * with list of valid options for the object. - */ - initStringInfo(&buf); - for (opt = valid_options; opt->optname; opt++) + for (const struct FileFdwOption *opt = valid_options; opt->optname; opt++) { if (catalog == opt->optcontext) - appendStringInfo(&buf, "%s%s", (buf.len > 0) ? ", " : "", - opt->optname); + { + int dist; + + dist = varstr_levenshtein(def->defname, strlen(def->defname), + opt->optname, strlen(opt->optname), + 1, 1, 1, false); + if (min_dist == -1 || dist < min_dist) + { + min_dist = dist; + hint = opt->optname; + } + } } 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) + hint != NULL + ? errhint("Did you mean \"%s\"?", hint) : 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..fe8a3e79f9 100644 --- a/contrib/postgres_fdw/option.c +++ b/contrib/postgres_fdw/option.c @@ -88,27 +88,31 @@ postgres_fdw_validator(PG_FUNCTION_ARGS) if (!is_valid_option(def->defname, catalog)) { - /* - * Unknown option specified, complain about it. Provide a hint - * with list of valid options for the object. - */ - PgFdwOption *opt; - StringInfoData buf; + int min_dist = -1; + const char *hint = NULL; - initStringInfo(&buf); - for (opt = postgres_fdw_options; opt->keyword; opt++) + for (PgFdwOption *opt = postgres_fdw_options; opt->keyword; opt++) { if (catalog == opt->optcontext) - appendStringInfo(&buf, "%s%s", (buf.len > 0) ? ", " : "", - opt->keyword); + { + int dist; + + dist = varstr_levenshtein(def->defname, strlen(def->defname), + opt->keyword, strlen(opt->keyword), + 1, 1, 1, false); + if (min_dist == -1 || dist < min_dist) + { + min_dist = dist; + hint = opt->keyword; + } + } } ereport(ERROR, (errcode(ERRCODE_FDW_INVALID_OPTION_NAME), errmsg("invalid option \"%s\"", def->defname), - buf.len > 0 - ? errhint("Valid options in this context are: %s", - buf.data) + hint != NULL + ? errhint("Did you mean \"%s\"?", hint) : errhint("There are no valid options in this context."))); } diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c index cf222fc3e9..a84116144e 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" /* @@ -620,25 +621,29 @@ postgresql_fdw_validator(PG_FUNCTION_ARGS) if (!is_conninfo_option(def->defname, catalog)) { - const struct ConnectionOption *opt; - StringInfoData buf; - - /* - * Unknown option specified, complain about it. Provide a hint - * with list of valid options for the object. - */ - initStringInfo(&buf); - for (opt = libpq_conninfo_options; opt->optname; opt++) + int min_dist = -1; + const char *hint = NULL; + + for (const struct ConnectionOption *opt = libpq_conninfo_options; opt->optname; opt++) if (catalog == opt->optcontext) - appendStringInfo(&buf, "%s%s", (buf.len > 0) ? ", " : "", - opt->optname); + { + int dist; + + dist = varstr_levenshtein(def->defname, strlen(def->defname), + opt->optname, strlen(opt->optname), + 1, 1, 1, false); + if (min_dist == -1 || dist < min_dist) + { + min_dist = dist; + hint = opt->optname; + } + } 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) + hint != NULL + ? errhint("Did you mean \"%s\"?", hint) : errhint("There are no valid options in this context."))); PG_RETURN_BOOL(false); 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');