Обсуждение: [HACKERS] no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...

Hackers,

just FYI, I cannot find any regression test coverage of this part of the grammar, not
even in the contrib/ directory or TAP tests.  I was going to submit a patch to help out,
and discovered it is not so easy to do, and perhaps that is why the coverage is missing.

My apologies for the noise if this is already common knowledge.  Also, if I'm wrong,
I'd appreciate a pointer to the test that I am failing to find.

Thanks,

Mark Dilger

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

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

> 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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

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

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

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