Re: pgsql_fdw in contrib

Harada-san,

I checked your patch, and had an impression that includes many
improvements from the previous revision that I looked at the last
commit fest.

However, I noticed several points to be revised, or investigated.

* It seems to me expected results of the regression test is not attached, even though test cases were included. Please
addit. 

* 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:

postgres=# BEGIN;
BEGIN
postgres=# SELECT * FROM ft3;a |  b
---+-----1 | aaa2 | bbb3 | ccc4 | ddd5 | eee6 | fff
(6 rows)

postgres=# SAVEPOINT sv_01;
SAVEPOINT

postgres=# SELECT * FROM ft3 WHERE 1 / (a - 4) > 0;  -- should be
error on remote transaction
ERROR:  could not execute remote query
DETAIL:  ERROR:  division by zero

HINT:  SELECT a, b FROM public.t1 WHERE (((1 OPERATOR(pg_catalog./) (a
OPERATOR(pg_catalog.-) 4)) OPERATOR(pg_catalog.>) 0))

postgres=# ROLLBACK TO SAVEPOINT sv_01;
ROLLBACK

postgres=# SELECT * FROM ft3;
ERROR:  could not execute EXPLAIN for cost estimation
DETAIL:  ERROR:  current transaction is aborted, commands ignored
until end of transaction block

HINT:  SELECT a, b FROM public.t1
 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
beingalready 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
localerror. 
 * Regarding to deparseSimpleSql(), it pulls attributes being referenced   from baserestrictinfo and reltargetlist
usingpull_var_clause().   Is it unavailable to use local_conds instead of baserestrictinfo?   We can optimize
referencesto the variable being consumed at the   remote side only. All we need to transfer is variables referenced
intargetlist and local-clauses.   In addition, is pull_var_clause() reasonable to list up all the attribute
referencedat the both expression tree? It seems to be pull_varattnos()   is more useful API in this situation. 
 * Regarding to deparseRelation(), it scans simple_rte_array to fetch   RangeTblEntry with relation-id of the target
foreigntable.   It does not match correct entry if same foreign table is appeared in   this list twice or more, like a
caseof self-join. I'd like to recommend   to use simple_rte_array[baserel->relid] instead.   In addition, it checks
whetherrelkind is RELKIND_FOREIGN_TABLE,   or not. It seems to me this check should be Assert(), if routines of
pgsql_fdwis 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()?

Thanks for your great jobs.

2012/6/14 Shigeru HANADA <shigeru.hanada@gmail.com>:
> I'd like to propose pgsql_fdw, FDW for PostgreSQL, as a contrib module
> in core, again.  This patch is basically rebased version of the patches
> proposed in 9.2 development cycle, and contains some additional changes
> such as concern about volatile variables for PG_CATCH blocks.  In
> addition, I combined old three patches (pgsql_fdw core functionality,
> push-down support, and analyze support) into one patch for ease of
> review. (I think these features are necessary for pgsql_fdw use.)
>
> After the development for 9.2 cycle, pgsql_fdw can gather statistics
> from remote side by providing sampling function to analyze module in
> backend core, use them to estimate selectivity of WHERE clauses which
> will be pushed-down, and retrieve query results from remote side with
> custom row processor which prevents memory exhaustion from huge result set.
>
> It would be possible to add some more features, such as ORDER BY
> push-down with index information support, without changing existing
> APIs, but at first add relatively simple pgsql_fdw and enhance it seems
> better.  In addition, once pgsql_fdw has been merged, it would help
> implementing proof-of-concept of SQL/MED-related features.
>
> Regards,
> --
> Shigeru HANADA
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>



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