Improve HINT message that FDW reports when there are no valid options.

The foreign data wrapper's validator function provides a HINT message with
list of valid options for the object specified in CREATE or ALTER command,
when the option given in the command is invalid. Previously
postgresql_fdw_validator() and the validator functions for postgres_fdw and
dblink_fdw worked in that way even there were no valid options in the object,
which could lead to the HINT message with empty list (because there were
no valid options). For example, ALTER FOREIGN DATA WRAPPER postgres_fdw
OPTIONS (format 'csv') reported the following ERROR and HINT messages.
This behavior was confusing.

    ERROR: invalid option "format"
    HINT: Valid options in this context are:

There is no such issue in file_fdw. The validator function for file_fdw
reports the HINT message "There are no valid options in this context."
instead in that case.

This commit improves postgresql_fdw_validator() and the validator functions
for postgres_fdw and dblink_fdw so that they do likewise. For example,
this change causes the above ALTER FOREIGN DATA WRAPPER command to
report the following messages.

    ERROR:  invalid option "nonexistent"
    HINT:  There are no valid options in this context.

Author: Kosei Masumura
Reviewed-by: Bharath Rupireddy, Fujii Masao
Discussion: https://postgr.es/m/557d06cebe19081bfcc83ee2affc98d3@oss.nttdata.com

diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 3a0beaa88e7b55aeabefc735d06cde6f77c9ab58..c8b0b4ff3fafda3cdb6193e209797a682c556435 100644 (file)
--- 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 6516d4f13133e3be2d88303b63e4cb0340accc86..91cbd744a996020c44a3e98fa1c7fd224008ef98 100644 (file)
--- a/contrib/dblink/expected/dblink.out
+++ b/contrib/dblink/expected/dblink.out
@@ -917,6 +917,10 @@ REVOKE EXECUTE ON FUNCTION dblink_connect_u(text, text) FROM regress_dblink_user
 DROP USER regress_dblink_user;
 DROP USER MAPPING FOR public SERVER fdtest;
 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/dblink/sql/dblink.sql b/contrib/dblink/sql/dblink.sql
index 3e96b9857128a7429e39ac138f253cbb03931d7d..7a71817d65b97fc549b4b54f531e6692f8273d58 100644 (file)
--- a/contrib/dblink/sql/dblink.sql
+++ b/contrib/dblink/sql/dblink.sql
@@ -463,6 +463,9 @@ DROP USER regress_dblink_user;
 DROP USER MAPPING FOR public SERVER fdtest;
 DROP SERVER fdtest;
 
+-- should fail
+ALTER FOREIGN DATA WRAPPER dblink_fdw OPTIONS (nonexistent 'fdw');
+
 -- test asynchronous notifications
 SELECT dblink_connect(connection_parameters());
 

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 44c4367b8f9bc399ed7575938bb3e0ab95ba0a63..fd141a0fa5c997aeba05cb890349954fa59a8eaa 100644 (file)
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -10746,7 +10746,7 @@ DROP TABLE join_tbl;
 ALTER SERVER loopback OPTIONS (DROP async_capable);
 ALTER SERVER loopback2 OPTIONS (DROP async_capable);
 -- ===================================================================
--- test invalid server and foreign table options
+-- test invalid server, foreign table and foreign data wrapper options
 -- ===================================================================
 -- Invalid fdw_startup_cost option
 CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw
@@ -10764,3 +10764,7 @@ ERROR:  invalid value for integer option "fetch_size": 100$%$#$#
 CREATE FOREIGN TABLE inv_bsz (c1 int )
        SERVER loopback OPTIONS (batch_size '100$%$#$#');
 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.

diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 5bb1af4084a9ecdc17e37e2773eacd7a92e20696..48c7417e6ecbe528226b420d182dee5f374e6b5a 100644 (file)
--- 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 e7b869f8ceac91e51c1cce6c952d491f5cfc0e7b..43c30d492da4b5b3d1561c816d694238599fcff0 100644 (file)
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -3409,7 +3409,7 @@ ALTER SERVER loopback OPTIONS (DROP async_capable);
 ALTER SERVER loopback2 OPTIONS (DROP async_capable);
 
 -- ===================================================================
--- test invalid server and foreign table options
+-- test invalid server, foreign table and foreign data wrapper options
 -- ===================================================================
 -- Invalid fdw_startup_cost option
 CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw
@@ -3423,3 +3423,6 @@ CREATE FOREIGN TABLE inv_fsz (c1 int )
 -- Invalid batch_size option
 CREATE FOREIGN TABLE inv_bsz (c1 int )
        SERVER loopback OPTIONS (batch_size '100$%$#$#');
+
+-- No option is allowed to be specified at foreign data wrapper level
+ALTER FOREIGN DATA WRAPPER postgres_fdw OPTIONS (nonexistent 'fdw');

diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c
index 5564dc3a1e2857d7ae534fa1998248a637a5cad2..e07cc57431266ffb438b357a024b1a1cfe5cf800 100644 (file)
--- 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("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.")));
 
                        PG_RETURN_BOOL(false);
                }

diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out
index 426080ae39b4cd86f0847d08c74cb98f2b345ccd..a6a68d1fa2a722911289639b21da5c2c284523cc 100644 (file)
--- a/src/test/regress/expected/foreign_data.out
+++ b/src/test/regress/expected/foreign_data.out
@@ -100,6 +100,9 @@ LINE 1: ...GN DATA WRAPPER test_fdw HANDLER test_fdw_handler HANDLER in...
 CREATE FOREIGN DATA WRAPPER test_fdw HANDLER test_fdw_handler;
 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;

diff --git a/src/test/regress/sql/foreign_data.sql b/src/test/regress/sql/foreign_data.sql
index 73f9f621d8ff9d06dad556142ba86b2591848c5f..a65f4ffdca11facca42f3be3a8018b42230365b2 100644 (file)
--- a/src/test/regress/sql/foreign_data.sql
+++ b/src/test/regress/sql/foreign_data.sql
@@ -59,6 +59,8 @@ CREATE FOREIGN DATA WRAPPER test_fdw HANDLER test_fdw_handler;
 DROP FOREIGN DATA WRAPPER test_fdw;
 
 -- ALTER FOREIGN DATA WRAPPER
+ALTER FOREIGN DATA WRAPPER foo OPTIONS (nonexistent 'fdw');         -- ERROR
+
 ALTER FOREIGN DATA WRAPPER foo;                             -- ERROR
 ALTER FOREIGN DATA WRAPPER foo VALIDATOR bar;               -- ERROR
 ALTER FOREIGN DATA WRAPPER foo NO VALIDATOR;