Re: pgsql_fdw, FDW for PostgreSQL server

Harada-san,

I checked the v9 patch, however, it still has some uncertain implementation.

[memory context of tuple store]
It calls tuplestore_begin_heap() under the memory context of
festate->scan_cxt at pgsqlBeginForeignScan.
On the other hand, tuplestore_gettupleslot() is called under the
memory context of festate->tuples.
I could not find a callback functions being invoked on errors,
so I doubt the memory objects acquired within tuplestore_begin_heap()
shall be leaked, even though it is my suggestion to create a sub-context
under the existing one.

In my opinion, it is a good choice to use es_query_cxt of the supplied EState.
What does prevent to apply this per-query memory context?

You mention about PGresult being malloc()'ed. However, it seems to me
fetch_result() and store_result() once copy the contents on malloc()'ed
area to the palloc()'ed area, and PQresult is released on an error using
PG_TRY() ... PG_CATCH() block.

[Minor comments]
Please set NULL to "sql" variable at begin_remote_tx().
Compiler raises a warnning due to references of uninitialized variable,
even though the code path never run.

It potentially causes a problem in case when fetch_result() raises an
error because of unexpected status (!= PGRES_TUPLES_OK).
One code path is not protected with PG_TRY(), and other code path
will call PQclear towards already released PQresult.

Although it is just a preference of mine, is the exprFunction necessary?
It seems to me, the point of push-down check is whether the supplied
node is built-in object, or not. So, an sufficient check is is_builtin() onto
FuncExpr->funcid, OpExpr->opno, ScalarArrayOpExpr->opno and so on.
It does not depend on whether the function implementing these nodes
are built-in or not.

Thanks,

2012年2月14日9:09 Shigeru Hanada <shigeru.hanada@gmail.com>:
> (2012/02/14 15:15), Shigeru Hanada wrote:
>> Good catch, thanks.  I'll revise pgsql_fdw tests little more.
>
> Here are the updated patches.  In addition to Fujita-san's comment, I
> moved DROP OPERATOR statements to clean up section of test script.
>
> Regards,
> --
> Shigeru Hanada



-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>