Обсуждение: Improve the HINT message of the ALTER command for postgres_fdw
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
Вложения
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.
> 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/
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/
Вложения
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
Вложения
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.
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
Вложения
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.
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
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.
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.
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
Вложения
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.
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