public inbox for [email protected]
help / color / mirror / Atom feedFrom: Nathan Bossart <[email protected]>
Subject: [PATCH v4 1/1] Adjust assorted hint messages that list all valid options.
Date: Fri, 2 Sep 2022 14:03:26 -0700
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--
view thread (7+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected]
Subject: Re: [PATCH v4 1/1] Adjust assorted hint messages that list all valid options.
In-Reply-To: <no-message-id-1860258@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox