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.)
From c079cf33ad8033875a6e18b9cff06e47330007ac Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <pe...@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
From eef24034a75e674e7e4a929557906395eab7673f Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <pe...@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
