Re: [HACKERS] no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...
On Fri, Sep 15, 2017 at 9:20 PM, Robert Haas wrote: > On Thu, Sep 14, 2017 at 8:30 AM, Ashutosh Bapat > wrote: >> LGTM. The patch applies cleanly on the current HEAD, compiles (small >> bit in regress.c requires compilation), and make check passes. Marking >> this as "ready for committer". > > Committed. Thanks, closed in the CF app. Regards, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...
On Thu, Sep 14, 2017 at 8:30 AM, Ashutosh Bapat wrote: > LGTM. The patch applies cleanly on the current HEAD, compiles (small > bit in regress.c requires compilation), and make check passes. Marking > this as "ready for committer". Committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...
On Wed, Sep 13, 2017 at 1:46 PM, Amit Langote wrote: >> >> Ok. May be then create_function_1.sql is the right place. Just add it >> to the section of passing tests and annotate that it's testing >> creating an FDW handler. Sorry for jumping back and forth. > > Alright, done. Thanks for reviewing. > LGTM. The patch applies cleanly on the current HEAD, compiles (small bit in regress.c requires compilation), and make check passes. Marking this as "ready for committer". -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...
On 2017/09/13 16:59, Ashutosh Bapat wrote: > On Wed, Sep 13, 2017 at 1:27 PM, Amit Langote > wrote: >> On 2017/09/13 16:42, Ashutosh Bapat wrote: >>> On Wed, Sep 13, 2017 at 7:49 AM, Amit Langote wrote: In the attached updated patch, I created separate .source files in src/test/regress/input and output directories called fdw_handler.source and put the test_fdw_handler function definition there. When I had originally thought of it back when I wrote the patch, it seemed to be an overkill, because we're just normally defining a single C function there to be used in the newly added foreign_data tests. In any case, we need to go the .source file way, because that's the only way to refer to paths to .so library when defining C language functions. >>> >>> It still looks like an overkill to add a new file to define a dummy >>> FDW handler. Why do we need to define a handler as a C function? Can't >>> we define handler as a SQL function. If we could do that we could add >>> the function definition in foreign_data.sql itself. >> >> I guess that's because the last time I tried to define the handler as a >> SQL function, I couldn't: >> >> create function test_fdw_handler() returns fdw_handler as '' language sql; >> ERROR: SQL functions cannot return type fdw_handler >> >> fdw_handler is a pseudo-type, which neither SQL nor plpgsql function can >> return. >> >> Am I missing something? > > Ok. May be then create_function_1.sql is the right place. Just add it > to the section of passing tests and annotate that it's testing > creating an FDW handler. Sorry for jumping back and forth. Alright, done. Thanks for reviewing. Regards, Amit From d0c28965b892ac76d83987e9bf35e8ab8fd62e11 Mon Sep 17 00:00:00 2001 From: amit Date: Wed, 10 May 2017 10:37:42 +0900 Subject: [PATCH] Add some FDW HANDLER DDL tests --- src/test/regress/expected/foreign_data.out | 28 +++- src/test/regress/input/create_function_1.source | 6 + src/test/regress/output/create_function_1.source | 5 + src/test/regress/regress.c | 7 ++ src/test/regress/sql/foreign_data.sql| 13 +++ 5 files changed, 53 insertions(+), 6 deletions(-) diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out index c6e558b07f..331f7a911f 100644 --- a/src/test/regress/expected/foreign_data.out +++ b/src/test/regress/expected/foreign_data.out @@ -89,6 +89,14 @@ CREATE FOREIGN DATA WRAPPER foo VALIDATOR postgresql_fdw_validator; postgresql | regress_foreign_data_user | - | postgresql_fdw_validator | | | (3 rows) +-- HANDLER related checks +CREATE FUNCTION invalid_fdw_handler() RETURNS int LANGUAGE SQL AS 'SELECT 1;'; +CREATE FOREIGN DATA WRAPPER test_fdw HANDLER invalid_fdw_handler; -- ERROR +ERROR: function invalid_fdw_handler must return type fdw_handler +CREATE FOREIGN DATA WRAPPER test_fdw HANDLER test_fdw_handler HANDLER invalid_fdw_handler; -- ERROR +ERROR: conflicting or redundant options +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; -- ERROR ERROR: syntax error at or near ";" @@ -188,18 +196,26 @@ ALTER FOREIGN DATA WRAPPER foo RENAME TO foo1; (3 rows) ALTER FOREIGN DATA WRAPPER foo1 RENAME TO foo; +-- HANDLER related checks +ALTER FOREIGN DATA WRAPPER foo HANDLER invalid_fdw_handler; -- ERROR +ERROR: function invalid_fdw_handler must return type fdw_handler +ALTER FOREIGN DATA WRAPPER foo HANDLER test_fdw_handler HANDLER anything; -- ERROR +ERROR: conflicting or redundant options +ALTER FOREIGN DATA WRAPPER foo HANDLER test_fdw_handler; +WARNING: changing the foreign-data wrapper handler can change behavior of existing foreign tables +DROP FUNCTION invalid_fdw_handler(); -- DROP FOREIGN DATA WRAPPER DROP FOREIGN DATA WRAPPER nonexistent; -- ERROR ERROR: foreign-data wrapper "nonexistent" does not exist DROP FOREIGN DATA WRAPPER IF EXISTS nonexistent; NOTICE: foreign-data wrapper "nonexistent" does not exist, skipping \dew+ -List of foreign-data wrappers -Name| Owner | Handler |Validator | Access privileges | FDW options | Description -+---+-+--+---+--+- - dummy | regress_foreign_data_user | - | -| | | useless - foo| regress_test_role_super | - | -| | (b '3', c '4', a '2', d '5') | - postgresql | regress_foreign_data_user | - | postgresql_fdw_validator | |
Re: [HACKERS] no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...
On Wed, Sep 13, 2017 at 1:27 PM, Amit Langote wrote: > On 2017/09/13 16:42, Ashutosh Bapat wrote: >> On Wed, Sep 13, 2017 at 7:49 AM, Amit Langote wrote: >>> In the attached updated patch, I created separate .source files in >>> src/test/regress/input and output directories called fdw_handler.source >>> and put the test_fdw_handler function definition there. When I had >>> originally thought of it back when I wrote the patch, it seemed to be an >>> overkill, because we're just normally defining a single C function there >>> to be used in the newly added foreign_data tests. In any case, we need to >>> go the .source file way, because that's the only way to refer to paths to >>> .so library when defining C language functions. >> >> It still looks like an overkill to add a new file to define a dummy >> FDW handler. Why do we need to define a handler as a C function? Can't >> we define handler as a SQL function. If we could do that we could add >> the function definition in foreign_data.sql itself. > > I guess that's because the last time I tried to define the handler as a > SQL function, I couldn't: > > create function test_fdw_handler() returns fdw_handler as '' language sql; > ERROR: SQL functions cannot return type fdw_handler > > fdw_handler is a pseudo-type, which neither SQL nor plpgsql function can > return. > > Am I missing something? Ok. May be then create_function_1.sql is the right place. Just add it to the section of passing tests and annotate that it's testing creating an FDW handler. Sorry for jumping back and forth. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...
On 2017/09/13 16:42, Ashutosh Bapat wrote: > On Wed, Sep 13, 2017 at 7:49 AM, Amit Langote wrote: >> In the attached updated patch, I created separate .source files in >> src/test/regress/input and output directories called fdw_handler.source >> and put the test_fdw_handler function definition there. When I had >> originally thought of it back when I wrote the patch, it seemed to be an >> overkill, because we're just normally defining a single C function there >> to be used in the newly added foreign_data tests. In any case, we need to >> go the .source file way, because that's the only way to refer to paths to >> .so library when defining C language functions. > > It still looks like an overkill to add a new file to define a dummy > FDW handler. Why do we need to define a handler as a C function? Can't > we define handler as a SQL function. If we could do that we could add > the function definition in foreign_data.sql itself. I guess that's because the last time I tried to define the handler as a SQL function, I couldn't: create function test_fdw_handler() returns fdw_handler as '' language sql; ERROR: SQL functions cannot return type fdw_handler fdw_handler is a pseudo-type, which neither SQL nor plpgsql function can return. Am I missing something? Thanks, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...
On Wed, Sep 13, 2017 at 7:49 AM, Amit Langote wrote: > On 2017/09/12 20:17, Ashutosh Bapat wrote: >> On Tue, Sep 12, 2017 at 2:27 PM, Amit Langote >> wrote: >>> Thanks Ashutosh for taking a look at this. >>> >>> On 2017/09/05 21:16, Ashutosh Bapat wrote: The patch needs a rebase. >>> >>> Attached rebased patch. >> >> Thanks for rebased patch. > > Thanks for the review. > >> We could annotate each ERROR with an explanation as to why that's an >> error, but then this file doesn't do that for other commands, so may >> be the patch is just fine. > > Agreed. Note that this patch is just about adding the tests, not > modifying foreigncmds.c to change error handling around HANDLER functions. Yes. I am not concerned about foreigncmds.c but foreign_data.sql/.out > >> Also, I am wondering whether we should create the new handler function >> in foreign.c similar to postgresql_fdw_validator(). The prologue has a >> caution >> >> 606 * Caution: this function is deprecated, and is now meant only for >> testing >> 607 * purposes, because the list of options it knows about doesn't >> necessarily >> 608 * square with those known to whichever libpq instance you might be >> using. >> 609 * Inquire of libpq itself, instead. >> >> So, may be we don't want to add it there. But adding the handler >> function in create_function_1 doesn't seem good. If that's the correct >> place, then at least it should be moved before " -- Things that >> shouldn't work:"; it doesn't belong to functions that don't work. > > In the attached updated patch, I created separate .source files in > src/test/regress/input and output directories called fdw_handler.source > and put the test_fdw_handler function definition there. When I had > originally thought of it back when I wrote the patch, it seemed to be an > overkill, because we're just normally defining a single C function there > to be used in the newly added foreign_data tests. In any case, we need to > go the .source file way, because that's the only way to refer to paths to > .so library when defining C language functions. It still looks like an overkill to add a new file to define a dummy FDW handler. Why do we need to define a handler as a C function? Can't we define handler as a SQL function. If we could do that we could add the function definition in foreign_data.sql itself. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...
On 2017/09/12 20:17, Ashutosh Bapat wrote: > On Tue, Sep 12, 2017 at 2:27 PM, Amit Langote > wrote: >> Thanks Ashutosh for taking a look at this. >> >> On 2017/09/05 21:16, Ashutosh Bapat wrote: >>> The patch needs a rebase. >> >> Attached rebased patch. > > Thanks for rebased patch. Thanks for the review. > We could annotate each ERROR with an explanation as to why that's an > error, but then this file doesn't do that for other commands, so may > be the patch is just fine. Agreed. Note that this patch is just about adding the tests, not modifying foreigncmds.c to change error handling around HANDLER functions. > Also, I am wondering whether we should create the new handler function > in foreign.c similar to postgresql_fdw_validator(). The prologue has a > caution > > 606 * Caution: this function is deprecated, and is now meant only for testing > 607 * purposes, because the list of options it knows about doesn't > necessarily > 608 * square with those known to whichever libpq instance you might be using. > 609 * Inquire of libpq itself, instead. > > So, may be we don't want to add it there. But adding the handler > function in create_function_1 doesn't seem good. If that's the correct > place, then at least it should be moved before " -- Things that > shouldn't work:"; it doesn't belong to functions that don't work. In the attached updated patch, I created separate .source files in src/test/regress/input and output directories called fdw_handler.source and put the test_fdw_handler function definition there. When I had originally thought of it back when I wrote the patch, it seemed to be an overkill, because we're just normally defining a single C function there to be used in the newly added foreign_data tests. In any case, we need to go the .source file way, because that's the only way to refer to paths to .so library when defining C language functions. Thanks, Amit From 510987531bfdf22df0bc8eef27f232e580d415b1 Mon Sep 17 00:00:00 2001 From: amit Date: Wed, 10 May 2017 10:37:42 +0900 Subject: [PATCH] Add some FDW HANDLER DDL tests --- src/test/regress/expected/foreign_data.out | 28 ++-- src/test/regress/input/fdw_handler.source | 5 + src/test/regress/output/fdw_handler.source | 5 + src/test/regress/parallel_schedule | 2 +- src/test/regress/regress.c | 7 +++ src/test/regress/serial_schedule | 1 + src/test/regress/sql/.gitignore| 1 + src/test/regress/sql/foreign_data.sql | 13 + 8 files changed, 55 insertions(+), 7 deletions(-) create mode 100644 src/test/regress/input/fdw_handler.source create mode 100644 src/test/regress/output/fdw_handler.source diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out index c6e558b07f..331f7a911f 100644 --- a/src/test/regress/expected/foreign_data.out +++ b/src/test/regress/expected/foreign_data.out @@ -89,6 +89,14 @@ CREATE FOREIGN DATA WRAPPER foo VALIDATOR postgresql_fdw_validator; postgresql | regress_foreign_data_user | - | postgresql_fdw_validator | | | (3 rows) +-- HANDLER related checks +CREATE FUNCTION invalid_fdw_handler() RETURNS int LANGUAGE SQL AS 'SELECT 1;'; +CREATE FOREIGN DATA WRAPPER test_fdw HANDLER invalid_fdw_handler; -- ERROR +ERROR: function invalid_fdw_handler must return type fdw_handler +CREATE FOREIGN DATA WRAPPER test_fdw HANDLER test_fdw_handler HANDLER invalid_fdw_handler; -- ERROR +ERROR: conflicting or redundant options +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; -- ERROR ERROR: syntax error at or near ";" @@ -188,18 +196,26 @@ ALTER FOREIGN DATA WRAPPER foo RENAME TO foo1; (3 rows) ALTER FOREIGN DATA WRAPPER foo1 RENAME TO foo; +-- HANDLER related checks +ALTER FOREIGN DATA WRAPPER foo HANDLER invalid_fdw_handler; -- ERROR +ERROR: function invalid_fdw_handler must return type fdw_handler +ALTER FOREIGN DATA WRAPPER foo HANDLER test_fdw_handler HANDLER anything; -- ERROR +ERROR: conflicting or redundant options +ALTER FOREIGN DATA WRAPPER foo HANDLER test_fdw_handler; +WARNING: changing the foreign-data wrapper handler can change behavior of existing foreign tables +DROP FUNCTION invalid_fdw_handler(); -- DROP FOREIGN DATA WRAPPER DROP FOREIGN DATA WRAPPER nonexistent; -- ERROR ERROR: foreign-data wrapper "nonexistent" does not exist DROP FOREIGN DATA WRAPPER IF EXISTS nonexistent; NOTICE: foreign-data wrapper "nonexistent" does not exist, skipping \dew+ -List of foreign-data wrappers -Name| Owner | Handler |Validator | Access privileges | FDW options | Description -+---+--
Re: [HACKERS] no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...
On Tue, Sep 12, 2017 at 2:27 PM, Amit Langote wrote: > Thanks Ashutosh for taking a look at this. > > On 2017/09/05 21:16, Ashutosh Bapat wrote: >> The patch needs a rebase. > > Attached rebased patch. Thanks for rebased patch. We could annotate each ERROR with an explanation as to why that's an error, but then this file doesn't do that for other commands, so may be the patch is just fine. Also, I am wondering whether we should create the new handler function in foreign.c similar to postgresql_fdw_validator(). The prologue has a caution 606 * Caution: this function is deprecated, and is now meant only for testing 607 * purposes, because the list of options it knows about doesn't necessarily 608 * square with those known to whichever libpq instance you might be using. 609 * Inquire of libpq itself, instead. So, may be we don't want to add it there. But adding the handler function in create_function_1 doesn't seem good. If that's the correct place, then at least it should be moved before " -- Things that shouldn't work:"; it doesn't belong to functions that don't work. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...
Thanks Ashutosh for taking a look at this. On 2017/09/05 21:16, Ashutosh Bapat wrote: > The patch needs a rebase. Attached rebased patch. Thanks, Amit From 75bcb6ebcc00193cb0251fced994f03d205e9e7f Mon Sep 17 00:00:00 2001 From: amit Date: Wed, 10 May 2017 10:37:42 +0900 Subject: [PATCH] Add some FDW HANDLER DDL tests --- src/test/regress/expected/foreign_data.out | 28 +++- src/test/regress/input/create_function_1.source | 3 +++ src/test/regress/output/create_function_1.source | 2 ++ src/test/regress/regress.c | 7 ++ src/test/regress/sql/foreign_data.sql| 13 +++ 5 files changed, 47 insertions(+), 6 deletions(-) diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out index c6e558b07f..331f7a911f 100644 --- a/src/test/regress/expected/foreign_data.out +++ b/src/test/regress/expected/foreign_data.out @@ -89,6 +89,14 @@ CREATE FOREIGN DATA WRAPPER foo VALIDATOR postgresql_fdw_validator; postgresql | regress_foreign_data_user | - | postgresql_fdw_validator | | | (3 rows) +-- HANDLER related checks +CREATE FUNCTION invalid_fdw_handler() RETURNS int LANGUAGE SQL AS 'SELECT 1;'; +CREATE FOREIGN DATA WRAPPER test_fdw HANDLER invalid_fdw_handler; -- ERROR +ERROR: function invalid_fdw_handler must return type fdw_handler +CREATE FOREIGN DATA WRAPPER test_fdw HANDLER test_fdw_handler HANDLER invalid_fdw_handler; -- ERROR +ERROR: conflicting or redundant options +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; -- ERROR ERROR: syntax error at or near ";" @@ -188,18 +196,26 @@ ALTER FOREIGN DATA WRAPPER foo RENAME TO foo1; (3 rows) ALTER FOREIGN DATA WRAPPER foo1 RENAME TO foo; +-- HANDLER related checks +ALTER FOREIGN DATA WRAPPER foo HANDLER invalid_fdw_handler; -- ERROR +ERROR: function invalid_fdw_handler must return type fdw_handler +ALTER FOREIGN DATA WRAPPER foo HANDLER test_fdw_handler HANDLER anything; -- ERROR +ERROR: conflicting or redundant options +ALTER FOREIGN DATA WRAPPER foo HANDLER test_fdw_handler; +WARNING: changing the foreign-data wrapper handler can change behavior of existing foreign tables +DROP FUNCTION invalid_fdw_handler(); -- DROP FOREIGN DATA WRAPPER DROP FOREIGN DATA WRAPPER nonexistent; -- ERROR ERROR: foreign-data wrapper "nonexistent" does not exist DROP FOREIGN DATA WRAPPER IF EXISTS nonexistent; NOTICE: foreign-data wrapper "nonexistent" does not exist, skipping \dew+ -List of foreign-data wrappers -Name| Owner | Handler |Validator | Access privileges | FDW options | Description -+---+-+--+---+--+- - dummy | regress_foreign_data_user | - | -| | | useless - foo| regress_test_role_super | - | -| | (b '3', c '4', a '2', d '5') | - postgresql | regress_foreign_data_user | - | postgresql_fdw_validator | | | + List of foreign-data wrappers +Name| Owner | Handler |Validator | Access privileges | FDW options | Description ++---+--+--+---+--+- + dummy | regress_foreign_data_user | -| - | | | useless + foo| regress_test_role_super | test_fdw_handler | - | | (b '3', c '4', a '2', d '5') | + postgresql | regress_foreign_data_user | -| postgresql_fdw_validator | | | (3 rows) DROP ROLE regress_test_role_super; -- ERROR diff --git a/src/test/regress/input/create_function_1.source b/src/test/regress/input/create_function_1.source index f2b1561cc2..669a5355b0 100644 --- a/src/test/regress/input/create_function_1.source +++ b/src/test/regress/input/create_function_1.source @@ -87,3 +87,6 @@ CREATE FUNCTION test1 (int) RETURNS int LANGUAGE C CREATE FUNCTION test1 (int) RETURNS int LANGUAGE internal AS 'nosuch'; + +CREATE FUNCTION test_fdw_handler () RETURNS fdw_handler LANGUAGE C +AS '@libdir@/regress@DLSUFFIX@', 'test_fdw_handler'; diff --git a/src/test/regress/output/create_function_1.source b/src/te
Re: [HACKERS] no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...
On Wed, May 10, 2017 at 5:55 AM, Amit Langote wrote: > On 2017/05/09 22:52, Mark Dilger wrote: >> >>> On May 9, 2017, at 12:18 AM, Amit Langote >>> wrote: >>> >>> Hi, >>> >>> On 2017/05/05 9:38, Mark Dilger wrote: Hackers, just FYI, I cannot find any regression test coverage of this part of the grammar, not even in the contrib/ directory or TAP tests. I was going to submit a patch to help out, and discovered it is not so easy to do, and perhaps that is why the coverage is missing. >>> >>> I think we could add the coverage by defining a dummy C FDW handler >>> function in regress.c. I see that some other regression tests use C >>> functions defined there, such as test_atomic_ops(). >>> >>> What do you think about the attached patch? I am assuming you only meant >>> to add tests for the code in foreigncmds.c (CREATE/ALTER FOREIGN DATA >>> WRAPPER). >> >> Thank you for creating the patch. I only see one small oversight, which is >> the >> successful case of ALTER FOREIGN DATA WRAPPER ... HANDLER ... is still >> not tested. I added one line for that in the attached modification of your >> patch. > > Ah right, thanks. > > Adding this to the next commitfest as Ashutosh suggested. > The patch needs a rebase. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...
On 2017/05/09 22:52, Mark Dilger wrote: > >> On May 9, 2017, at 12:18 AM, Amit Langote >> wrote: >> >> Hi, >> >> On 2017/05/05 9:38, Mark Dilger wrote: >>> Hackers, >>> >>> just FYI, I cannot find any regression test coverage of this part of the >>> grammar, not >>> even in the contrib/ directory or TAP tests. I was going to submit a patch >>> to help out, >>> and discovered it is not so easy to do, and perhaps that is why the >>> coverage is missing. >> >> I think we could add the coverage by defining a dummy C FDW handler >> function in regress.c. I see that some other regression tests use C >> functions defined there, such as test_atomic_ops(). >> >> What do you think about the attached patch? I am assuming you only meant >> to add tests for the code in foreigncmds.c (CREATE/ALTER FOREIGN DATA >> WRAPPER). > > Thank you for creating the patch. I only see one small oversight, which is > the > successful case of ALTER FOREIGN DATA WRAPPER ... HANDLER ... is still > not tested. I added one line for that in the attached modification of your > patch. Ah right, thanks. Adding this to the next commitfest as Ashutosh suggested. Regards, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...
> On May 9, 2017, at 12:18 AM, Amit Langote > wrote: > > Hi, > > On 2017/05/05 9:38, Mark Dilger wrote: >> Hackers, >> >> just FYI, I cannot find any regression test coverage of this part of the >> grammar, not >> even in the contrib/ directory or TAP tests. I was going to submit a patch >> to help out, >> and discovered it is not so easy to do, and perhaps that is why the coverage >> is missing. > > I think we could add the coverage by defining a dummy C FDW handler > function in regress.c. I see that some other regression tests use C > functions defined there, such as test_atomic_ops(). > > What do you think about the attached patch? I am assuming you only meant > to add tests for the code in foreigncmds.c (CREATE/ALTER FOREIGN DATA > WRAPPER). Thank you for creating the patch. I only see one small oversight, which is the successful case of ALTER FOREIGN DATA WRAPPER ... HANDLER ... is still not tested. I added one line for that in the attached modification of your patch. Mark Dilger 0002-Add-some-FDW-HANDLER-DDL-tests.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...
On Tue, May 9, 2017 at 12:48 PM, Amit Langote wrote: > Hi, > > On 2017/05/05 9:38, Mark Dilger wrote: >> Hackers, >> >> just FYI, I cannot find any regression test coverage of this part of the >> grammar, not >> even in the contrib/ directory or TAP tests. I was going to submit a patch >> to help out, >> and discovered it is not so easy to do, and perhaps that is why the coverage >> is missing. > > I think we could add the coverage by defining a dummy C FDW handler > function in regress.c. I see that some other regression tests use C > functions defined there, such as test_atomic_ops(). > > What do you think about the attached patch? I am assuming you only meant > to add tests for the code in foreigncmds.c (CREATE/ALTER FOREIGN DATA > WRAPPER). Looks ok to me. It at least tests whether function with the right return type has been provided and when there are multiple handlers provided. May be add it to the next commitfest for more opinions. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...
Hi, On 2017/05/05 9:38, Mark Dilger wrote: > Hackers, > > just FYI, I cannot find any regression test coverage of this part of the > grammar, not > even in the contrib/ directory or TAP tests. I was going to submit a patch > to help out, > and discovered it is not so easy to do, and perhaps that is why the coverage > is missing. I think we could add the coverage by defining a dummy C FDW handler function in regress.c. I see that some other regression tests use C functions defined there, such as test_atomic_ops(). What do you think about the attached patch? I am assuming you only meant to add tests for the code in foreigncmds.c (CREATE/ALTER FOREIGN DATA WRAPPER). Thanks, Amit >From 402d9212a07b75474145abd3358d4806f77476d7 Mon Sep 17 00:00:00 2001 From: amit Date: Mon, 8 May 2017 17:39:40 +0900 Subject: [PATCH] Add some FDW HANDLER DDL tests --- src/test/regress/expected/foreign_data.out | 14 ++ src/test/regress/input/create_function_1.source | 3 +++ src/test/regress/output/create_function_1.source | 2 ++ src/test/regress/regress.c | 7 +++ src/test/regress/sql/foreign_data.sql| 12 5 files changed, 38 insertions(+) diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out index 1c7a7593f9..035dd282ad 100644 --- a/src/test/regress/expected/foreign_data.out +++ b/src/test/regress/expected/foreign_data.out @@ -89,6 +89,14 @@ CREATE FOREIGN DATA WRAPPER foo VALIDATOR postgresql_fdw_validator; postgresql | regress_foreign_data_user | - | postgresql_fdw_validator | | | (3 rows) +-- HANDLER related checks +CREATE FUNCTION invalid_fdw_handler() RETURNS int LANGUAGE SQL AS 'SELECT 1;'; +CREATE FOREIGN DATA WRAPPER test_fdw HANDLER invalid_fdw_handler; -- ERROR +ERROR: function invalid_fdw_handler must return type fdw_handler +CREATE FOREIGN DATA WRAPPER test_fdw HANDLER test_fdw_handler HANDLER invalid_fdw_handler; -- ERROR +ERROR: conflicting or redundant options +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; -- ERROR ERROR: syntax error at or near ";" @@ -188,6 +196,12 @@ ALTER FOREIGN DATA WRAPPER foo RENAME TO foo1; (3 rows) ALTER FOREIGN DATA WRAPPER foo1 RENAME TO foo; +-- HANDLER related checks +ALTER FOREIGN DATA WRAPPER foo HANDLER invalid_fdw_handler; -- ERROR +ERROR: function invalid_fdw_handler must return type fdw_handler +ALTER FOREIGN DATA WRAPPER foo HANDLER test_fdw_handler HANDLER anything; -- ERROR +ERROR: conflicting or redundant options +DROP FUNCTION invalid_fdw_handler(); -- DROP FOREIGN DATA WRAPPER DROP FOREIGN DATA WRAPPER nonexistent; -- ERROR ERROR: foreign-data wrapper "nonexistent" does not exist diff --git a/src/test/regress/input/create_function_1.source b/src/test/regress/input/create_function_1.source index f2b1561cc2..669a5355b0 100644 --- a/src/test/regress/input/create_function_1.source +++ b/src/test/regress/input/create_function_1.source @@ -87,3 +87,6 @@ CREATE FUNCTION test1 (int) RETURNS int LANGUAGE C CREATE FUNCTION test1 (int) RETURNS int LANGUAGE internal AS 'nosuch'; + +CREATE FUNCTION test_fdw_handler () RETURNS fdw_handler LANGUAGE C +AS '@libdir@/regress@DLSUFFIX@', 'test_fdw_handler'; diff --git a/src/test/regress/output/create_function_1.source b/src/test/regress/output/create_function_1.source index 957595c51e..54e572d7a7 100644 --- a/src/test/regress/output/create_function_1.source +++ b/src/test/regress/output/create_function_1.source @@ -88,3 +88,5 @@ ERROR: could not find function "nosuchsymbol" in file "@libdir@/regress@DLSUFFI CREATE FUNCTION test1 (int) RETURNS int LANGUAGE internal AS 'nosuch'; ERROR: there is no built-in function named "nosuch" +CREATE FUNCTION test_fdw_handler () RETURNS fdw_handler LANGUAGE C +AS '@libdir@/regress@DLSUFFIX@', 'test_fdw_handler'; diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c index 80d0929df3..311b613659 100644 --- a/src/test/regress/regress.c +++ b/src/test/regress/regress.c @@ -1098,3 +1098,10 @@ test_atomic_ops(PG_FUNCTION_ARGS) PG_RETURN_BOOL(true); } + +PG_FUNCTION_INFO_V1(test_fdw_handler); +Datum +test_fdw_handler(PG_FUNCTION_ARGS) +{ + PG_RETURN_NULL(); +} diff --git a/src/test/regress/sql/foreign_data.sql b/src/test/regress/sql/foreign_data.sql index aaf079cf52..cc43535c30 100644 --- a/src/test/regress/sql/foreign_data.sql +++ b/src/test/regress/sql/foreign_data.sql @@ -51,6 +51,13 @@ RESET ROLE; CREATE FOREIGN DATA WRAPPER foo VALIDATOR postgresql_fdw_validator; \dew+ +-- HANDLER related checks +CREATE FUNCTION invalid_fdw_handler() RETURNS int LANGUAGE SQL AS 'SELECT 1;'; +CREATE FOREIGN DATA WRAPPER test_fdw HANDLER invalid_fdw_handler; -- ERROR +CREATE FOREIGN DATA WRAPPER test_fdw HANDLER test_fd
Re: [HACKERS] no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...
On Fri, May 5, 2017 at 6:08 AM, Mark Dilger wrote: > Hackers, > > just FYI, I cannot find any regression test coverage of this part of the > grammar, not > even in the contrib/ directory or TAP tests. I was going to submit a patch > to help out, > and discovered it is not so easy to do, and perhaps that is why the coverage > is missing. > > My apologies for the noise if this is already common knowledge. Also, if I'm > wrong, > I'd appreciate a pointer to the test that I am failing to find. > It depends on what do you want to test. You could write a simple test in postgres_fdw or file_fdw where you specify the same handler as CREATE FOREIGN DATA WRAPPER command (basically a no-op) to check whether the command succeeds. If you want to do any more serious testing, it will require defining at some of the FDW hooks that will be returned by the handler, and then exercising those hooks. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...
Hackers, just FYI, I cannot find any regression test coverage of this part of the grammar, not even in the contrib/ directory or TAP tests. I was going to submit a patch to help out, and discovered it is not so easy to do, and perhaps that is why the coverage is missing. My apologies for the noise if this is already common knowledge. Also, if I'm wrong, I'd appreciate a pointer to the test that I am failing to find. Thanks, Mark Dilger -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers