Re: FDW for PostgreSQL

Kaigai-san,

Thanks for the review.

On Thu, Oct 4, 2012 at 6:10 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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