On 2021/10/13 14:00, Bharath Rupireddy wrote:
On Tue, Oct 12, 2021 at 11:11 PM Fujii Masao
<masao.fu...@oss.nttdata.com> wrote:
BTW, I found file_fdw.c, dblink.c, postgres_fdw/option.c and foreign.c
use different error codes for the same error message as follows.
They should use the same error code? If yes, ISTM that
ERRCODE_FDW_INVALID_OPTION_NAME is better because
the error message is "invalid option ...".
- ERRCODE_FDW_INVALID_OPTION_NAME (file_fdw.c)
- ERRCODE_FDW_OPTION_NAME_NOT_FOUND (dblink.c)
- ERRCODE_FDW_INVALID_OPTION_NAME (postgres_fdw/option.c)
- ERRCODE_SYNTAX_ERROR (foreign.c)
Good catch. ERRCODE_FDW_INVALID_OPTION_NAME seems reasonable to me. I
think we can remove the error code ERRCODE_FDW_OPTION_NAME_NOT_FOUND
(it is being used only by dblink.c), instead use
ERRCODE_FDW_INVALID_OPTION_NAME for all option parsing and
validations.
Alvaro told me the difference of those error codes as follows at [1].
This makes me think that ERRCODE_FDW_OPTION_NAME_NOT_FOUND
is more proper for the error message. Thought?
-----------------------
in SQL/MED compare GetServerOptByName: INVALID OPTION NAME is used
when the buffer length does not match the option name length;
OPTION NAME NOT FOUND is used when an option of that name is not found
-----------------------
[1] https://twitter.com/alvherre/status/1447991206286348302
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
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..91cbd744a9 100644
--- 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 3e96b98571..7a71817d65 100644
--- 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/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index 2c2f149fb0..f63c1ffa63 100644
--- a/contrib/file_fdw/file_fdw.c
+++ b/contrib/file_fdw/file_fdw.c
@@ -229,7 +229,7 @@ file_fdw_validator(PG_FUNCTION_ARGS)
}
ereport(ERROR,
-
(errcode(ERRCODE_FDW_INVALID_OPTION_NAME),
+
(errcode(ERRCODE_FDW_OPTION_NAME_NOT_FOUND),
errmsg("invalid option \"%s\"",
def->defname),
buf.len > 0
? errhint("Valid options in this
context are: %s",
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out
b/contrib/postgres_fdw/expected/postgres_fdw.out
index 44c4367b8f..fd141a0fa5 100644
--- 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 5bb1af4084..68538f89db 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -105,10 +105,12 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
}
ereport(ERROR,
-
(errcode(ERRCODE_FDW_INVALID_OPTION_NAME),
+
(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/postgres_fdw/sql/postgres_fdw.sql
b/contrib/postgres_fdw/sql/postgres_fdw.sql
index e7b869f8ce..43c30d492d 100644
--- 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 5564dc3a1e..3d8c5e1036 100644
--- a/src/backend/foreign/foreign.c
+++ b/src/backend/foreign/foreign.c
@@ -668,10 +668,12 @@ postgresql_fdw_validator(PG_FUNCTION_ARGS)
opt->optname);
ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_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.")));
PG_RETURN_BOOL(false);
}
diff --git a/src/test/regress/expected/foreign_data.out
b/src/test/regress/expected/foreign_data.out
index 426080ae39..a6a68d1fa2 100644
--- 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 73f9f621d8..a65f4ffdca 100644
--- 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;
