From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
To: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: postgres_fdw hint messages
Date: Tue, 30 Aug 2022 09:20:53 +0200
Message-ID: <6da49de5-9334-23ea-1c50-36eda347dbee@enterprisedb.com> (raw)
In-Reply-To: <b1f9f399-3a1a-b554-283f-4ae7f34608e2@enterprisedb.com>
References: <b1f9f399-3a1a-b554-283f-4ae7f34608e2@enterprisedb.com>
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:download all (2 files, .tar.gz)v2-0001-postgres_fdw-Remove-useless-DO-block-in-test.patch (text/plain, 2.7K) download
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.outindex 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.sqlindex 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.patch (text/plain, 13.8K) download
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.cindex 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.outindex 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.outindex 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.cindex 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.outindex 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.cindex 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.cindex 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.outindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: peter.eisentraut@enterprisedb.com
Subject: Re: postgres_fdw hint messages
In-Reply-To: <6da49de5-9334-23ea-1c50-36eda347dbee@enterprisedb.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox