Re: pgsql_fdw in contrib

Hi Kaigai-san,

Sorry for delayed reply.

On Tue, Jun 26, 2012 at 10:50 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> * It seems to me expected results of the regression test is not
> attached, even though test cases were included. Please add it.

AFAICS the patch I posted contains both test script and expected result.
If you have created new git branch and applied patch, you might have
forgotten to git-add expected/pgsql_fdw.out. Would you check that?

> * cleanup_connection() does not close the connection in case
> when this callback was invoked due to an error under sub-
> transaction. It probably makes problematic behavior.
>
> See the following steps to reproduce the matter:

Oops, good catch. I could reproduce this problem.

> Once we got an error under the remote transaction, it eventually raises
> an error on local side, then cleanup_connection() should be invoked.
> But it ignores the error due to sub-transaction, thus, the remote transaction
> being already aborted is kept.
> I'd like to suggest two remedy.
> 1. connections are closed, even if errors on sub-transaction.
> 2. close the connection if PQexecParams() returns an error,
> on execute_query() prior to raise a local error.

Background: the reason why pgsql_fdw doesn't disconnect at errors in
sub-transaction is to allow FETCHing from cursor which uses foreign
table and is opened before the local error.

How about to close connection when released connection is in failed
transaction? It can be determined with PQtransactionStatus() in
ReleaseConnection().

> * Regarding to deparseSimpleSql(), it pulls attributes being referenced
> from baserestrictinfo and reltargetlist using pull_var_clause().
> Is it unavailable to use local_conds instead of baserestrictinfo?
> We can optimize references to the variable being consumed at the
> remote side only. All we need to transfer is variables referenced
> in targetlist and local-clauses.

It sounds possible.

> In addition, is pull_var_clause() reasonable to list up all the attribute
> referenced at the both expression tree? It seems to be pull_varattnos()
> is more useful API in this situation.

Oh, I didn't know that. It seems more efficient, I'll check it out.

> * Regarding to deparseRelation(), it scans simple_rte_array to fetch
> RangeTblEntry with relation-id of the target foreign table.
> It does not match correct entry if same foreign table is appeared in
> this list twice or more, like a case of self-join. I'd like to recommend
> to use simple_rte_array[baserel->relid] instead.
> In addition, it checks whether relkind is RELKIND_FOREIGN_TABLE,
> or not. It seems to me this check should be Assert(), if routines of
> pgsql_fdw is called towards regular relations.
>
> * Regarding to deparseVar(), is it unnecessary to check relkind of
> the relation being referenced by Var node, isn't it?
>
> * Regarding to deparseBoolExpr(), compiler raised a warning
> because op can be used without initialized.
>
> * Even though it is harmless, sortConditions() is a misleading function
> name. How about categolizeConditions() or screeningConditions()?

I'll reply these comments in another post.

Regards,
--
Shigeru Hanada