pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedImprove the HINT message of the ALTER command for postgres_fdw
14+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2021-10-26 5fedf7417b Improve HINT message that FDW reports when there are no valid options.
* Improve the HINT message of the ALTER command for postgres_fdw
@ 2021-10-08 07:18 bt21masumurak <bt21masumurak@oss.nttdata.com>
0 siblings, 1 reply; 14+ messages in thread
From: bt21masumurak @ 2021-10-08 07:18 UTC (permalink / raw)
To: pgsql-hackers
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.
regards,
Kosei Masumura
Attachments:
diff.patch (text/x-diff, 740B) download
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.")));
}
/*
* Re: Improve the HINT message of the ALTER command for postgres_fdw
@ 2021-10-08 07:38 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
parent: bt21masumurak <bt21masumurak@oss.nttdata.com>
0 siblings, 1 reply; 14+ messages in thread
From: Bharath Rupireddy @ 2021-10-08 07:38 UTC (permalink / raw)
To: bt21masumurak <bt21masumurak@oss.nttdata.com>; +Cc: pgsql-hackers
On Fri, Oct 8, 2021 at 12:48 PM bt21masumurak
<bt21masumurak@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. 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.
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.
Regards,
Bharath Rupireddy.
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Improve the HINT message of the ALTER command for postgres_fdw
@ 2021-10-08 08:13 Daniel Gustafsson <daniel@yesql.se>
parent: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Daniel Gustafsson @ 2021-10-08 08:13 UTC (permalink / raw)
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; +Cc: bt21masumurak <bt21masumurak@oss.nttdata.com>; pgsql-hackers
> On 8 Oct 2021, at 09:38, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Fri, Oct 8, 2021 at 12:48 PM bt21masumurak
> <bt21masumurak@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/
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Improve the HINT message of the ALTER command for postgres_fdw
@ 2021-10-12 10:57 bt21masumurak <bt21masumurak@oss.nttdata.com>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 14+ messages in thread
From: bt21masumurak @ 2021-10-12 10:57 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; pgsql-hackers
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.rupireddyforpostgres@gmail.com> wrote:
>>
>> On Fri, Oct 8, 2021 at 12:48 PM bt21masumurak
>> <bt21masumurak@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/
Attachments:
diff.patch (text/x-diff, 6.8K) download
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;
* Re: Improve the HINT message of the ALTER command for postgres_fdw
@ 2021-10-12 17:41 Fujii Masao <masao.fujii@oss.nttdata.com>
parent: bt21masumurak <bt21masumurak@oss.nttdata.com>
0 siblings, 1 reply; 14+ messages in thread
From: Fujii Masao @ 2021-10-12 17:41 UTC (permalink / raw)
To: bt21masumurak <bt21masumurak@oss.nttdata.com>; Daniel Gustafsson <daniel@yesql.se>; +Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; pgsql-hackers
On 2021/10/12 19:57, bt21masumurak wrote:
> I made new patch based on those comments.
Thanks for updating the patch!
- errhint("HOGEHOGEValid options in this context are: %s",
- buf.data)));
The patch contains the garbage "HOGEHOGE" in the above,
which causes the compiler to fail. Attached is the updated version
of the patch. I got rid of the garbage.
+--HINT test
+ALTER FOREIGN DATA WRAPPER file_fdw OPTIONS (format 'csv');
file_fdw already has the test for ALTER FOREIGN DATA WRAPPER .. OPTIONS,
so you don't need to add new test for the command into file_fdw.
I removed that test from file_fdw.
Also I moved the tests for ALTER FOREIGN DATA WRAPPER .. OPTIONS,
in the tests of postgres_fdw, dblink, and foreign data, into more proper
places.
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)
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Attachments:
alter_foreign_data_wrapper_options_v2.patch (text/plain, 6.6K) download
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/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..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.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..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.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;
* Re: Improve the HINT message of the ALTER command for postgres_fdw
@ 2021-10-13 05:00 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
parent: Fujii Masao <masao.fujii@oss.nttdata.com>
0 siblings, 1 reply; 14+ messages in thread
From: Bharath Rupireddy @ 2021-10-13 05:00 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@oss.nttdata.com>; +Cc: bt21masumurak <bt21masumurak@oss.nttdata.com>; Daniel Gustafsson <daniel@yesql.se>; pgsql-hackers
On Tue, Oct 12, 2021 at 11:11 PM Fujii Masao
<masao.fujii@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.
Regards,
Bharath Rupireddy.
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Improve the HINT message of the ALTER command for postgres_fdw
@ 2021-10-13 17:36 Fujii Masao <masao.fujii@oss.nttdata.com>
parent: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Fujii Masao @ 2021-10-13 17:36 UTC (permalink / raw)
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; +Cc: bt21masumurak <bt21masumurak@oss.nttdata.com>; Daniel Gustafsson <daniel@yesql.se>; pgsql-hackers
On 2021/10/13 14:00, Bharath Rupireddy wrote:
> On Tue, Oct 12, 2021 at 11:11 PM Fujii Masao
> <masao.fujii@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
Attachments:
alter_foreign_data_wrapper_options_v3.patch (text/plain, 7.2K) download
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;
* Re: Improve the HINT message of the ALTER command for postgres_fdw
@ 2021-10-16 10:43 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
parent: Fujii Masao <masao.fujii@oss.nttdata.com>
0 siblings, 1 reply; 14+ messages in thread
From: Bharath Rupireddy @ 2021-10-16 10:43 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@oss.nttdata.com>; +Cc: bt21masumurak <bt21masumurak@oss.nttdata.com>; Daniel Gustafsson <daniel@yesql.se>; pgsql-hackers
On Wed, Oct 13, 2021 at 11:06 PM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:
> On 2021/10/13 14:00, Bharath Rupireddy wrote:
> > On Tue, Oct 12, 2021 at 11:11 PM Fujii Masao
> > <masao.fujii@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
I'm fine with the distinction that's made, now I'm thinking about the
appropriate areas where ERRCODE_FDW_INVALID_OPTION_NAME can be used.
Is it correct to use ERRCODE_FDW_INVALID_OPTION_NAME in
postgresImportForeignSchema where we don't check buffer length and
option name length but throw the error when we don't find what's being
expected for IMPORT FOREIGN SCHEMA command? Isn't it the
ERRCODE_FDW_OPTION_NAME_NOT_FOUND right choice there? I've seen some
of the option parsing logic(with the search text "stmt->options)" in
the code base), they are mostly using "option \"%s\" not recognized"
without an error code or "unrecognized XXXX option \"%s\"" with
ERRCODE_SYNTAX_ERROR. I'm not sure which is right. If not in
postgresImportForeignSchema, where else can
ERRCODE_FDW_INVALID_OPTION_NAME be used?
If we were to retain the error code ERRCODE_FDW_INVALID_OPTION_NAME,
do we need to maintain the difference in documentation or in code
comments or in the commit message at least?
Regards,
Bharath Rupireddy.
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Improve the HINT message of the ALTER command for postgres_fdw
@ 2021-10-25 06:30 Fujii Masao <masao.fujii@oss.nttdata.com>
parent: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
0 siblings, 2 replies; 14+ messages in thread
From: Fujii Masao @ 2021-10-25 06:30 UTC (permalink / raw)
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; +Cc: bt21masumurak <bt21masumurak@oss.nttdata.com>; Daniel Gustafsson <daniel@yesql.se>; pgsql-hackers
On 2021/10/16 19:43, Bharath Rupireddy wrote:
> I'm fine with the distinction that's made, now I'm thinking about the
> appropriate areas where ERRCODE_FDW_INVALID_OPTION_NAME can be used.
> Is it correct to use ERRCODE_FDW_INVALID_OPTION_NAME in
> postgresImportForeignSchema where we don't check buffer length and
> option name length but throw the error when we don't find what's being
> expected for IMPORT FOREIGN SCHEMA command? Isn't it the
> ERRCODE_FDW_OPTION_NAME_NOT_FOUND right choice there? I've seen some
> of the option parsing logic(with the search text "stmt->options)" in
> the code base), they are mostly using "option \"%s\" not recognized"
> without an error code or "unrecognized XXXX option \"%s\"" with
> ERRCODE_SYNTAX_ERROR. I'm not sure which is right. If not in
> postgresImportForeignSchema, where else can
> ERRCODE_FDW_INVALID_OPTION_NAME be used?
These are good questions. But TBH I don't know the answers and have not
found good articles describing more detail definitions of those error codes.
I'm tempted to improve the HINT message part at first because it has
obviously an issue. And then we can consider what error code should be
used in FDW layer if necessary.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Improve the HINT message of the ALTER command for postgres_fdw
@ 2021-10-25 07:44 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
parent: Fujii Masao <masao.fujii@oss.nttdata.com>
1 sibling, 1 reply; 14+ messages in thread
From: Bharath Rupireddy @ 2021-10-25 07:44 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@oss.nttdata.com>; +Cc: bt21masumurak <bt21masumurak@oss.nttdata.com>; Daniel Gustafsson <daniel@yesql.se>; pgsql-hackers
On Mon, Oct 25, 2021 at 12:00 PM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:
>
> On 2021/10/16 19:43, Bharath Rupireddy wrote:
> > I'm fine with the distinction that's made, now I'm thinking about the
> > appropriate areas where ERRCODE_FDW_INVALID_OPTION_NAME can be used.
> > Is it correct to use ERRCODE_FDW_INVALID_OPTION_NAME in
> > postgresImportForeignSchema where we don't check buffer length and
> > option name length but throw the error when we don't find what's being
> > expected for IMPORT FOREIGN SCHEMA command? Isn't it the
> > ERRCODE_FDW_OPTION_NAME_NOT_FOUND right choice there? I've seen some
> > of the option parsing logic(with the search text "stmt->options)" in
> > the code base), they are mostly using "option \"%s\" not recognized"
> > without an error code or "unrecognized XXXX option \"%s\"" with
> > ERRCODE_SYNTAX_ERROR. I'm not sure which is right. If not in
> > postgresImportForeignSchema, where else can
> > ERRCODE_FDW_INVALID_OPTION_NAME be used?
>
> These are good questions. But TBH I don't know the answers and have not
> found good articles describing more detail definitions of those error codes.
> I'm tempted to improve the HINT message part at first because it has
> obviously an issue. And then we can consider what error code should be
> used in FDW layer if necessary.
Yeah, let's focus on fixing the hint message here and the
alter_foreign_data_wrapper_options_v3.patch LGTM.
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.
Regards,
Bharath Rupireddy.
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Improve the HINT message of the ALTER command for postgres_fdw
@ 2021-10-25 07:50 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
parent: Fujii Masao <masao.fujii@oss.nttdata.com>
1 sibling, 0 replies; 14+ messages in thread
From: Bharath Rupireddy @ 2021-10-25 07:50 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@oss.nttdata.com>; +Cc: bt21masumurak <bt21masumurak@oss.nttdata.com>; Daniel Gustafsson <daniel@yesql.se>; pgsql-hackers
On Mon, Oct 25, 2021 at 12:00 PM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:
> On 2021/10/16 19:43, Bharath Rupireddy wrote:
> > I'm fine with the distinction that's made, now I'm thinking about the
> > appropriate areas where ERRCODE_FDW_INVALID_OPTION_NAME can be used.
> > Is it correct to use ERRCODE_FDW_INVALID_OPTION_NAME in
> > postgresImportForeignSchema where we don't check buffer length and
> > option name length but throw the error when we don't find what's being
> > expected for IMPORT FOREIGN SCHEMA command? Isn't it the
> > ERRCODE_FDW_OPTION_NAME_NOT_FOUND right choice there? I've seen some
> > of the option parsing logic(with the search text "stmt->options)" in
> > the code base), they are mostly using "option \"%s\" not recognized"
> > without an error code or "unrecognized XXXX option \"%s\"" with
> > ERRCODE_SYNTAX_ERROR. I'm not sure which is right. If not in
> > postgresImportForeignSchema, where else can
> > ERRCODE_FDW_INVALID_OPTION_NAME be used?
>
> These are good questions. But TBH I don't know the answers and have not
> found good articles describing more detail definitions of those error codes.
> And then we can consider what error code should be
> used in FDW layer if necessary.
Yeah, after this HINT message correction patch gets in, another thread
can be started for the error code usage discussion.
Regards,
Bharath Rupireddy.
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Improve the HINT message of the ALTER command for postgres_fdw
@ 2021-10-25 11:06 Fujii Masao <masao.fujii@oss.nttdata.com>
parent: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Fujii Masao @ 2021-10-25 11:06 UTC (permalink / raw)
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; +Cc: bt21masumurak <bt21masumurak@oss.nttdata.com>; Daniel Gustafsson <daniel@yesql.se>; pgsql-hackers
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) download
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/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..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.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..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.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;
* Re: Improve the HINT message of the ALTER command for postgres_fdw
@ 2021-10-25 12:27 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
parent: Fujii Masao <masao.fujii@oss.nttdata.com>
0 siblings, 1 reply; 14+ messages in thread
From: Bharath Rupireddy @ 2021-10-25 12:27 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@oss.nttdata.com>; +Cc: bt21masumurak <bt21masumurak@oss.nttdata.com>; Daniel Gustafsson <daniel@yesql.se>; pgsql-hackers
On Mon, Oct 25, 2021 at 4:36 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> 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.
That's okay as we plan to deal with the error code stuff separately.
> > 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.
I missed it. Thanks. I see that there are already test cases covering
error message with hint - "There are no valid options in this
context."
The v4 patch LGTM.
Regards,
Bharath Rupireddy.
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Improve the HINT message of the ALTER command for postgres_fdw
@ 2021-10-26 16:04 Fujii Masao <masao.fujii@oss.nttdata.com>
parent: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
0 siblings, 0 replies; 14+ messages in thread
From: Fujii Masao @ 2021-10-26 16:04 UTC (permalink / raw)
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; +Cc: bt21masumurak <bt21masumurak@oss.nttdata.com>; Daniel Gustafsson <daniel@yesql.se>; pgsql-hackers
On 2021/10/25 21:27, Bharath Rupireddy wrote:
> On Mon, Oct 25, 2021 at 4:36 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>> 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.
>
> That's okay as we plan to deal with the error code stuff separately.
>
>>> 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.
>
> I missed it. Thanks. I see that there are already test cases covering
> error message with hint - "There are no valid options in this
> context."
>
> The v4 patch LGTM.
Pushed. Thanks!
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 14+ messages in thread
end of thread, other threads:[~2021-10-26 16:04 UTC | newest]
Thread overview: 14+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-10-08 07:18 Improve the HINT message of the ALTER command for postgres_fdw bt21masumurak <bt21masumurak@oss.nttdata.com>
2021-10-08 07:38 ` Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
2021-10-08 08:13 ` Daniel Gustafsson <daniel@yesql.se>
2021-10-12 10:57 ` bt21masumurak <bt21masumurak@oss.nttdata.com>
2021-10-12 17:41 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2021-10-13 05:00 ` Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
2021-10-13 17:36 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2021-10-16 10:43 ` Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
2021-10-25 06:30 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2021-10-25 07:44 ` Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
2021-10-25 11:06 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2021-10-25 12:27 ` Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
2021-10-26 16:04 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2021-10-25 07:50 ` Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
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