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

From: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
To: Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>
Cc: Mark Dilger <hornschnorter(at)gmail(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...
Date: 2017-09-13 02:19:56
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: 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(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.

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


Attachment Content-Type Size
0001-Add-some-FDW-HANDLER-DDL-tests.patch text/plain 8.3 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Kyotaro HORIGUCHI 2017-09-13 02:25:43 Re: Race between SELECT and ALTER TABLE NO INHERIT
Previous Message Simon Riggs 2017-09-13 02:04:05 Re: generated columns