Hi
Thank you for your comments.
It seems like the change proposed for postgres_fdw_validator is
similar to what
file_fdw is doing in file_fdw_validator. I think we also need to do
the same
change in dblink_fdw_validator and postgresql_fdw_validator as well.
Agreed.
While on this, it's better to add test cases for the error message
"There are no valid options in this context." for all the three fdws
i.e. file_fdw, postgres_fdw and dblink_fdw may be in their respective
contrib modules test files, and for postgresql_fdw_validator in
src/test/regress/sql/foreign_data.sql.
+1.
I made new patch based on those comments.
Regards,
Kosei Masumura
2021-10-08 17:13 に Daniel Gustafsson さんは書きました:
On 8 Oct 2021, at 09:38, Bharath Rupireddy
<bharath.rupireddyforpostg...@gmail.com> wrote:
On Fri, Oct 8, 2021 at 12:48 PM bt21masumurak
<bt21masumu...@oss.nttdata.com> wrote:
Hi
When 'ALTER FOREIGN DATA WRAPPER OPTIONS' is executed against
postgres_fdw, the HINT message is printed as shown below, even though
there are no valid options in this context.
=# ALTER FOREIGN DATA WRAPPER postgres_fdw OPTIONS (format 'csv');
ERROR: invalid option "format"
HINT: Valid options in this context are:
I made a patch for this problem.
Good catch.
+1
It seems like the change proposed for postgres_fdw_validator is
similar to what
file_fdw is doing in file_fdw_validator. I think we also need to do
the same
change in dblink_fdw_validator and postgresql_fdw_validator as well.
Agreed.
While on this, it's better to add test cases for the error message
"There are no valid options in this context." for all the three fdws
i.e. file_fdw, postgres_fdw and dblink_fdw may be in their respective
contrib modules test files, and for postgresql_fdw_validator in
src/test/regress/sql/foreign_data.sql.
+1.
--
Daniel Gustafsson https://vmware.com/
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 3a0beaa88e..c8b0b4ff3f 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -2054,8 +2054,10 @@ dblink_fdw_validator(PG_FUNCTION_ARGS)
ereport(ERROR,
(errcode(ERRCODE_FDW_OPTION_NAME_NOT_FOUND),
errmsg("invalid option \"%s\"", def->defname),
- errhint("Valid options in this context are: %s",
- buf.data)));
+ buf.len > 0
+ ? errhint("Valid options in this context are: %s",
+ buf.data)
+ : errhint("There are no valid options in this context.")));
}
}
diff --git a/contrib/dblink/expected/dblink.out b/contrib/dblink/expected/dblink.out
index 6516d4f131..48a4d5ea99 100644
--- a/contrib/dblink/expected/dblink.out
+++ b/contrib/dblink/expected/dblink.out
@@ -1,4 +1,8 @@
CREATE EXTENSION dblink;
+-- HINT test
+ALTER FOREIGN DATA WRAPPER dblink_fdw OPTIONS (format 'csv');
+ERROR: invalid option "format"
+HINT: There are no valid options in this context.
-- want context for notices
\set SHOW_CONTEXT always
CREATE TABLE foo(f1 int, f2 text, f3 text[], primary key (f1,f2));
diff --git a/contrib/dblink/sql/dblink.sql b/contrib/dblink/sql/dblink.sql
index 3e96b98571..fa0a307c73 100644
--- a/contrib/dblink/sql/dblink.sql
+++ b/contrib/dblink/sql/dblink.sql
@@ -1,5 +1,6 @@
CREATE EXTENSION dblink;
-
+-- HINT test
+ALTER FOREIGN DATA WRAPPER dblink_fdw OPTIONS (format 'csv');
-- want context for notices
\set SHOW_CONTEXT always
diff --git a/contrib/file_fdw/input/file_fdw.source b/contrib/file_fdw/input/file_fdw.source
index 45b728eeb3..d634f70ebf 100644
--- a/contrib/file_fdw/input/file_fdw.source
+++ b/contrib/file_fdw/input/file_fdw.source
@@ -14,6 +14,9 @@ CREATE ROLE regress_no_priv_user LOGIN; -- has priv but no user
-- Install file_fdw
CREATE EXTENSION file_fdw;
+--HINT test
+ALTER FOREIGN DATA WRAPPER file_fdw OPTIONS (format 'csv');
+
-- regress_file_fdw_superuser owns fdw-related objects
SET ROLE regress_file_fdw_superuser;
CREATE SERVER file_server FOREIGN DATA WRAPPER file_fdw;
diff --git a/contrib/file_fdw/output/file_fdw.source b/contrib/file_fdw/output/file_fdw.source
index 52b4d5f1df..7bcd8a1e9b 100644
--- a/contrib/file_fdw/output/file_fdw.source
+++ b/contrib/file_fdw/output/file_fdw.source
@@ -10,6 +10,10 @@ CREATE ROLE regress_file_fdw_user LOGIN; -- has priv and user map
CREATE ROLE regress_no_priv_user LOGIN; -- has priv but no user mapping
-- Install file_fdw
CREATE EXTENSION file_fdw;
+--HINT test
+ALTER FOREIGN DATA WRAPPER file_fdw OPTIONS (format 'csv');
+ERROR: invalid option "format"
+HINT: There are no valid options in this context.
-- regress_file_fdw_superuser owns fdw-related objects
SET ROLE regress_file_fdw_superuser;
CREATE SERVER file_server FOREIGN DATA WRAPPER file_fdw;
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index c7b7db8065..857ed56d62 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -2,6 +2,10 @@
-- create FDW objects
-- ===================================================================
CREATE EXTENSION postgres_fdw;
+-- HINT test
+ALTER FOREIGN DATA WRAPPER postgres_fdw OPTIONS (format 'csv');
+ERROR: invalid option "format"
+HINT: There are no valid options in this context.
CREATE SERVER testserver1 FOREIGN DATA WRAPPER postgres_fdw;
DO $d$
BEGIN
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 5bb1af4084..19edf98360 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -107,8 +107,10 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
ereport(ERROR,
(errcode(ERRCODE_FDW_INVALID_OPTION_NAME),
errmsg("invalid option \"%s\"", def->defname),
- errhint("Valid options in this context are: %s",
- buf.data)));
+ buf.len > 0
+ ? errhint("Valid options in this context are: %s",
+ buf.data)
+ : errhint("There are no valid options in this context.")));
}
/*
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 38f4a7837f..46c26938ce 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -3,6 +3,8 @@
-- ===================================================================
CREATE EXTENSION postgres_fdw;
+-- HINT test
+ALTER FOREIGN DATA WRAPPER postgres_fdw OPTIONS (format 'csv');
CREATE SERVER testserver1 FOREIGN DATA WRAPPER postgres_fdw;
DO $d$
diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c
index 6b9533fcf0..e07cc57431 100644
--- a/src/backend/foreign/foreign.c
+++ b/src/backend/foreign/foreign.c
@@ -670,8 +670,10 @@ postgresql_fdw_validator(PG_FUNCTION_ARGS)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("invalid option \"%s\"", def->defname),
- errhint("HOGEHOGEValid options in this context are: %s",
- buf.data)));
+ buf.len > 0
+ ? errhint("Valid options in this context are: %s",
+ buf.data)
+ : 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 426080ae39..1a1e8a6478 100644
--- a/src/test/regress/expected/foreign_data.out
+++ b/src/test/regress/expected/foreign_data.out
@@ -16,6 +16,9 @@ CREATE ROLE regress_unprivileged_role;
CREATE FOREIGN DATA WRAPPER dummy;
COMMENT ON FOREIGN DATA WRAPPER dummy IS 'useless';
CREATE FOREIGN DATA WRAPPER postgresql VALIDATOR postgresql_fdw_validator;
+ALTER FOREIGN DATA WRAPPER postgresql OPTIONS (format 'csv');
+ERROR: invalid option "format"
+HINT: There are no valid options in this context.
-- At this point we should have 2 built-in wrappers and no servers.
SELECT fdwname, fdwhandler::regproc, fdwvalidator::regproc, fdwoptions FROM pg_foreign_data_wrapper ORDER BY 1, 2, 3;
fdwname | fdwhandler | fdwvalidator | fdwoptions
diff --git a/src/test/regress/sql/foreign_data.sql b/src/test/regress/sql/foreign_data.sql
index 73f9f621d8..95d9d575dc 100644
--- a/src/test/regress/sql/foreign_data.sql
+++ b/src/test/regress/sql/foreign_data.sql
@@ -24,6 +24,8 @@ CREATE FOREIGN DATA WRAPPER dummy;
COMMENT ON FOREIGN DATA WRAPPER dummy IS 'useless';
CREATE FOREIGN DATA WRAPPER postgresql VALIDATOR postgresql_fdw_validator;
+ALTER FOREIGN DATA WRAPPER postgresql OPTIONS (format 'csv');
+
-- At this point we should have 2 built-in wrappers and no servers.
SELECT fdwname, fdwhandler::regproc, fdwvalidator::regproc, fdwoptions FROM pg_foreign_data_wrapper ORDER BY 1, 2, 3;
SELECT srvname, srvoptions FROM pg_foreign_server;
