Date: Thu, 1 Sep 2022 15:31:28 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Robert Haas <robertmhaas@gmail.com>,
	Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: postgres_fdw hint messages
Message-ID: <20220901223128.GA932597@nathanxps13>
References: <b1f9f399-3a1a-b554-283f-4ae7f34608e2@enterprisedb.com>
 <CA+TgmoY+BuSZm-Fbxx5zC6SuML=2F9YvaYuzVAJ=XyE65BnchQ@mail.gmail.com>
 <1929340.1661531738@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="C7zPtVaVf+AK4Oqc"
Content-Disposition: inline
In-Reply-To: <1929340.1661531738@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/20220901223128.GA932597%40nathanxps13>
Precedence: bulk


--C7zPtVaVf+AK4Oqc
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--C7zPtVaVf+AK4Oqc
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="did_you_mean.patch"

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');

--C7zPtVaVf+AK4Oqc--