From: bt21masumurak <bt21masumurak@oss.nttdata.com>
To: Daniel Gustafsson <daniel@yesql.se>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Improve the HINT message of the ALTER command for postgres_fdw
Date: Tue, 12 Oct 2021 19:57:53 +0900
Message-ID: <46864d7a8ca6c2faeb2f9a7a401d47c0@oss.nttdata.com> (raw)
In-Reply-To: <250E9750-57EA-45BA-82A4-787CC61F7309@yesql.se>
References: <557d06cebe19081bfcc83ee2affc98d3@oss.nttdata.com>
<CALj2ACUkZ9FWZA0XgjBeMX-M_Pg2Z=xhvFsy39X6699jDWr5xw@mail.gmail.com>
<250E9750-57EA-45BA-82A4-787CC61F7309@yesql.se>
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) 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..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.sqlindex 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.sourceindex 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.sourceindex 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.outindex 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.cindex 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.sqlindex 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.cindex 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.outindex 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.sqlindex 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;
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: bt21masumurak@oss.nttdata.com, daniel@yesql.se, bharath.rupireddyforpostgres@gmail.com
Subject: Re: Improve the HINT message of the ALTER command for postgres_fdw
In-Reply-To: <46864d7a8ca6c2faeb2f9a7a401d47c0@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