Re: [HACKERS] no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...

2017-09-15 Thread Amit Langote 
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 ...

2017-09-15 Thread Robert Haas 
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 ...

2017-09-14 Thread Ashutosh Bapat 
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 ...

2017-09-13 Thread Amit Langote 
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 | -

Re: [HACKERS] no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...

2017-09-13 Thread Ashutosh Bapat 
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 ...

2017-09-13 Thread Amit Langote 
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 ...

2017-09-13 Thread Ashutosh Bapat 
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 ...

2017-09-12 Thread Amit Langote 
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  |

Re: [HACKERS] no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...

2017-09-12 Thread Ashutosh Bapat 
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 ...

2017-09-12 Thread Amit Langote 
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

Re: [HACKERS] no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...

2017-09-05 Thread Ashutosh Bapat 
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 ...

2017-05-09 Thread Amit Langote 
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 ...

2017-05-09 Thread Mark Dilger 

> 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 ...

2017-05-09 Thread Ashutosh Bapat 
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 ...

2017-05-09 Thread Amit Langote 
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

Re: [HACKERS] no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...

2017-05-08 Thread Ashutosh Bapat 
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 ...

2017-05-04 Thread Mark Dilger 
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