From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: bt21masumurak <bt21masumurak@oss.nttdata.com>
Cc: Daniel Gustafsson <daniel@yesql.se>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Improve the HINT message of the ALTER command for postgres_fdw
Date: Mon, 25 Oct 2021 20:06:29 +0900
Message-ID: <29298fbf-6aec-7700-5cff-74e21d5d0e2e@oss.nttdata.com> (raw)
In-Reply-To: <CALj2ACXxepgwujDzOk3w93kSz5c6j79GYubE1ng1_-y1c2Jt7w@mail.gmail.com>
References: <557d06cebe19081bfcc83ee2affc98d3@oss.nttdata.com>
<CALj2ACUkZ9FWZA0XgjBeMX-M_Pg2Z=xhvFsy39X6699jDWr5xw@mail.gmail.com>
<250E9750-57EA-45BA-82A4-787CC61F7309@yesql.se>
<46864d7a8ca6c2faeb2f9a7a401d47c0@oss.nttdata.com>
<595194b4-20e1-58a5-4413-c0c7a1aee004@oss.nttdata.com>
<CALj2ACU7XhadSLpT1x3988RY5ryZRGqo+vwRVF+zmaedzmhi2A@mail.gmail.com>
<4dbda741-2e88-fd07-c2a6-396d776ccf20@oss.nttdata.com>
<CALj2ACU_hHxz=Jdfsx6-eRyN872bYfec_gtLb130Umef=oXhQw@mail.gmail.com>
<33141684-a9bd-aea8-e05c-2826411c70fd@oss.nttdata.com>
<CALj2ACXxepgwujDzOk3w93kSz5c6j79GYubE1ng1_-y1c2Jt7w@mail.gmail.com>
On 2021/10/25 16:44, Bharath Rupireddy wrote:
> Yeah, let's focus on fixing the hint message here and the
> alter_foreign_data_wrapper_options_v3.patch LGTM.
Thanks! But since v3 changed the error codes, I got rid of those
changes and made v4 patch. Attached.
> Why didn't we have a test case for file_fdw? It looks like the
> file_fdw contrib module doesn't have any test cases in its sql
> directory. I'm not sure if the module code is being covered in
> someother tests.
You can see the regression test for file_fdw,
in contrib/file_fdw/input and output directories.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Attachments:alter_foreign_data_wrapper_options_v4.patch (text/plain, 6.6K) downloaddiff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.cindex 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.outindex 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.sqlindex 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/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.outindex 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.cindex 5bb1af4084..48c7417e6e 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.sqlindex 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.cindex 5564dc3a1e..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("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.outindex 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.sqlindex 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;
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: masao.fujii@oss.nttdata.com, bharath.rupireddyforpostgres@gmail.com, bt21masumurak@oss.nttdata.com, daniel@yesql.se
Subject: Re: Improve the HINT message of the ALTER command for postgres_fdw
In-Reply-To: <29298fbf-6aec-7700-5cff-74e21d5d0e2e@oss.nttdata.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