On Wed, Sep 13, 2017 at 7:49 AM, Amit Langote <langote_amit...@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...@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
