From eef24034a75e674e7e4a929557906395eab7673f Mon Sep 17 00:00:00 2001 From: Peter Eisentraut 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