Re: FDW for PostgreSQL - Mailing list pgsql-hackers

Kaigai-san,

Thanks for the review.

On Thu, Oct 4, 2012 at 6:10 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
> At the postgresql_fdw/deparse.c,
>
> * Even though deparseVar() is never invoked with need_prefix=true,
>   I doubt why Var reference needs to be qualified with relation alias.
>   It seems to me relation alias is never used in the remote query,
>   so isn't it a possible bug?

This must be a remaining of my effort against supporting JOIN-push-down
(it is one of future improvements).  At the moment it is not clear what
should be used as column prefix, so I removed need_prefix parameter to
avoid possible confusion.  I removed need_prefix from deparseRelation as
well.

> * deparseFuncExpr() has case handling depending on funcformat
>   of FuncExpr. I think all the cases can be deparsed using explicit
>   function call, and it can avoid a trouble when remote host has
>   inconsistent cast configuration.

Hm, your point is that specifying underlying function, e.g.
"cast_func(value)", is better than simple cast notation,e.g.
"value::type" and "CAST(value AS type)", because such explicit statement
prevents possible problems caused by difference of cast configuration,
right?  If so, I agree about explicit casts.  I'm not sure about
implicit casts because it seems difficult to avoid unexpected implicit
cast at all.

As background, I just followed the logic implemented in ruleutils.c for
FuncExpr, which deparses explicit cast in format of 'value::type'.  If
it's sure that FuncExpr comes from cast never takes arguments more than
one, we can go your way.  I'll check it.

> At the postgresql_fdw/connection.c,
>
> * I'm worry about the condition for invocation of begin_remote_tx().
>   + if (use_tx && entry->refs == 1)
>   +    begin_remote_tx(entry->conn);
>   + entry->use_tx = use_tx;
>   My preference is: if (use_tx && !entry->use_tx), instead.
>   Even though here is no code path to make a problem obvious,
>   it may cause possible difficult-to-find bug, in case when caller
>   tried to get a connection being already acquired by someone
>   but no transaction needed.

I got it.  In addition, I fixed ReleaseConnection to call
abort_remote_tx after decrementing reference counter, as GetConnection
does for begin_remote_tx.

> At the postgresql_fdw/postgresql_fdw.c,
>
> * When pgsqlGetForeignPaths() add SQL statement into
>   fdw_private, it is implemented as:
>   +     /* Construct list of SQL statements and bind it with the path. */
>   +     fdw_private = lappend(NIL, makeString(fpstate->sql.data));
>   Could you use list_make1() instead?

Fixed.

>
> * At the bottom half of query_row_processor(), I found these
>   mysterious two lines.
>             MemoryContextSwitchTo(festate->scan_cxt);
>             MemoryContextSwitchTo(festate->temp_cxt);
>   Why not switch temp_cxt directly?

It must be a copy-and-paste mistake.  Removed both.

>
> At the sgml/postgresql-fdw.sgml,
>
> * Please add this version does not support sub-transaction handling.
>   Especially, all we can do is abort top-level transaction in case when
>   an error is occurred at the remote side within sub-transaction.

I don't think that abort of local top-level transaction is not necessary
in such case, because now connection_cleanup() closes remote connection
whenever remote error occurs in sub-transactions.  For instance, we can
recover from remote syntax error (it could easily happen from wrong
relname setting) by ROLLBACK.  Am I missing something?

$ ALTER FOREIGN TABLE foo OPTIONS (SET relname 'invalid');
$ BEGIN;                        -- explicit transaction
$ SAVEPOINT a;                  -- begin sub-transaction
$ SELECT * FROM foo;            -- this causes remote error, then remote
                                -- connection is closed automatically
$ ROLLBACK TO a;                -- clears local error state
$ SELECT 1;                     -- this should be successfully executed

> I hope to take over this patch for committer soon.

I hope so too :)
Please examine attached v2 patch (note that is should be applied onto
latest dblink_fdw_validator patch).

Regards,
--
Shigeru HANADA