Re: FDW for PostgreSQL

Hanada-san,

I tried to check this patch. Because we also had some discussion
on this extension through the last two commit fests, I have no
fundamental design arguments.

So, let me drop in the implementation detail of this patch.

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?

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

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.

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?

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

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 hope to take over this patch for committer soon.

Thanks,

2012/9/14 Shigeru HANADA <shigeru(dot)hanada(at)gmail(dot)com>:
> Hi all,
>
> I'd like to propose FDW for PostgreSQL as a contrib module again.
> Attached patch is updated version of the patch proposed in 9.2
> development cycle.
>
> For ease of review, I summarized what the patch tries to achieve.
>
> Abstract
> ========
> This patch provides FDW for PostgreSQL which allows users to access
> external data stored in remote PostgreSQL via foreign tables. Of course
> external instance can be beyond network. And I think that this FDW
> could be an example of other RDBMS-based FDW, and it would be useful for
> proof-of-concept of FDW-related features.
>
> Note that the name has been changed from "pgsql_fdw" which was used in
> last proposal, since I got a comment which says that most of existing
> FDWs have name "${PRODUCT_NAME}_fdw" so "postgresql_fdw" or
> "postgres_fdw" would be better. For this issue, I posted another patch
> which moves existing postgresql_fdw_validator into contrib/dblink with
> renaming in order to reserve the name "postgresql_fdw" for this FDW.
> Please note that the attached patch requires dblink_fdw_validator.patch
> to be applied first.
> http://archives.postgresql.org/pgsql-hackers/2012-09/msg00454.php
>
> Query deparser
> ==============
> Now postgresql_fdw has its own SQL query deparser inside, so it's free
> from backend's ruleutils module.
>
> This deparser maps object names when generic options below were set.
>
> nspname of foreign table: used as namespace (schema) of relation
> relname of foreign table: used as relation name
> colname of foreign column: used as column name
>
> This mapping allows flexible schema design.
>
> SELECT optimization
> ===================
> postgresql_fdw always retrieves as much columns as foreign table from
> remote to avoid overhead of column mapping. However, often some of them
> (or sometimes all of them) are not used on local side, so postgresql_fdw
> uses NULL literal as such unused columns in SELECT clause of remote
> query. For example, let's assume one of pgbench workloads:
>
> SELECT abalance FROM pgbench_accounts WHERE aid = 1;
>
> This query generates a remote query below. In addition to bid and
> filler, aid is replaced with NULL because it's already evaluated on
> remote side.
>
> SELECT NULL, NULL, abalance, NULL FROM pgbench_accounts
> WHERE (aid OPERATOR(pg_catalog.=) 1);
>
> This trick would improve performance notably by reducing amount of data
> to be transferred.
>
> One more example. Let's assume counting rows.
>
> SELCT count(*) FROM pgbench_accounts;
>
> This query requires only existence of row, so no actual column reference
> is in SELECT clause.
>
> SELECT NULL, NULL, NULL, NULL FROM pgbench_accounts;
>
> WHERE push down
> ===============
> postgresql_fdw pushes down some of restrictions (IOW, top level elements
> in WHERE clause which are connected with AND) which can be evaluated on
> remote side safely. Currently the criteria "safe" is declared as
> whether an expression contains only:
> - column reference
> - constant of bult-in type (scalar and array)
> - external parameter of EXECUTE statement
> - built-in operator which uses built-in immutable function
> (operator cannot be collative unless it's "=" or "<>")
> - built-in immutable function
>
> Some other elements might be also safe to be pushed down, but criteria
> above seems enough for basic use cases.
>
> Although it might seem odd, but operators are deparsed into OPERATOR
> notation to avoid search_path problem.
> E.g.
> local query : WHERE col = 1
> remote query: WHERE (col OPERATOR(pg_catalog.=) 1)
>
> Connection management
> =====================
> postgresql_fdw has its own connection manager. Connection is
> established when first foreign scan on a server is planned, and it's
> pooled in the backend. If another foreign scan on same server is
> invoked, same connection will be used. Connection pool is per-backend.
> This means that different backends never share connection.
>
> postgresql_fdw_connections view shows active connections, and
> postgresql_fdw_disconnect() allows users to discard particular
> connection at arbitrary timing.
>
> Transaction management
> ======================
> If multiple foreign tables on same foreign server is used in a local
> query, postgresql_fdw uses same connection to retrieve results in a
> transaction to make results consistent. Currently remote transaction is
> closed at the end of local query, so following local query might produce
> inconsistent result.
>
> Costs estimation
> ================
> To estimate costs and result rows of a foreign scan, postgresql_fdw
> executes EXPLAIN statement on remote side, and retrieves costs and rows
> values from the result. For cost estimation, cost of connection
> establishment and data transfer are added to the base costs. Currently
> these two factors is hard-coded, but making them configurable is not so
> difficult.
>
> Executing EXPLAIN is not cheap, but remote query itself is usually very
> expensive, so such additional cost would be acceptable.
>
> ANALYZE support
> ===============
> postgresql_fdw supports ANALYZE to improve selectivity estimation of
> filtering done on local side (WHERE clauses which could not been pushed
> down. The sampler function retrieves all rows from remote table and
> skip some of them so that result fits requested size. As same as
> file_fdw, postgresql_fdw doesn't care order of result, because it's
> important for only correlation, and correlation is important for only
> index scans, which is not supported for this FDW.
>
> Fetching Data
> =============
> postgresql_fdw uses single-row mode of libpq so that memory usage is
> kept in low level even if the result is huge.
>
> To cope with difference of encoding, postgresql_fdw automatically sets
> client_encoding to server encoding of local database.
>
> Future improvement
> ==================
> I have some ideas for improvement:
> - Provide sorted result path (requires index information?)
> - Provide parameterized path
> - Transaction mapping between local and remotes (2PC)
> - binary transfer (only against servers with same PG major version?)
> - JOIN push-down (requires support by core)
>
> Any comments and questions are welcome.
> --
> Shigeru HANADA
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>