On Thu, Sep 01, 2022 at 04:31:20PM -0700, Nathan Bossart wrote:
> On Thu, Sep 01, 2022 at 07:08:49PM -0400, Tom Lane wrote:
>> (1) there probably needs to be some threshold of closeness, so we don't
>> offer "foobar" when the user wrote "huh"
>
> Agreed.
>
>> (2) there are several places doing this now, and there will no doubt
>> be more later, so we need to try to abstract the logic so it can be
>> shared.
>
> Will do.
Here is a new patch. Two notes:
* I considered whether to try to unite this new functionality with the
existing stuff in parse_relation.c, but the existing code looked a bit too
specialized.
* I chose a Levenshtein distance of 5 as the threshold of closeness for the
hint messages. This felt lenient, but it should hopefully still filter out
some of the more ridiculous suggestions. However, it's still little more
than a wild guess, so if folks think the threshold needs to be higher or
lower, I'd readily change it.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From c3c7793daf7e6b225605a44a1faead2f7678bca3 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandboss...@gmail.com>
Date: Fri, 2 Sep 2022 14:03:26 -0700
Subject: [PATCH v2 1/1] Adjust assorted hint messages that list all valid
options.
Instead of listing all valid options, we now try to provide one
that looks similar. Since this may be useful elsewhere, this
change introduces a new set of functions that can be reused for
similar purposes.
---
contrib/dblink/dblink.c | 26 +++---
contrib/dblink/expected/dblink.out | 2 +-
contrib/file_fdw/expected/file_fdw.out | 2 -
contrib/file_fdw/file_fdw.c | 23 ++++--
.../postgres_fdw/expected/postgres_fdw.out | 4 +-
contrib/postgres_fdw/option.c | 22 +++--
src/backend/foreign/foreign.c | 25 ++++--
src/backend/utils/adt/varlena.c | 82 +++++++++++++++++++
src/include/utils/varlena.h | 12 +++
src/test/regress/expected/foreign_data.out | 8 +-
10 files changed, 159 insertions(+), 47 deletions(-)
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index e323fdd0e6..c134f73adb 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -2008,27 +2008,31 @@ dblink_fdw_validator(PG_FUNCTION_ARGS)
{
/*
* Unknown option, or invalid option for the context specified, so
- * complain about it. Provide a hint with list of valid options
- * for the context.
+ * complain about it. Provide a hint with a valid option that
+ * looks similar, if there is one.
*/
- StringInfoData buf;
const PQconninfoOption *opt;
+ const char *closest_match;
+ ClosestMatchState match_state;
+ bool has_valid_options = false;
- initStringInfo(&buf);
+ initClosestMatch(&match_state, def->defname, 5);
for (opt = options; opt->keyword; opt++)
{
if (is_valid_dblink_option(options, opt->keyword, context))
- appendStringInfo(&buf, "%s%s",
- (buf.len > 0) ? ", " : "",
- opt->keyword);
+ {
+ has_valid_options = true;
+ updateClosestMatch(&match_state, opt->keyword);
+ }
}
+
+ closest_match = getClosestMatch(&match_state);
ereport(ERROR,
(errcode(ERRCODE_FDW_OPTION_NAME_NOT_FOUND),
errmsg("invalid option \"%s\"", def->defname),
- buf.len > 0
- ? errhint("Valid options in this context are: %s",
- buf.data)
- : errhint("There are no valid options in this context.")));
+ has_valid_options ? closest_match ?
+ errhint("Did you mean \"%s\"?", closest_match) : 0 :
+ errhint("There are no valid options in this context.")));
}
}
diff --git a/contrib/dblink/expected/dblink.out b/contrib/dblink/expected/dblink.out
index c7bde6ad07..2b874fc450 100644
--- a/contrib/dblink/expected/dblink.out
+++ b/contrib/dblink/expected/dblink.out
@@ -897,7 +897,7 @@ $d$;
CREATE USER MAPPING FOR public SERVER fdtest
OPTIONS (server 'localhost'); -- fail, can't specify server here
ERROR: invalid option "server"
-HINT: Valid options in this context are: user, password, sslpassword
+HINT: Did you mean "user"?
CREATE USER MAPPING FOR public SERVER fdtest OPTIONS (user :'USER');
GRANT USAGE ON FOREIGN SERVER fdtest TO regress_dblink_user;
GRANT EXECUTE ON FUNCTION dblink_connect_u(text, text) TO regress_dblink_user;
diff --git a/contrib/file_fdw/expected/file_fdw.out b/contrib/file_fdw/expected/file_fdw.out
index 261af1a8b5..36d76ba26c 100644
--- a/contrib/file_fdw/expected/file_fdw.out
+++ b/contrib/file_fdw/expected/file_fdw.out
@@ -166,7 +166,6 @@ ERROR: invalid option "force_not_null"
HINT: There are no valid options in this context.
CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (force_not_null '*'); -- ERROR
ERROR: invalid option "force_not_null"
-HINT: Valid options in this context are: filename, program, format, header, delimiter, quote, escape, null, encoding
-- force_null is not allowed to be specified at any 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
