no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...

Started by Mark Dilgerover 8 years ago17 messages

hornschnorter@gmail.com

over 8 years ago

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

Ashutosh Bapat

ashutosh.bapat@enterprisedb.com

over 8 years ago

In reply to: Mark Dilger (#1)

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

On Fri, May 5, 2017 at 6:08 AM, Mark Dilger <hornschnorter@gmail.com> 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

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

over 8 years ago

In reply to: Mark Dilger (#1)

1 attachment(s)

Re: 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

Attachments:

0001-Add-some-FDW-HANDLER-DDL-tests.patchtext/x-diff; name=0001-Add-some-FDW-HANDLER-DDL-tests.patchDownload

From 402d9212a07b75474145abd3358d4806f77476d7 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
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_fdw_handler HANDLER invalid_fdw_handler;  -- ERROR
+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
 ALTER FOREIGN DATA WRAPPER foo VALIDATOR bar;               -- ERROR
@@ -88,6 +95,11 @@ ALTER FOREIGN DATA WRAPPER foo RENAME TO foo1;
 \dew+
 ALTER FOREIGN DATA WRAPPER foo1 RENAME TO foo;
 
+-- HANDLER related checks
+ALTER FOREIGN DATA WRAPPER foo HANDLER invalid_fdw_handler;  -- ERROR
+ALTER FOREIGN DATA WRAPPER foo HANDLER test_fdw_handler HANDLER anything;  -- ERROR
+DROP FUNCTION invalid_fdw_handler();
+
 -- DROP FOREIGN DATA WRAPPER
 DROP FOREIGN DATA WRAPPER nonexistent;                      -- ERROR
 DROP FOREIGN DATA WRAPPER IF EXISTS nonexistent;
-- 
2.11.0

Ashutosh Bapat

ashutosh.bapat@enterprisedb.com

over 8 years ago

In reply to: Amit Langote (#3)

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

On Tue, May 9, 2017 at 12:48 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> 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

Mark Dilger

hornschnorter@gmail.com

over 8 years ago

In reply to: Amit Langote (#3)

1 attachment(s)

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

On May 9, 2017, at 12:18 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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

Attachments:

0002-Add-some-FDW-HANDLER-DDL-tests.patchapplication/octet-stream; name=0002-Add-some-FDW-HANDLER-DDL-tests.patchDownload

diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out
index af327d0..f20d36b 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 f2b1561..669a535 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 957595c..54e572d 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 80d0929..311b613 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 ba528b5..91eccde 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_fdw_handler HANDLER invalid_fdw_handler;  -- ERROR
+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
 ALTER FOREIGN DATA WRAPPER foo VALIDATOR bar;               -- ERROR
@@ -88,6 +95,12 @@ ALTER FOREIGN DATA WRAPPER foo RENAME TO foo1;
 \dew+
 ALTER FOREIGN DATA WRAPPER foo1 RENAME TO foo;
 
+-- HANDLER related checks
+ALTER FOREIGN DATA WRAPPER foo HANDLER invalid_fdw_handler;  -- ERROR
+ALTER FOREIGN DATA WRAPPER foo HANDLER test_fdw_handler HANDLER anything;  -- ERROR
+ALTER FOREIGN DATA WRAPPER foo HANDLER test_fdw_handler;
+DROP FUNCTION invalid_fdw_handler();
+
 -- DROP FOREIGN DATA WRAPPER
 DROP FOREIGN DATA WRAPPER nonexistent;                      -- ERROR
 DROP FOREIGN DATA WRAPPER IF EXISTS nonexistent;

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

over 8 years ago

In reply to: Mark Dilger (#5)

Re: 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 <Langote_Amit_f8@lab.ntt.co.jp> 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

Ashutosh Bapat

ashutosh.bapat@enterprisedb.com

over 8 years ago

In reply to: Amit Langote (#6)

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

On Wed, May 10, 2017 at 5:55 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

On 2017/05/09 22:52, Mark Dilger wrote:

On May 9, 2017, at 12:18 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

over 8 years ago

In reply to: Ashutosh Bapat (#7)

1 attachment(s)

Re: 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

Attachments:

0001-Add-some-FDW-HANDLER-DDL-tests.patchtext/plain; charset=UTF-8; name=0001-Add-some-FDW-HANDLER-DDL-tests.patchDownload

From 75bcb6ebcc00193cb0251fced994f03d205e9e7f Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
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/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 734947cc98..0a123f2b39 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -1096,3 +1096,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 ebe8ffbffe..1af7258718 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_fdw_handler HANDLER invalid_fdw_handler;  -- ERROR
+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
 ALTER FOREIGN DATA WRAPPER foo VALIDATOR bar;               -- ERROR
@@ -88,6 +95,12 @@ ALTER FOREIGN DATA WRAPPER foo RENAME TO foo1;
 \dew+
 ALTER FOREIGN DATA WRAPPER foo1 RENAME TO foo;
 
+-- HANDLER related checks
+ALTER FOREIGN DATA WRAPPER foo HANDLER invalid_fdw_handler;  -- ERROR
+ALTER FOREIGN DATA WRAPPER foo HANDLER test_fdw_handler HANDLER anything;  -- ERROR
+ALTER FOREIGN DATA WRAPPER foo HANDLER test_fdw_handler;
+DROP FUNCTION invalid_fdw_handler();
+
 -- DROP FOREIGN DATA WRAPPER
 DROP FOREIGN DATA WRAPPER nonexistent;                      -- ERROR
 DROP FOREIGN DATA WRAPPER IF EXISTS nonexistent;
-- 
2.11.0

Ashutosh Bapat

ashutosh.bapat@enterprisedb.com

over 8 years ago

In reply to: Amit Langote (#8)

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

On Tue, Sep 12, 2017 at 2:27 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> 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

#10

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

over 8 years ago

In reply to: Ashutosh Bapat (#9)

1 attachment(s)

Re: 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
<Langote_Amit_f8@lab.ntt.co.jp> 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

Attachments:

0001-Add-some-FDW-HANDLER-DDL-tests.patchtext/plain; charset=UTF-8; name=0001-Add-some-FDW-HANDLER-DDL-tests.patchDownload

From 510987531bfdf22df0bc8eef27f232e580d415b1 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
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 
-------------+---------------------------+---------+--------------------------+-------------------+------------------------------+-------------
- 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/fdw_handler.source b/src/test/regress/input/fdw_handler.source
new file mode 100644
index 0000000000..103ae6e8b9
--- /dev/null
+++ b/src/test/regress/input/fdw_handler.source
@@ -0,0 +1,5 @@
+--
+-- For FDW handler tests in sql/foreign_data.sql
+--
+CREATE FUNCTION test_fdw_handler () RETURNS fdw_handler LANGUAGE C
+    AS '@libdir@/regress@DLSUFFIX@', 'test_fdw_handler';
diff --git a/src/test/regress/output/fdw_handler.source b/src/test/regress/output/fdw_handler.source
new file mode 100644
index 0000000000..c9d0f7de6f
--- /dev/null
+++ b/src/test/regress/output/fdw_handler.source
@@ -0,0 +1,5 @@
+--
+-- For FDW handler tests in sql/foreign_data.sql
+--
+CREATE FUNCTION test_fdw_handler () RETURNS fdw_handler LANGUAGE C
+    AS '/home/amit/pg/mygit/src/test/regress/regress.so', 'test_fdw_handler';
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index 2fd3f2b1b1..f8361c49ce 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -89,7 +89,7 @@ test: brin gin gist spgist privileges init_privs security_label collate matview
 # ----------
 # Another group of parallel tests
 # ----------
-test: alter_generic alter_operator misc psql async dbsize misc_functions sysviews tsrf tidscan stats_ext
+test: alter_generic alter_operator misc psql async dbsize misc_functions sysviews tsrf tidscan stats_ext fdw_handler
 
 # rules cannot run concurrently with any test that creates a view
 test: rules psql_crosstab amutils
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index 734947cc98..0a123f2b39 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -1096,3 +1096,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/serial_schedule b/src/test/regress/serial_schedule
index 76b0de30a7..b4bb866616 100644
--- a/src/test/regress/serial_schedule
+++ b/src/test/regress/serial_schedule
@@ -147,6 +147,7 @@ test: bitmapops
 test: combocid
 test: tsearch
 test: tsdicts
+test: fdw_handler
 test: foreign_data
 test: window
 test: xmlmap
diff --git a/src/test/regress/sql/.gitignore b/src/test/regress/sql/.gitignore
index 46c8112094..21b5a62dd5 100644
--- a/src/test/regress/sql/.gitignore
+++ b/src/test/regress/sql/.gitignore
@@ -2,6 +2,7 @@
 /copy.sql
 /create_function_1.sql
 /create_function_2.sql
+/fdw_handler.sql
 /largeobject.sql
 /misc.sql
 /security_label.sql
diff --git a/src/test/regress/sql/foreign_data.sql b/src/test/regress/sql/foreign_data.sql
index ebe8ffbffe..1af7258718 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_fdw_handler HANDLER invalid_fdw_handler;  -- ERROR
+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
 ALTER FOREIGN DATA WRAPPER foo VALIDATOR bar;               -- ERROR
@@ -88,6 +95,12 @@ ALTER FOREIGN DATA WRAPPER foo RENAME TO foo1;
 \dew+
 ALTER FOREIGN DATA WRAPPER foo1 RENAME TO foo;
 
+-- HANDLER related checks
+ALTER FOREIGN DATA WRAPPER foo HANDLER invalid_fdw_handler;  -- ERROR
+ALTER FOREIGN DATA WRAPPER foo HANDLER test_fdw_handler HANDLER anything;  -- ERROR
+ALTER FOREIGN DATA WRAPPER foo HANDLER test_fdw_handler;
+DROP FUNCTION invalid_fdw_handler();
+
 -- DROP FOREIGN DATA WRAPPER
 DROP FOREIGN DATA WRAPPER nonexistent;                      -- ERROR
 DROP FOREIGN DATA WRAPPER IF EXISTS nonexistent;
-- 
2.11.0

#11

Ashutosh Bapat

ashutosh.bapat@enterprisedb.com

over 8 years ago

In reply to: Amit Langote (#10)

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

On Wed, Sep 13, 2017 at 7:49 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

On 2017/09/12 20:17, Ashutosh Bapat wrote:

On Tue, Sep 12, 2017 at 2:27 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> 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

#12

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

over 8 years ago

In reply to: Ashutosh Bapat (#11)

Re: 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

#13

Ashutosh Bapat

ashutosh.bapat@enterprisedb.com

over 8 years ago

In reply to: Amit Langote (#12)

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

On Wed, Sep 13, 2017 at 1:27 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> 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

#14

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

over 8 years ago

In reply to: Ashutosh Bapat (#13)

1 attachment(s)

Re: 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
<Langote_Amit_f8@lab.ntt.co.jp> 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

Attachments:

0001-Add-some-FDW-HANDLER-DDL-tests.patchtext/plain; charset=UTF-8; name=0001-Add-some-FDW-HANDLER-DDL-tests.patchDownload

From d0c28965b892ac76d83987e9bf35e8ab8fd62e11 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
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 |                   |                              | 
+                                                             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..cde78eb1a0 100644
--- a/src/test/regress/input/create_function_1.source
+++ b/src/test/regress/input/create_function_1.source
@@ -62,6 +62,12 @@ CREATE FUNCTION test_atomic_ops()
     AS '@libdir@/regress@DLSUFFIX@'
     LANGUAGE C;
 
+-- Tests creating a FDW handler
+CREATE FUNCTION test_fdw_handler()
+    RETURNS fdw_handler
+    AS '@libdir@/regress@DLSUFFIX@', 'test_fdw_handler'
+    LANGUAGE C;
+
 -- Things that shouldn't work:
 
 CREATE FUNCTION test1 (int) RETURNS int LANGUAGE SQL
diff --git a/src/test/regress/output/create_function_1.source b/src/test/regress/output/create_function_1.source
index 957595c51e..ab601be375 100644
--- a/src/test/regress/output/create_function_1.source
+++ b/src/test/regress/output/create_function_1.source
@@ -55,6 +55,11 @@ CREATE FUNCTION test_atomic_ops()
     RETURNS bool
     AS '@libdir@/regress@DLSUFFIX@'
     LANGUAGE C;
+-- Tests creating a FDW handler
+CREATE FUNCTION test_fdw_handler()
+    RETURNS fdw_handler
+    AS '@libdir@/regress@DLSUFFIX@', 'test_fdw_handler'
+    LANGUAGE C;
 -- Things that shouldn't work:
 CREATE FUNCTION test1 (int) RETURNS int LANGUAGE SQL
     AS 'SELECT ''not an integer'';';
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index 734947cc98..0a123f2b39 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -1096,3 +1096,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 ebe8ffbffe..1af7258718 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_fdw_handler HANDLER invalid_fdw_handler;  -- ERROR
+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
 ALTER FOREIGN DATA WRAPPER foo VALIDATOR bar;               -- ERROR
@@ -88,6 +95,12 @@ ALTER FOREIGN DATA WRAPPER foo RENAME TO foo1;
 \dew+
 ALTER FOREIGN DATA WRAPPER foo1 RENAME TO foo;
 
+-- HANDLER related checks
+ALTER FOREIGN DATA WRAPPER foo HANDLER invalid_fdw_handler;  -- ERROR
+ALTER FOREIGN DATA WRAPPER foo HANDLER test_fdw_handler HANDLER anything;  -- ERROR
+ALTER FOREIGN DATA WRAPPER foo HANDLER test_fdw_handler;
+DROP FUNCTION invalid_fdw_handler();
+
 -- DROP FOREIGN DATA WRAPPER
 DROP FOREIGN DATA WRAPPER nonexistent;                      -- ERROR
 DROP FOREIGN DATA WRAPPER IF EXISTS nonexistent;
-- 
2.11.0

#15

Ashutosh Bapat

ashutosh.bapat@enterprisedb.com

over 8 years ago

In reply to: Amit Langote (#14)

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

On Wed, Sep 13, 2017 at 1:46 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> 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

#16

Robert Haas

robertmhaas@gmail.com

over 8 years ago

In reply to: Ashutosh Bapat (#15)

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

On Thu, Sep 14, 2017 at 8:30 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> 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

#17

Amit Langote

amitlangote09@gmail.com

over 8 years ago

In reply to: Robert Haas (#16)

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

On Fri, Sep 15, 2017 at 9:20 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Sep 14, 2017 at 8:30 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> 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