postgres_fdw hint messages

Started by Peter Eisentrautover 3 years ago18 messages
#1Peter Eisentraut
peter.eisentraut@enterprisedb.com
2 attachment(s)

The postgres_fdw tests contain this (as amended by patch 0001):

ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw');
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

This annoys developers who are working on libpq connection options,
because any option added, removed, or changed causes this test to need
to be updated.

It's also questionable how useful this hint is in its current form,
considering how long it is and that the options are in an
implementation-dependent order.

Possible changes:

- Hide the hint from this particular test (done in the attached patches).

- Keep the hint, but maybe make it sorted?

- Remove all the hints like this from foreign data commands.

- Don't show the hint when there are more than N valid options.

- Do some kind of "did you mean" like we have for column names.

Thoughts?

Attachments:

0001-postgres_fdw-Remove-useless-DO-block-in-test.patchtext/plain; charset=UTF-8; name=0001-postgres_fdw-Remove-useless-DO-block-in-test.patchDownload
From 4adb3b6846e98a55ee19d8425dfb3d6d12145f16 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 25 Aug 2022 15:31:10 +0200
Subject: [PATCH 1/2] postgres_fdw: Remove useless DO block in test

---
 contrib/postgres_fdw/expected/postgres_fdw.out | 8 +-------
 contrib/postgres_fdw/sql/postgres_fdw.sql      | 6 +-----
 2 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 7bf35602b0..a42c9720c3 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -9621,15 +9621,9 @@ ERROR:  password is required
 DETAIL:  Non-superusers must provide a password in the user mapping.
 -- If we add a password to the connstr it'll fail, because we don't allow passwords
 -- in connstrs only in user mappings.
-DO $d$
-    BEGIN
-        EXECUTE $$ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw')$$;
-    END;
-$d$;
+ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw');
 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
-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
 -- error because the password wasn't actually *used* when we run with trust auth.
 --
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 42735ae78a..63581a457d 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -2886,11 +2886,7 @@ CREATE FOREIGN TABLE pg_temp.ft1_nopw (
 -- If we add a password to the connstr it'll fail, because we don't allow passwords
 -- in connstrs only in user mappings.
 
-DO $d$
-    BEGIN
-        EXECUTE $$ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw')$$;
-    END;
-$d$;
+ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw');
 
 -- If we add a password for our user mapping instead, we should get a different
 -- error because the password wasn't actually *used* when we run with trust auth.
-- 
2.37.1

0002-postgres_fdw-Avoid-listing-all-libpq-connection-opti.patchtext/plain; charset=UTF-8; name=0002-postgres_fdw-Avoid-listing-all-libpq-connection-opti.patchDownload
From 986ea253cd887b472e8040580f8c1a2642f3cada Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 25 Aug 2022 15:31:11 +0200
Subject: [PATCH 2/2] postgres_fdw: Avoid listing all libpq connection options
 in error hint

---
 contrib/postgres_fdw/expected/postgres_fdw.out | 7 ++++++-
 contrib/postgres_fdw/sql/postgres_fdw.sql      | 6 ++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index a42c9720c3..3678428b95 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -9621,9 +9621,14 @@ ERROR:  password is required
 DETAIL:  Non-superusers must provide a password in the user mapping.
 -- If we add a password to the connstr it'll fail, because we don't allow passwords
 -- in connstrs only in user mappings.
+--
+-- Set to terse verbosity to avoid hint listing all possible libpq
+-- connection options, which causes annoying test failures when doing
+-- libpq work.
+\set VERBOSITY terse
 ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw');
 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
+\set VERBOSITY default
 -- If we add a password for our user mapping instead, we should get a different
 -- error because the password wasn't actually *used* when we run with trust auth.
 --
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 63581a457d..a8b65cdc88 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -2885,8 +2885,14 @@ CREATE FOREIGN TABLE pg_temp.ft1_nopw (
 
 -- If we add a password to the connstr it'll fail, because we don't allow passwords
 -- in connstrs only in user mappings.
+--
+-- Set to terse verbosity to avoid hint listing all possible libpq
+-- connection options, which causes annoying test failures when doing
+-- libpq work.
 
+\set VERBOSITY terse
 ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw');
+\set VERBOSITY default
 
 -- If we add a password for our user mapping instead, we should get a different
 -- error because the password wasn't actually *used* when we run with trust auth.
-- 
2.37.1

#2Ranier Vilela
ranier.vf@gmail.com
In reply to: Peter Eisentraut (#1)
1 attachment(s)
re: postgres_fdw hint messages

The postgres_fdw tests contain this (as amended by patch 0001):

ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw');
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

This annoys developers who are working on libpq connection options,
because any option added, removed, or changed causes this test to need
to be updated.

It's also questionable how useful this hint is in its current form,
considering how long it is and that the options are in an
implementation-dependent order.

Possible changes:

- Hide the hint from this particular test (done in the attached patches).

+1
I vote for this option.
Less work for future developers changes, I think worth the effort.

Anyway, in alphabetical order, it's a lot easier for users to read.

Patch attached.

regards,
Ranier Vilela

Attachments:

0003-reorder-alphabetical-libpq-options.patchapplication/octet-stream; name=0003-reorder-alphabetical-libpq-options.patchDownload
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 95dde056eb..c0bcf489a7 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -222,36 +222,25 @@ InitPgFdwOptions(void)
 
 	/* non-libpq FDW-specific FDW options */
 	static const PgFdwOption non_libpq_options[] = {
-		{"schema_name", ForeignTableRelationId, false},
-		{"table_name", ForeignTableRelationId, false},
+		/* async_capable is available on both server and table */
+		{"async_capable", ForeignServerRelationId, false},
+		{"async_capable", ForeignTableRelationId, false},
+		/* batch_size is available on both server and table */
+		{"batch_size", ForeignServerRelationId, false},
+		{"batch_size", ForeignTableRelationId, false},
 		{"column_name", AttributeRelationId, false},
-		/* use_remote_estimate is available on both server and table */
-		{"use_remote_estimate", ForeignServerRelationId, false},
-		{"use_remote_estimate", ForeignTableRelationId, false},
-		/* cost factors */
-		{"fdw_startup_cost", ForeignServerRelationId, false},
-		{"fdw_tuple_cost", ForeignServerRelationId, false},
 		/* shippable extensions */
 		{"extensions", ForeignServerRelationId, false},
-		/* updatable is available on both server and table */
-		{"updatable", ForeignServerRelationId, false},
-		{"updatable", ForeignTableRelationId, false},
-		/* truncatable is available on both server and table */
-		{"truncatable", ForeignServerRelationId, false},
-		{"truncatable", ForeignTableRelationId, false},
+		{"fdw_startup_cost", ForeignServerRelationId, false},
+		{"fdw_tuple_cost", ForeignServerRelationId, false},
 		/* fetch_size is available on both server and table */
 		{"fetch_size", ForeignServerRelationId, false},
 		{"fetch_size", ForeignTableRelationId, false},
-		/* batch_size is available on both server and table */
-		{"batch_size", ForeignServerRelationId, false},
-		{"batch_size", ForeignTableRelationId, false},
-		/* async_capable is available on both server and table */
-		{"async_capable", ForeignServerRelationId, false},
-		{"async_capable", ForeignTableRelationId, false},
-		{"parallel_commit", ForeignServerRelationId, false},
+		/* cost factors */
 		{"keep_connections", ForeignServerRelationId, false},
+		{"parallel_commit", ForeignServerRelationId, false},
 		{"password_required", UserMappingRelationId, false},
-
+		{"schema_name", ForeignTableRelationId, false},
 		/*
 		 * sslcert and sslkey are in fact libpq options, but we repeat them
 		 * here to allow them to appear in both foreign server context (when
@@ -259,7 +248,16 @@ InitPgFdwOptions(void)
 		 */
 		{"sslcert", UserMappingRelationId, true},
 		{"sslkey", UserMappingRelationId, true},
-
+		{"table_name", ForeignTableRelationId, false},
+		/* truncatable is available on both server and table */
+		{"truncatable", ForeignServerRelationId, false},
+		{"truncatable", ForeignTableRelationId, false},
+		/* updatable is available on both server and table */
+		{"updatable", ForeignServerRelationId, false},
+		{"updatable", ForeignTableRelationId, false},
+		/* use_remote_estimate is available on both server and table */
+		{"use_remote_estimate", ForeignServerRelationId, false},
+		{"use_remote_estimate", ForeignTableRelationId, false},
 		{NULL, InvalidOid, false}
 	};
diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c
index cf222fc3e9..fc4afb1e0b 100644
--- a/src/backend/foreign/foreign.c
+++ b/src/backend/foreign/foreign.c
@@ -560,19 +560,19 @@ struct ConnectionOption
  */
 static const struct ConnectionOption libpq_conninfo_options[] = {
 	{"authtype", ForeignServerRelationId},
-	{"service", ForeignServerRelationId},
-	{"user", UserMappingRelationId},
-	{"password", UserMappingRelationId},
 	{"connect_timeout", ForeignServerRelationId},
 	{"dbname", ForeignServerRelationId},
 	{"host", ForeignServerRelationId},
 	{"hostaddr", ForeignServerRelationId},
-	{"port", ForeignServerRelationId},
-	{"tty", ForeignServerRelationId},
+	{"gsslib", ForeignServerRelationId},
 	{"options", ForeignServerRelationId},
+	{"password", UserMappingRelationId},
+	{"port", ForeignServerRelationId},
 	{"requiressl", ForeignServerRelationId},
+	{"service", ForeignServerRelationId},
 	{"sslmode", ForeignServerRelationId},
-	{"gsslib", ForeignServerRelationId},
+	{"tty", ForeignServerRelationId},
+	{"user", UserMappingRelationId},
 	{NULL, InvalidOid}
 };
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 8cefef20d1..f5d05f13f4 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -186,25 +186,18 @@ typedef struct _internalPQconninfoOption
 } internalPQconninfoOption;
 
 static const internalPQconninfoOption PQconninfoOptions[] = {
-	{"service", "PGSERVICE", NULL, NULL,
-	"Database-Service", "", 20, -1},
-
-	{"user", "PGUSER", NULL, NULL,
-		"Database-User", "", 20,
-	offsetof(struct pg_conn, pguser)},
-
-	{"password", "PGPASSWORD", NULL, NULL,
-		"Database-Password", "*", 20,
-	offsetof(struct pg_conn, pgpass)},
-
-	{"passfile", "PGPASSFILE", NULL, NULL,
-		"Database-Password-File", "", 64,
-	offsetof(struct pg_conn, pgpassfile)},
+	{"application_name", "PGAPPNAME", NULL, NULL,
+		"Application-Name", "", 64,
+	offsetof(struct pg_conn, appname)},
 
 	{"channel_binding", "PGCHANNELBINDING", DefaultChannelBinding, NULL,
 		"Channel-Binding", "", 8,	/* sizeof("require") == 8 */
 	offsetof(struct pg_conn, channel_binding)},
 
+	{"client_encoding", "PGCLIENTENCODING", NULL, NULL,
+		"Client-Encoding", "", 10,
+	offsetof(struct pg_conn, client_encoding_initial)},
+
 	{"connect_timeout", "PGCONNECT_TIMEOUT", NULL, NULL,
 		"Connect-timeout", "", 10,	/* strlen(INT32_MAX) == 10 */
 	offsetof(struct pg_conn, connect_timeout)},
@@ -213,6 +206,10 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
 		"Database-Name", "", 20,
 	offsetof(struct pg_conn, dbName)},
 
+	{"fallback_application_name", NULL, NULL, NULL,
+		"Fallback-Application-Name", "", 64,
+	offsetof(struct pg_conn, fbappname)},
+
 	{"host", "PGHOST", NULL, NULL,
 		"Database-Host", "", 40,
 	offsetof(struct pg_conn, pghost)},
@@ -221,30 +218,26 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
 		"Database-Host-IP-Address", "", 45,
 	offsetof(struct pg_conn, pghostaddr)},
 
-	{"port", "PGPORT", DEF_PGPORT_STR, NULL,
-		"Database-Port", "", 6,
-	offsetof(struct pg_conn, pgport)},
-
-	{"client_encoding", "PGCLIENTENCODING", NULL, NULL,
-		"Client-Encoding", "", 10,
-	offsetof(struct pg_conn, client_encoding_initial)},
-
-	{"options", "PGOPTIONS", DefaultOption, NULL,
-		"Backend-Options", "", 40,
-	offsetof(struct pg_conn, pgoptions)},
-
-	{"application_name", "PGAPPNAME", NULL, NULL,
-		"Application-Name", "", 64,
-	offsetof(struct pg_conn, appname)},
+	/*
+	 * As with SSL, all GSS options are exposed even in builds that don't have
+	 * support.
+	 */
+	{"gssencmode", "PGGSSENCMODE", DefaultGSSMode, NULL,
+		"GSSENC-Mode", "", 8,	/* sizeof("disable") == 8 */
+	offsetof(struct pg_conn, gssencmode)},
 
-	{"fallback_application_name", NULL, NULL, NULL,
-		"Fallback-Application-Name", "", 64,
-	offsetof(struct pg_conn, fbappname)},
+	{"gsslib", "PGGSSLIB", NULL, NULL,
+		"GSS-library", "", 7,	/* sizeof("gssapi") == 7 */
+	offsetof(struct pg_conn, gsslib)},
 
 	{"keepalives", NULL, NULL, NULL,
 		"TCP-Keepalives", "", 1,	/* should be just '0' or '1' */
 	offsetof(struct pg_conn, keepalives)},
 
+	{"keepalives_count", NULL, NULL, NULL,
+		"TCP-Keepalives-Count", "", 10, /* strlen(INT32_MAX) == 10 */
+	offsetof(struct pg_conn, keepalives_count)},
+
 	{"keepalives_idle", NULL, NULL, NULL,
 		"TCP-Keepalives-Idle", "", 10,	/* strlen(INT32_MAX) == 10 */
 	offsetof(struct pg_conn, keepalives_idle)},
@@ -253,13 +246,37 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
 		"TCP-Keepalives-Interval", "", 10,	/* strlen(INT32_MAX) == 10 */
 	offsetof(struct pg_conn, keepalives_interval)},
 
-	{"keepalives_count", NULL, NULL, NULL,
-		"TCP-Keepalives-Count", "", 10, /* strlen(INT32_MAX) == 10 */
-	offsetof(struct pg_conn, keepalives_count)},
+	/* Kerberos and GSSAPI authentication support specifying the service name */
+	{"krbsrvname", "PGKRBSRVNAME", PG_KRB_SRVNAM, NULL,
+		"Kerberos-service-name", "", 20,
+	offsetof(struct pg_conn, krbsrvname)},
 
-	{"tcp_user_timeout", NULL, NULL, NULL,
-		"TCP-User-Timeout", "", 10, /* strlen(INT32_MAX) == 10 */
-	offsetof(struct pg_conn, pgtcp_user_timeout)},
+	{"options", "PGOPTIONS", DefaultOption, NULL,
+		"Backend-Options", "", 40,
+	offsetof(struct pg_conn, pgoptions)},
+
+	{"password", "PGPASSWORD", NULL, NULL,
+		"Database-Password", "*", 20,
+	offsetof(struct pg_conn, pgpass)},
+
+	{"passfile", "PGPASSFILE", NULL, NULL,
+		"Database-Password-File", "", 64,
+	offsetof(struct pg_conn, pgpassfile)},
+
+	{"port", "PGPORT", DEF_PGPORT_STR, NULL,
+		"Database-Port", "", 6,
+	offsetof(struct pg_conn, pgport)},
+
+	{"replication", NULL, NULL, NULL,
+		"Replication", "D", 5,
+	offsetof(struct pg_conn, replication)},
+
+	{"requirepeer", "PGREQUIREPEER", NULL, NULL,
+		"Require-Peer", "", 10,
+	offsetof(struct pg_conn, requirepeer)},
+
+	{"service", "PGSERVICE", NULL, NULL,
+	"Database-Service", "", 20, -1},
 
 	/*
 	 * ssl options are allowed even without client SSL support because the
@@ -267,22 +284,30 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
 	 * parameters have no effect on non-SSL connections, so there is no reason
 	 * to exclude them since none of them are mandatory.
 	 */
-	{"sslmode", "PGSSLMODE", DefaultSSLMode, NULL,
-		"SSL-Mode", "", 12,		/* sizeof("verify-full") == 12 */
-	offsetof(struct pg_conn, sslmode)},
+	{"sslcert", "PGSSLCERT", NULL, NULL,
+		"SSL-Client-Cert", "", 64,
+	offsetof(struct pg_conn, sslcert)},
 
 	{"sslcompression", "PGSSLCOMPRESSION", "0", NULL,
 		"SSL-Compression", "", 1,
 	offsetof(struct pg_conn, sslcompression)},
 
-	{"sslcert", "PGSSLCERT", NULL, NULL,
-		"SSL-Client-Cert", "", 64,
-	offsetof(struct pg_conn, sslcert)},
+	{"sslcrl", "PGSSLCRL", NULL, NULL,
+		"SSL-Revocation-List", "", 64,
+	offsetof(struct pg_conn, sslcrl)},
+
+	{"sslcrldir", "PGSSLCRLDIR", NULL, NULL,
+		"SSL-Revocation-List-Dir", "", 64,
+	offsetof(struct pg_conn, sslcrldir)},
 
 	{"sslkey", "PGSSLKEY", NULL, NULL,
 		"SSL-Client-Key", "", 64,
 	offsetof(struct pg_conn, sslkey)},
 
+	{"sslmode", "PGSSLMODE", DefaultSSLMode, NULL,
+		"SSL-Mode", "", 12,		/* sizeof("verify-full") == 12 */
+	offsetof(struct pg_conn, sslmode)},
+
 	{"sslpassword", NULL, NULL, NULL,
 		"SSL-Client-Key-Password", "*", 20,
 	offsetof(struct pg_conn, sslpassword)},
@@ -291,56 +316,31 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
 		"SSL-Root-Certificate", "", 64,
 	offsetof(struct pg_conn, sslrootcert)},
 
-	{"sslcrl", "PGSSLCRL", NULL, NULL,
-		"SSL-Revocation-List", "", 64,
-	offsetof(struct pg_conn, sslcrl)},
-
-	{"sslcrldir", "PGSSLCRLDIR", NULL, NULL,
-		"SSL-Revocation-List-Dir", "", 64,
-	offsetof(struct pg_conn, sslcrldir)},
-
 	{"sslsni", "PGSSLSNI", "1", NULL,
 		"SSL-SNI", "", 1,
 	offsetof(struct pg_conn, sslsni)},
 
-	{"requirepeer", "PGREQUIREPEER", NULL, NULL,
-		"Require-Peer", "", 10,
-	offsetof(struct pg_conn, requirepeer)},
-
-	{"ssl_min_protocol_version", "PGSSLMINPROTOCOLVERSION", "TLSv1.2", NULL,
-		"SSL-Minimum-Protocol-Version", "", 8,	/* sizeof("TLSv1.x") == 8 */
-	offsetof(struct pg_conn, ssl_min_protocol_version)},
-
 	{"ssl_max_protocol_version", "PGSSLMAXPROTOCOLVERSION", NULL, NULL,
 		"SSL-Maximum-Protocol-Version", "", 8,	/* sizeof("TLSv1.x") == 8 */
 	offsetof(struct pg_conn, ssl_max_protocol_version)},
 
-	/*
-	 * As with SSL, all GSS options are exposed even in builds that don't have
-	 * support.
-	 */
-	{"gssencmode", "PGGSSENCMODE", DefaultGSSMode, NULL,
-		"GSSENC-Mode", "", 8,	/* sizeof("disable") == 8 */
-	offsetof(struct pg_conn, gssencmode)},
-
-	/* Kerberos and GSSAPI authentication support specifying the service name */
-	{"krbsrvname", "PGKRBSRVNAME", PG_KRB_SRVNAM, NULL,
-		"Kerberos-service-name", "", 20,
-	offsetof(struct pg_conn, krbsrvname)},
-
-	{"gsslib", "PGGSSLIB", NULL, NULL,
-		"GSS-library", "", 7,	/* sizeof("gssapi") == 7 */
-	offsetof(struct pg_conn, gsslib)},
-
-	{"replication", NULL, NULL, NULL,
-		"Replication", "D", 5,
-	offsetof(struct pg_conn, replication)},
+	{"ssl_min_protocol_version", "PGSSLMINPROTOCOLVERSION", "TLSv1.2", NULL,
+		"SSL-Minimum-Protocol-Version", "", 8,	/* sizeof("TLSv1.x") == 8 */
+	offsetof(struct pg_conn, ssl_min_protocol_version)},
 
 	{"target_session_attrs", "PGTARGETSESSIONATTRS",
 		DefaultTargetSessionAttrs, NULL,
 		"Target-Session-Attrs", "", 15, /* sizeof("prefer-standby") = 15 */
 	offsetof(struct pg_conn, target_session_attrs)},
 
+	{"tcp_user_timeout", NULL, NULL, NULL,
+		"TCP-User-Timeout", "", 10, /* strlen(INT32_MAX) == 10 */
+	offsetof(struct pg_conn, pgtcp_user_timeout)},
+
+	{"user", "PGUSER", NULL, NULL,
+		"Database-User", "", 20,
+	offsetof(struct pg_conn, pguser)},
+
 	/* Terminating entry --- MUST BE LAST */
 	{NULL, NULL, NULL, NULL,
 	NULL, NULL, 0}
diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out
index 33505352cc..d5ffc4f16a 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:  Valid options in this context are: authtype, connect_timeout, dbname, host, hostaddr, gsslib, options, port, requiressl, service, sslmode, tty
 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:  Valid options in this context are: authtype, connect_timeout, dbname, host, hostaddr, gsslib, options, port, requiressl, service, sslmode, tty
 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:  Valid options in this context are: password, 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:  Valid options in this context are: password, 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');
#3Ranier Vilela
ranier.vf@gmail.com
In reply to: Ranier Vilela (#2)
1 attachment(s)
Re: postgres_fdw hint messages

Em qui., 25 de ago. de 2022 às 14:31, Ranier Vilela <ranier.vf@gmail.com>
escreveu:

The postgres_fdw tests contain this (as amended by patch 0001):

ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw');
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

This annoys developers who are working on libpq connection options,
because any option added, removed, or changed causes this test to need
to be updated.

It's also questionable how useful this hint is in its current form,
considering how long it is and that the options are in an
implementation-dependent order.

Possible changes:

- Hide the hint from this particular test (done in the attached patches).

+1
I vote for this option.
Less work for future developers changes, I think worth the effort.

Anyway, in alphabetical order, it's a lot easier for users to read.

Patch attached.

Little tweak in the comments.

regards,
Ranier Vilela

Show quoted text

Attachments:

v1-0003-reorder-alphabetical-libpq-options.patchapplication/octet-stream; name=v1-0003-reorder-alphabetical-libpq-options.patchDownload
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 95dde056eb..eb76c07fa6 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -222,36 +222,25 @@ InitPgFdwOptions(void)
 
 	/* non-libpq FDW-specific FDW options */
 	static const PgFdwOption non_libpq_options[] = {
-		{"schema_name", ForeignTableRelationId, false},
-		{"table_name", ForeignTableRelationId, false},
+		/* async_capable is available on both server and table */
+		{"async_capable", ForeignServerRelationId, false},
+		{"async_capable", ForeignTableRelationId, false},
+		/* batch_size is available on both server and table */
+		{"batch_size", ForeignServerRelationId, false},
+		{"batch_size", ForeignTableRelationId, false},
 		{"column_name", AttributeRelationId, false},
-		/* use_remote_estimate is available on both server and table */
-		{"use_remote_estimate", ForeignServerRelationId, false},
-		{"use_remote_estimate", ForeignTableRelationId, false},
+		/* shippable extensions */
+		{"extensions", ForeignServerRelationId, false},
 		/* cost factors */
 		{"fdw_startup_cost", ForeignServerRelationId, false},
 		{"fdw_tuple_cost", ForeignServerRelationId, false},
-		/* shippable extensions */
-		{"extensions", ForeignServerRelationId, false},
-		/* updatable is available on both server and table */
-		{"updatable", ForeignServerRelationId, false},
-		{"updatable", ForeignTableRelationId, false},
-		/* truncatable is available on both server and table */
-		{"truncatable", ForeignServerRelationId, false},
-		{"truncatable", ForeignTableRelationId, false},
 		/* fetch_size is available on both server and table */
 		{"fetch_size", ForeignServerRelationId, false},
 		{"fetch_size", ForeignTableRelationId, false},
-		/* batch_size is available on both server and table */
-		{"batch_size", ForeignServerRelationId, false},
-		{"batch_size", ForeignTableRelationId, false},
-		/* async_capable is available on both server and table */
-		{"async_capable", ForeignServerRelationId, false},
-		{"async_capable", ForeignTableRelationId, false},
-		{"parallel_commit", ForeignServerRelationId, false},
 		{"keep_connections", ForeignServerRelationId, false},
+		{"parallel_commit", ForeignServerRelationId, false},
 		{"password_required", UserMappingRelationId, false},
-
+		{"schema_name", ForeignTableRelationId, false},
 		/*
 		 * sslcert and sslkey are in fact libpq options, but we repeat them
 		 * here to allow them to appear in both foreign server context (when
@@ -259,7 +248,16 @@ InitPgFdwOptions(void)
 		 */
 		{"sslcert", UserMappingRelationId, true},
 		{"sslkey", UserMappingRelationId, true},
-
+		{"table_name", ForeignTableRelationId, false},
+		/* truncatable is available on both server and table */
+		{"truncatable", ForeignServerRelationId, false},
+		{"truncatable", ForeignTableRelationId, false},
+		/* updatable is available on both server and table */
+		{"updatable", ForeignServerRelationId, false},
+		{"updatable", ForeignTableRelationId, false},
+		/* use_remote_estimate is available on both server and table */
+		{"use_remote_estimate", ForeignServerRelationId, false},
+		{"use_remote_estimate", ForeignTableRelationId, false},
 		{NULL, InvalidOid, false}
 	};
diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c
index cf222fc3e9..fc4afb1e0b 100644
--- a/src/backend/foreign/foreign.c
+++ b/src/backend/foreign/foreign.c
@@ -560,19 +560,19 @@ struct ConnectionOption
  */
 static const struct ConnectionOption libpq_conninfo_options[] = {
 	{"authtype", ForeignServerRelationId},
-	{"service", ForeignServerRelationId},
-	{"user", UserMappingRelationId},
-	{"password", UserMappingRelationId},
 	{"connect_timeout", ForeignServerRelationId},
 	{"dbname", ForeignServerRelationId},
 	{"host", ForeignServerRelationId},
 	{"hostaddr", ForeignServerRelationId},
-	{"port", ForeignServerRelationId},
-	{"tty", ForeignServerRelationId},
+	{"gsslib", ForeignServerRelationId},
 	{"options", ForeignServerRelationId},
+	{"password", UserMappingRelationId},
+	{"port", ForeignServerRelationId},
 	{"requiressl", ForeignServerRelationId},
+	{"service", ForeignServerRelationId},
 	{"sslmode", ForeignServerRelationId},
-	{"gsslib", ForeignServerRelationId},
+	{"tty", ForeignServerRelationId},
+	{"user", UserMappingRelationId},
 	{NULL, InvalidOid}
 };
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 8cefef20d1..b6331618f8 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -186,25 +186,18 @@ typedef struct _internalPQconninfoOption
 } internalPQconninfoOption;
 
 static const internalPQconninfoOption PQconninfoOptions[] = {
-	{"service", "PGSERVICE", NULL, NULL,
-	"Database-Service", "", 20, -1},
-
-	{"user", "PGUSER", NULL, NULL,
-		"Database-User", "", 20,
-	offsetof(struct pg_conn, pguser)},
-
-	{"password", "PGPASSWORD", NULL, NULL,
-		"Database-Password", "*", 20,
-	offsetof(struct pg_conn, pgpass)},
-
-	{"passfile", "PGPASSFILE", NULL, NULL,
-		"Database-Password-File", "", 64,
-	offsetof(struct pg_conn, pgpassfile)},
+	{"application_name", "PGAPPNAME", NULL, NULL,
+		"Application-Name", "", 64,
+	offsetof(struct pg_conn, appname)},
 
 	{"channel_binding", "PGCHANNELBINDING", DefaultChannelBinding, NULL,
 		"Channel-Binding", "", 8,	/* sizeof("require") == 8 */
 	offsetof(struct pg_conn, channel_binding)},
 
+	{"client_encoding", "PGCLIENTENCODING", NULL, NULL,
+		"Client-Encoding", "", 10,
+	offsetof(struct pg_conn, client_encoding_initial)},
+
 	{"connect_timeout", "PGCONNECT_TIMEOUT", NULL, NULL,
 		"Connect-timeout", "", 10,	/* strlen(INT32_MAX) == 10 */
 	offsetof(struct pg_conn, connect_timeout)},
@@ -213,6 +206,10 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
 		"Database-Name", "", 20,
 	offsetof(struct pg_conn, dbName)},
 
+	{"fallback_application_name", NULL, NULL, NULL,
+		"Fallback-Application-Name", "", 64,
+	offsetof(struct pg_conn, fbappname)},
+
 	{"host", "PGHOST", NULL, NULL,
 		"Database-Host", "", 40,
 	offsetof(struct pg_conn, pghost)},
@@ -221,30 +218,27 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
 		"Database-Host-IP-Address", "", 45,
 	offsetof(struct pg_conn, pghostaddr)},
 
-	{"port", "PGPORT", DEF_PGPORT_STR, NULL,
-		"Database-Port", "", 6,
-	offsetof(struct pg_conn, pgport)},
-
-	{"client_encoding", "PGCLIENTENCODING", NULL, NULL,
-		"Client-Encoding", "", 10,
-	offsetof(struct pg_conn, client_encoding_initial)},
-
-	{"options", "PGOPTIONS", DefaultOption, NULL,
-		"Backend-Options", "", 40,
-	offsetof(struct pg_conn, pgoptions)},
-
-	{"application_name", "PGAPPNAME", NULL, NULL,
-		"Application-Name", "", 64,
-	offsetof(struct pg_conn, appname)},
+	/*
+	 * As with SSL, all GSS options are exposed even in builds that don't have
+	 * support.
+	 */
+	{"gssencmode", "PGGSSENCMODE", DefaultGSSMode, NULL,
+		"GSSENC-Mode", "", 8,	/* sizeof("disable") == 8 */
+	offsetof(struct pg_conn, gssencmode)},
 
-	{"fallback_application_name", NULL, NULL, NULL,
-		"Fallback-Application-Name", "", 64,
-	offsetof(struct pg_conn, fbappname)},
+	/* GSSAPI authentication support specifying the service name */
+	{"gsslib", "PGGSSLIB", NULL, NULL,
+		"GSS-library", "", 7,	/* sizeof("gssapi") == 7 */
+	offsetof(struct pg_conn, gsslib)},
 
 	{"keepalives", NULL, NULL, NULL,
 		"TCP-Keepalives", "", 1,	/* should be just '0' or '1' */
 	offsetof(struct pg_conn, keepalives)},
 
+	{"keepalives_count", NULL, NULL, NULL,
+		"TCP-Keepalives-Count", "", 10, /* strlen(INT32_MAX) == 10 */
+	offsetof(struct pg_conn, keepalives_count)},
+
 	{"keepalives_idle", NULL, NULL, NULL,
 		"TCP-Keepalives-Idle", "", 10,	/* strlen(INT32_MAX) == 10 */
 	offsetof(struct pg_conn, keepalives_idle)},
@@ -253,13 +247,37 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
 		"TCP-Keepalives-Interval", "", 10,	/* strlen(INT32_MAX) == 10 */
 	offsetof(struct pg_conn, keepalives_interval)},
 
-	{"keepalives_count", NULL, NULL, NULL,
-		"TCP-Keepalives-Count", "", 10, /* strlen(INT32_MAX) == 10 */
-	offsetof(struct pg_conn, keepalives_count)},
+	/* Kerberos authentication support specifying the service name */
+	{"krbsrvname", "PGKRBSRVNAME", PG_KRB_SRVNAM, NULL,
+		"Kerberos-service-name", "", 20,
+	offsetof(struct pg_conn, krbsrvname)},
 
-	{"tcp_user_timeout", NULL, NULL, NULL,
-		"TCP-User-Timeout", "", 10, /* strlen(INT32_MAX) == 10 */
-	offsetof(struct pg_conn, pgtcp_user_timeout)},
+	{"options", "PGOPTIONS", DefaultOption, NULL,
+		"Backend-Options", "", 40,
+	offsetof(struct pg_conn, pgoptions)},
+
+	{"password", "PGPASSWORD", NULL, NULL,
+		"Database-Password", "*", 20,
+	offsetof(struct pg_conn, pgpass)},
+
+	{"passfile", "PGPASSFILE", NULL, NULL,
+		"Database-Password-File", "", 64,
+	offsetof(struct pg_conn, pgpassfile)},
+
+	{"port", "PGPORT", DEF_PGPORT_STR, NULL,
+		"Database-Port", "", 6,
+	offsetof(struct pg_conn, pgport)},
+
+	{"replication", NULL, NULL, NULL,
+		"Replication", "D", 5,
+	offsetof(struct pg_conn, replication)},
+
+	{"requirepeer", "PGREQUIREPEER", NULL, NULL,
+		"Require-Peer", "", 10,
+	offsetof(struct pg_conn, requirepeer)},
+
+	{"service", "PGSERVICE", NULL, NULL,
+	"Database-Service", "", 20, -1},
 
 	/*
 	 * ssl options are allowed even without client SSL support because the
@@ -267,22 +285,30 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
 	 * parameters have no effect on non-SSL connections, so there is no reason
 	 * to exclude them since none of them are mandatory.
 	 */
-	{"sslmode", "PGSSLMODE", DefaultSSLMode, NULL,
-		"SSL-Mode", "", 12,		/* sizeof("verify-full") == 12 */
-	offsetof(struct pg_conn, sslmode)},
+	{"sslcert", "PGSSLCERT", NULL, NULL,
+		"SSL-Client-Cert", "", 64,
+	offsetof(struct pg_conn, sslcert)},
 
 	{"sslcompression", "PGSSLCOMPRESSION", "0", NULL,
 		"SSL-Compression", "", 1,
 	offsetof(struct pg_conn, sslcompression)},
 
-	{"sslcert", "PGSSLCERT", NULL, NULL,
-		"SSL-Client-Cert", "", 64,
-	offsetof(struct pg_conn, sslcert)},
+	{"sslcrl", "PGSSLCRL", NULL, NULL,
+		"SSL-Revocation-List", "", 64,
+	offsetof(struct pg_conn, sslcrl)},
+
+	{"sslcrldir", "PGSSLCRLDIR", NULL, NULL,
+		"SSL-Revocation-List-Dir", "", 64,
+	offsetof(struct pg_conn, sslcrldir)},
 
 	{"sslkey", "PGSSLKEY", NULL, NULL,
 		"SSL-Client-Key", "", 64,
 	offsetof(struct pg_conn, sslkey)},
 
+	{"sslmode", "PGSSLMODE", DefaultSSLMode, NULL,
+		"SSL-Mode", "", 12,		/* sizeof("verify-full") == 12 */
+	offsetof(struct pg_conn, sslmode)},
+
 	{"sslpassword", NULL, NULL, NULL,
 		"SSL-Client-Key-Password", "*", 20,
 	offsetof(struct pg_conn, sslpassword)},
@@ -291,56 +317,31 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
 		"SSL-Root-Certificate", "", 64,
 	offsetof(struct pg_conn, sslrootcert)},
 
-	{"sslcrl", "PGSSLCRL", NULL, NULL,
-		"SSL-Revocation-List", "", 64,
-	offsetof(struct pg_conn, sslcrl)},
-
-	{"sslcrldir", "PGSSLCRLDIR", NULL, NULL,
-		"SSL-Revocation-List-Dir", "", 64,
-	offsetof(struct pg_conn, sslcrldir)},
-
 	{"sslsni", "PGSSLSNI", "1", NULL,
 		"SSL-SNI", "", 1,
 	offsetof(struct pg_conn, sslsni)},
 
-	{"requirepeer", "PGREQUIREPEER", NULL, NULL,
-		"Require-Peer", "", 10,
-	offsetof(struct pg_conn, requirepeer)},
-
-	{"ssl_min_protocol_version", "PGSSLMINPROTOCOLVERSION", "TLSv1.2", NULL,
-		"SSL-Minimum-Protocol-Version", "", 8,	/* sizeof("TLSv1.x") == 8 */
-	offsetof(struct pg_conn, ssl_min_protocol_version)},
-
 	{"ssl_max_protocol_version", "PGSSLMAXPROTOCOLVERSION", NULL, NULL,
 		"SSL-Maximum-Protocol-Version", "", 8,	/* sizeof("TLSv1.x") == 8 */
 	offsetof(struct pg_conn, ssl_max_protocol_version)},
 
-	/*
-	 * As with SSL, all GSS options are exposed even in builds that don't have
-	 * support.
-	 */
-	{"gssencmode", "PGGSSENCMODE", DefaultGSSMode, NULL,
-		"GSSENC-Mode", "", 8,	/* sizeof("disable") == 8 */
-	offsetof(struct pg_conn, gssencmode)},
-
-	/* Kerberos and GSSAPI authentication support specifying the service name */
-	{"krbsrvname", "PGKRBSRVNAME", PG_KRB_SRVNAM, NULL,
-		"Kerberos-service-name", "", 20,
-	offsetof(struct pg_conn, krbsrvname)},
-
-	{"gsslib", "PGGSSLIB", NULL, NULL,
-		"GSS-library", "", 7,	/* sizeof("gssapi") == 7 */
-	offsetof(struct pg_conn, gsslib)},
-
-	{"replication", NULL, NULL, NULL,
-		"Replication", "D", 5,
-	offsetof(struct pg_conn, replication)},
+	{"ssl_min_protocol_version", "PGSSLMINPROTOCOLVERSION", "TLSv1.2", NULL,
+		"SSL-Minimum-Protocol-Version", "", 8,	/* sizeof("TLSv1.x") == 8 */
+	offsetof(struct pg_conn, ssl_min_protocol_version)},
 
 	{"target_session_attrs", "PGTARGETSESSIONATTRS",
 		DefaultTargetSessionAttrs, NULL,
 		"Target-Session-Attrs", "", 15, /* sizeof("prefer-standby") = 15 */
 	offsetof(struct pg_conn, target_session_attrs)},
 
+	{"tcp_user_timeout", NULL, NULL, NULL,
+		"TCP-User-Timeout", "", 10, /* strlen(INT32_MAX) == 10 */
+	offsetof(struct pg_conn, pgtcp_user_timeout)},
+
+	{"user", "PGUSER", NULL, NULL,
+		"Database-User", "", 20,
+	offsetof(struct pg_conn, pguser)},
+
 	/* Terminating entry --- MUST BE LAST */
 	{NULL, NULL, NULL, NULL,
 	NULL, NULL, 0}
diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out
index 33505352cc..d5ffc4f16a 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:  Valid options in this context are: authtype, connect_timeout, dbname, host, hostaddr, gsslib, options, port, requiressl, service, sslmode, tty
 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:  Valid options in this context are: authtype, connect_timeout, dbname, host, hostaddr, gsslib, options, port, requiressl, service, sslmode, tty
 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:  Valid options in this context are: password, 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:  Valid options in this context are: password, 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');
#4Ibrar Ahmed
ibrar.ahmad@gmail.com
In reply to: Peter Eisentraut (#1)
Re: postgres_fdw hint messages

On Thu, Aug 25, 2022 at 6:42 PM Peter Eisentraut <
peter.eisentraut@enterprisedb.com> wrote:

The postgres_fdw tests contain this (as amended by patch 0001):

ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw');
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

This annoys developers who are working on libpq connection options,
because any option added, removed, or changed causes this test to need
to be updated.

It's also questionable how useful this hint is in its current form,
considering how long it is and that the options are in an
implementation-dependent order.

Thanks Peter, for looking at that; this HINT message is growing over time.

In my opinion, we should hide the complete message in case of an invalid
option. But
try to show dependent options; for example, if someone specify "sslcrl" and
that option
require some more options, then show the HINT of that options.

Possible changes:

- Hide the hint from this particular test (done in the attached patches).

- Keep the hint, but maybe make it sorted?

- Remove all the hints like this from foreign data commands.

- Don't show the hint when there are more than N valid options.

- Do some kind of "did you mean" like we have for column names.

Thoughts?

--
Ibrar Ahmed

#5Robert Haas
robertmhaas@gmail.com
In reply to: Peter Eisentraut (#1)
Re: postgres_fdw hint messages

On Thu, Aug 25, 2022 at 9:42 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

The postgres_fdw tests contain this (as amended by patch 0001):

ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw');
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

This annoys developers who are working on libpq connection options,
because any option added, removed, or changed causes this test to need
to be updated.

It's also questionable how useful this hint is in its current form,
considering how long it is and that the options are in an
implementation-dependent order.

I think the place to list the legal options is in the documentation,
not the HINT.

--
Robert Haas
EDB: http://www.enterprisedb.com

#6Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#5)
Re: postgres_fdw hint messages

Robert Haas <robertmhaas@gmail.com> writes:

On Thu, Aug 25, 2022 at 9:42 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

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

This annoys developers who are working on libpq connection options,
because any option added, removed, or changed causes this test to need
to be updated.

It's also questionable how useful this hint is in its current form,
considering how long it is and that the options are in an
implementation-dependent order.

I think the place to list the legal options is in the documentation,
not the HINT.

I think listing them in a hint is reasonable as long as the hint doesn't
get longer than a line or two. This one is entirely out of hand, so
I agree with just dropping it.

Note that there is essentially identical code in dblink, file_fdw,
and backend/foreign/foreign.c. Do we want to nuke them all? Or
maybe make a policy decision to suppress such HINTs when there are
more than ~10 matches? (The latter policy would likely eventually
end by always suppressing everything...)

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.

regards, tom lane

#7Peter Eisentraut
peter.eisentraut@enterprisedb.com
In reply to: Peter Eisentraut (#1)
2 attachment(s)
Re: postgres_fdw hint messages

On 25.08.22 15:42, Peter Eisentraut wrote:

It's also questionable how useful this hint is in its current form,
considering how long it is and that the options are in an
implementation-dependent order.

Possible changes:

- Remove all the hints like this from foreign data commands.

It appears that there was a strong preference toward this solution, so
that's what I implemented in the updated patch set.

(Considering the hints that are removed in the tests cases, I don't
think this loses any value. What might be useful in practice instead is
something like "the option you specified on this foreign server should
actually be specified on a user mapping or foreign table", but this
would take a fair amount of code to cover a reasonable set of cases, so
I'll leave this as a future exercise.)

Attachments:

v2-0001-postgres_fdw-Remove-useless-DO-block-in-test.patchtext/plain; charset=UTF-8; name=v2-0001-postgres_fdw-Remove-useless-DO-block-in-test.patchDownload
From c079cf33ad8033875a6e18b9cff06e47330007ac Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 25 Aug 2022 15:31:10 +0200
Subject: [PATCH v2 1/2] postgres_fdw: Remove useless DO block in test

---
 contrib/postgres_fdw/expected/postgres_fdw.out | 8 +-------
 contrib/postgres_fdw/sql/postgres_fdw.sql      | 6 +-----
 2 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 7bf35602b0..a42c9720c3 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -9621,15 +9621,9 @@ ERROR:  password is required
 DETAIL:  Non-superusers must provide a password in the user mapping.
 -- If we add a password to the connstr it'll fail, because we don't allow passwords
 -- in connstrs only in user mappings.
-DO $d$
-    BEGIN
-        EXECUTE $$ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw')$$;
-    END;
-$d$;
+ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw');
 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
-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
 -- error because the password wasn't actually *used* when we run with trust auth.
 --
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 42735ae78a..63581a457d 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -2886,11 +2886,7 @@ CREATE FOREIGN TABLE pg_temp.ft1_nopw (
 -- If we add a password to the connstr it'll fail, because we don't allow passwords
 -- in connstrs only in user mappings.
 
-DO $d$
-    BEGIN
-        EXECUTE $$ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw')$$;
-    END;
-$d$;
+ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw');
 
 -- If we add a password for our user mapping instead, we should get a different
 -- error because the password wasn't actually *used* when we run with trust auth.
-- 
2.37.1

v2-0002-Remove-hints-from-FDW-s-invalid-option-errors.patchtext/plain; charset=UTF-8; name=v2-0002-Remove-hints-from-FDW-s-invalid-option-errors.patchDownload
From eef24034a75e674e7e4a929557906395eab7673f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Tue, 30 Aug 2022 09:08:53 +0200
Subject: [PATCH v2 2/2] Remove hints from FDW's invalid option errors

The FDWs provided in tree all had code to list valid options in an
error message hint if an invalid option was given to a
forein-data-related command.  These lists have in some cases gotten
extremely bulky, and in general they don't really help the user
correct their command.  (They might help in case of typos, but not if
there is a semantic misunderstanding about how the foreign-data setup
should be configured.)  So per discussion, remove these hints.

Discussion: https://www.postgresql.org/message-id/flat/b1f9f399-3a1a-b554-283f-4ae7f34608e2%40enterprisedb.com
---
 contrib/dblink/dblink.c                       | 24 +------------------
 contrib/dblink/expected/dblink.out            |  2 --
 contrib/file_fdw/expected/file_fdw.out        |  8 -------
 contrib/file_fdw/file_fdw.c                   | 23 +-----------------
 .../postgres_fdw/expected/postgres_fdw.out    |  3 ---
 contrib/postgres_fdw/option.c                 | 23 +-----------------
 src/backend/foreign/foreign.c                 | 19 +--------------
 src/test/regress/expected/foreign_data.out    |  5 ----
 8 files changed, 4 insertions(+), 103 deletions(-)

diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index e323fdd0e6..d50a4bf240 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -2005,31 +2005,9 @@ dblink_fdw_validator(PG_FUNCTION_ARGS)
 		DefElem    *def = (DefElem *) lfirst(cell);
 
 		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;
-
-			initStringInfo(&buf);
-			for (opt = options; opt->keyword; opt++)
-			{
-				if (is_valid_dblink_option(options, opt->keyword, context))
-					appendStringInfo(&buf, "%s%s",
-									 (buf.len > 0) ? ", " : "",
-									 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)
-					 : errhint("There are no valid options in this context.")));
-		}
+					 errmsg("invalid option \"%s\"", def->defname)));
 	}
 
 	PG_RETURN_VOID();
diff --git a/contrib/dblink/expected/dblink.out b/contrib/dblink/expected/dblink.out
index c7bde6ad07..a6f41a746c 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;
@@ -938,7 +937,6 @@ DROP SERVER fdtest;
 -- should fail
 ALTER FOREIGN DATA WRAPPER dblink_fdw OPTIONS (nonexistent 'fdw');
 ERROR:  invalid option "nonexistent"
-HINT:  There are no valid options in this context.
 -- test asynchronous notifications
 SELECT dblink_connect(connection_parameters());
  dblink_connect 
diff --git a/contrib/file_fdw/expected/file_fdw.out b/contrib/file_fdw/expected/file_fdw.out
index 261af1a8b5..5cab3fa307 100644
--- a/contrib/file_fdw/expected/file_fdw.out
+++ b/contrib/file_fdw/expected/file_fdw.out
@@ -157,29 +157,21 @@ ALTER FOREIGN TABLE text_csv ALTER COLUMN word3 OPTIONS (force_not_null 'true');
 -- force_not_null is not allowed to be specified at any foreign object level:
 ALTER FOREIGN DATA WRAPPER file_fdw OPTIONS (ADD force_not_null '*'); -- ERROR
 ERROR:  invalid option "force_not_null"
-HINT:  There are no valid options in this context.
 ALTER SERVER file_server OPTIONS (ADD force_not_null '*'); -- ERROR
 ERROR:  invalid option "force_not_null"
-HINT:  There are no valid options in this context.
 CREATE USER MAPPING FOR public SERVER file_server OPTIONS (force_not_null '*'); -- ERROR
 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"
-HINT:  There are no valid options in this context.
 ALTER SERVER file_server OPTIONS (ADD force_null '*'); -- ERROR
 ERROR:  invalid option "force_null"
-HINT:  There are no valid options in this context.
 CREATE USER MAPPING FOR public SERVER file_server OPTIONS (force_null '*'); -- ERROR
 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..c5153346aa 100644
--- a/contrib/file_fdw/file_fdw.c
+++ b/contrib/file_fdw/file_fdw.c
@@ -212,30 +212,9 @@ file_fdw_validator(PG_FUNCTION_ARGS)
 		DefElem    *def = (DefElem *) lfirst(cell);
 
 		if (!is_valid_option(def->defname, catalog))
-		{
-			const struct FileFdwOption *opt;
-			StringInfoData buf;
-
-			/*
-			 * 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++)
-			{
-				if (catalog == opt->optcontext)
-					appendStringInfo(&buf, "%s%s", (buf.len > 0) ? ", " : "",
-									 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)
-					 : errhint("There are no valid options in this context.")));
-		}
+					 errmsg("invalid option \"%s\"", def->defname)));
 
 		/*
 		 * Separate out filename, program, and column-specific options, since
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index a42c9720c3..f8e0163b4d 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');
@@ -9623,7 +9622,6 @@ DETAIL:  Non-superusers must provide a password in the user mapping.
 -- in connstrs only in user mappings.
 ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw');
 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
 -- If we add a password for our user mapping instead, we should get a different
 -- error because the password wasn't actually *used* when we run with trust auth.
 --
@@ -11237,7 +11235,6 @@ ERROR:  invalid value for integer option "batch_size": 100$%$#$#
 -- No option is allowed to be specified at foreign data wrapper level
 ALTER FOREIGN DATA WRAPPER postgres_fdw OPTIONS (nonexistent 'fdw');
 ERROR:  invalid option "nonexistent"
-HINT:  There are no valid options in this context.
 -- ===================================================================
 -- test postgres_fdw.application_name GUC
 -- ===================================================================
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 95dde056eb..56d443e4f9 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -87,30 +87,9 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
 		DefElem    *def = (DefElem *) lfirst(cell);
 
 		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;
-
-			initStringInfo(&buf);
-			for (opt = postgres_fdw_options; opt->keyword; opt++)
-			{
-				if (catalog == opt->optcontext)
-					appendStringInfo(&buf, "%s%s", (buf.len > 0) ? ", " : "",
-									 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)
-					 : errhint("There are no valid options in this context.")));
-		}
+					 errmsg("invalid option \"%s\"", def->defname)));
 
 		/*
 		 * Validate option value, when we can do so without any context.
diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c
index cf222fc3e9..194dbea5c4 100644
--- a/src/backend/foreign/foreign.c
+++ b/src/backend/foreign/foreign.c
@@ -620,26 +620,9 @@ 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++)
-				if (catalog == opt->optcontext)
-					appendStringInfo(&buf, "%s%s", (buf.len > 0) ? ", " : "",
-									 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)
-					 : errhint("There are no valid options in this context.")));
+					 errmsg("invalid option \"%s\"", def->defname)));
 
 			PG_RETURN_BOOL(false);
 		}
diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out
index 33505352cc..ae0c7a5c18 100644
--- a/src/test/regress/expected/foreign_data.out
+++ b/src/test/regress/expected/foreign_data.out
@@ -110,7 +110,6 @@ DROP FOREIGN DATA WRAPPER test_fdw;
 -- ALTER FOREIGN DATA WRAPPER
 ALTER FOREIGN DATA WRAPPER foo OPTIONS (nonexistent 'fdw');         -- ERROR
 ERROR:  invalid option "nonexistent"
-HINT:  There are no valid options in this context.
 ALTER FOREIGN DATA WRAPPER foo;                             -- ERROR
 ERROR:  syntax error at or near ";"
 LINE 1: ALTER FOREIGN DATA WRAPPER foo;
@@ -329,7 +328,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 +438,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 +594,6 @@ 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
 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 +632,6 @@ 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
 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.37.1

#8Nathan Bossart
nathandbossart@gmail.com
In reply to: Tom Lane (#6)
1 attachment(s)
Re: postgres_fdw hint messages

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

Attachments:

did_you_mean.patchtext/x-diff; charset=us-asciiDownload
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');
#9Tom Lane
tgl@sss.pgh.pa.us
In reply to: Nathan Bossart (#8)
Re: postgres_fdw hint messages

Nathan Bossart <nathandbossart@gmail.com> writes:

On Fri, Aug 26, 2022 at 12:35:38PM -0400, Tom Lane wrote:

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.

Seems reasonable to me, but

(1) there probably needs to be some threshold of closeness, so we don't
offer "foobar" when the user wrote "huh"

(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.

regards, tom lane

#10Nathan Bossart
nathandbossart@gmail.com
In reply to: Tom Lane (#9)
Re: postgres_fdw hint messages

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.

I'm also considering checking whether the user-provided string is longer
than MAX_LEVENSHTEIN_STRLEN so that we can avoid the ERROR from
varstr_levenshtein(). Or perhaps varstr_levenshtein() could indicate that
the string is too long without ERROR-ing (e.g., by returning -1). If the
user-provided string is too long, we'd just omit the hint.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#11Nathan Bossart
nathandbossart@gmail.com
In reply to: Nathan Bossart (#10)
1 attachment(s)
Re: postgres_fdw hint messages

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

Attachments:

v2-0001-Adjust-assorted-hint-messages-that-list-all-valid.patchtext/x-diff; charset=us-asciiDownload
From c3c7793daf7e6b225605a44a1faead2f7678bca3 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandbossart@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

#12Michael Paquier
michael@paquier.xyz
In reply to: Nathan Bossart (#11)
Re: postgres_fdw hint messages

On Fri, Sep 02, 2022 at 02:26:09PM -0700, Nathan Bossart wrote:

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.

Hmm. FWIW I would tend toward simplifying all this code and just drop
all the hints rather than increasing the dependency to more
levenshtein calculations in those error code paths, which is what
Peter E has posted.
--
Michael

#13Tom Lane
tgl@sss.pgh.pa.us
In reply to: Michael Paquier (#12)
Re: postgres_fdw hint messages

Michael Paquier <michael@paquier.xyz> writes:

On Fri, Sep 02, 2022 at 02:26:09PM -0700, Nathan Bossart wrote:

* 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.

Hmm. FWIW I would tend toward simplifying all this code and just drop
all the hints rather than increasing the dependency to more
levenshtein calculations in those error code paths, which is what
Peter E has posted.

Personally I'm not a huge fan of this style of hint either. However,
people seem to like the ones for misspelled column names, so I'm
betting there will be a majority in favor of this one too.

I think the distance limit of 5 is too loose though. I see that
it accommodates examples like "passfile" for "password", which
seems great at first glance; but it also allows fundamentally
silly suggestions like "user" for "server" or "host" for "foo".
We'd need something smarter than Levenshtein if we want to offer
"passfile" for "password" without looking stupid on a whole lot
of other cases --- those words seem close, but they are close
semantically not textually.

regards, tom lane

#14Nathan Bossart
nathandbossart@gmail.com
In reply to: Tom Lane (#13)
1 attachment(s)
Re: postgres_fdw hint messages

On Fri, Sep 02, 2022 at 10:06:54PM -0400, Tom Lane wrote:

I think the distance limit of 5 is too loose though. I see that
it accommodates examples like "passfile" for "password", which
seems great at first glance; but it also allows fundamentally
silly suggestions like "user" for "server" or "host" for "foo".
We'd need something smarter than Levenshtein if we want to offer
"passfile" for "password" without looking stupid on a whole lot
of other cases --- those words seem close, but they are close
semantically not textually.

Yeah, it's really only useful for simple misspellings, but IMO even that is
rather handy.

I noticed that the parse_relation.c stuff excludes matches where more than
half the characters are different, so I added that here and lowered the
distance limit to 4. This seems to prevent the silly suggestions (e.g.,
"host" for "foo") while retaining the more believable ones (e.g.,
"passfile" for "password"), at least for the small set of examples covered
in the tests.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Attachments:

v3-0001-Adjust-assorted-hint-messages-that-list-all-valid.patchtext/x-diff; charset=us-asciiDownload
From 1ce0024357343cf4c1436cd74ed20e304d673762 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandbossart@gmail.com>
Date: Fri, 2 Sep 2022 14:03:26 -0700
Subject: [PATCH v3 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            |  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

#15Peter Eisentraut
peter.eisentraut@enterprisedb.com
In reply to: Nathan Bossart (#14)
Re: postgres_fdw hint messages

On 03.09.22 06:30, Nathan Bossart wrote:

On Fri, Sep 02, 2022 at 10:06:54PM -0400, Tom Lane wrote:

I think the distance limit of 5 is too loose though. I see that
it accommodates examples like "passfile" for "password", which
seems great at first glance; but it also allows fundamentally
silly suggestions like "user" for "server" or "host" for "foo".
We'd need something smarter than Levenshtein if we want to offer
"passfile" for "password" without looking stupid on a whole lot
of other cases --- those words seem close, but they are close
semantically not textually.

Yeah, it's really only useful for simple misspellings, but IMO even that is
rather handy.

I noticed that the parse_relation.c stuff excludes matches where more than
half the characters are different, so I added that here and lowered the
distance limit to 4. This seems to prevent the silly suggestions (e.g.,
"host" for "foo") while retaining the more believable ones (e.g.,
"passfile" for "password"), at least for the small set of examples covered
in the tests.

I think this code is compact enough and the hints it produces are
reasonable, so I think we could go with it.

I notice that for column misspellings, the hint is phrased "Perhaps you
meant X." whereas here we have "Did you mean X?". Let's make that uniform.

#16Nathan Bossart
nathandbossart@gmail.com
In reply to: Peter Eisentraut (#15)
1 attachment(s)
Re: postgres_fdw hint messages

On Tue, Sep 13, 2022 at 08:32:43AM +0200, Peter Eisentraut wrote:

I notice that for column misspellings, the hint is phrased "Perhaps you
meant X." whereas here we have "Did you mean X?". Let's make that uniform.

Good point. I attached a new version of the patch that uses the column
phrasing. I wasn't sure whether "reference" was the right word to use in
this context, but I used it for now for consistency with the column hints.
I think "specify" or "use" would work as well.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Attachments:

v4-0001-Adjust-assorted-hint-messages-that-list-all-valid.patchtext/x-diff; charset=us-asciiDownload
From f9e5d136214e2b55c0bcf0b7b8ec2485aab0e7ba Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandbossart@gmail.com>
Date: Fri, 2 Sep 2022 14:03:26 -0700
Subject: [PATCH v4 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                       | 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

#17Peter Eisentraut
peter.eisentraut@enterprisedb.com
In reply to: Nathan Bossart (#16)
Re: postgres_fdw hint messages

On 13.09.22 21:02, Nathan Bossart wrote:

On Tue, Sep 13, 2022 at 08:32:43AM +0200, Peter Eisentraut wrote:

I notice that for column misspellings, the hint is phrased "Perhaps you
meant X." whereas here we have "Did you mean X?". Let's make that uniform.

Good point. I attached a new version of the patch that uses the column
phrasing. I wasn't sure whether "reference" was the right word to use in
this context, but I used it for now for consistency with the column hints.
I think "specify" or "use" would work as well.

I don't think we need a verb there at all. I committed it without a verb.

#18Nathan Bossart
nathandbossart@gmail.com
In reply to: Peter Eisentraut (#17)
Re: postgres_fdw hint messages

On Fri, Sep 16, 2022 at 03:54:53PM +0200, Peter Eisentraut wrote:

I don't think we need a verb there at all. I committed it without a verb.

Thanks!

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com