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(at)lab(dot)ntt(dot)co(dot)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(at)lab(dot)ntt(dot)co(dot)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