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.hanada@gmail.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@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

On Fri, Sep 14, 2012 at 9:25 AM, Shigeru HANADA
<shigeru.hanada@gmail.com> wrote:

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.

very nice.

- binary transfer (only against servers with same PG major version?)

Unfortunately this is not enough -- at least not without some
additional work. The main problem is user defined types, especially
composites. Binary wire format sends internal member oids which the
receiving server will have to interpret.

merlin

Kaigai-san,

Thanks for the review.

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

Hi Hanada-san,

Please examine attached v2 patch (note that is should be applied onto
latest dblink_fdw_validator patch).

I've reviewed your patch quickly. I noticed that the patch has been created in
a slightly different way from the guidelines:
http://www.postgresql.org/docs/devel/static/fdw-planning.html The guidelines
say the following, but the patch identifies the clauses in
baserel->baserestrictinfo in GetForeignRelSize, not GetForeignPaths. Also, it
has been implemented so that most sub_expressions are evaluated at the remote
end, not the local end, though I'm missing something. For postgresql_fdw to be
a good reference for FDW developers, ISTM it would be better that it be
consistent with the guidelines. I think it would be needed to update the
following document or redesign the function to be consistent with the following
document.

As an example, the FDW might identify some restriction clauses of the form
foreign_variable= sub_expression, which it determines can be executed on the
remote server given the locally-evaluated value of the sub_expression. The
actual identification of such a clause should happen during GetForeignPaths,
since it would affect the cost estimate for the path.

Thanks,

Best regards,
Etsuro Fujita

2012/10/11 Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>:

Hi Hanada-san,

Please examine attached v2 patch (note that is should be applied onto
latest dblink_fdw_validator patch).

I've reviewed your patch quickly. I noticed that the patch has been created in
a slightly different way from the guidelines:
http://www.postgresql.org/docs/devel/static/fdw-planning.html The guidelines
say the following, but the patch identifies the clauses in
baserel->baserestrictinfo in GetForeignRelSize, not GetForeignPaths. Also, it
has been implemented so that most sub_expressions are evaluated at the remote
end, not the local end, though I'm missing something. For postgresql_fdw to be
a good reference for FDW developers, ISTM it would be better that it be
consistent with the guidelines. I think it would be needed to update the
following document or redesign the function to be consistent with the following
document.

Hmm. It seems to me Fujita-san's comment is right.

Even though the latest implementation gets an estimated number of rows
using EXPLAIN with qualified SELECT statement on remote side, then, it is
adjusted with selectivity of local qualifiers, we shall be able to obtain same
cost estimation because postgresql_fdw assumes all the pushed-down
qualifiers are built-in only.

So, is it available to move classifyConditions() to pgsqlGetForeignPlan(),
then, separate remote qualifiers and local ones here?
If we call get_remote_estimate() without WHERE clause, remote side
will give just whole number of rows of remote table. Then, it can be adjusted
with selectivity of "all" the RestrictInfo (including both remote and local).

Sorry, I should suggest it at the beginning.

This change may affects the optimization that obtains remote columns
being in-use at local side. Let me suggest an expression walker that
sets member of BitmapSet for columns in-use at local side or target list.
Then, we can list up them on the target list of the remote query.

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

Shigeru HANADA escribió:

Please examine attached v2 patch (note that is should be applied onto
latest dblink_fdw_validator patch).

Tom committed parts of the dblink_fdw_validator patch, but not the
removal, so it seems this patch needs to be rebased on top of that
somehow. I am not able to say what's the best resolution for that
conflict, however. But it seems to me that maybe you will need to
choose a different name for the validator after all, to support binary
upgrades.

There are some other comments downthread that a followup patch probably
needs to address too, as well. I am marking this patch Returned with
Feedback. Please submit an updated version to CF3.

Thanks.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Sorry for delayed response.

On Sun, Oct 21, 2012 at 3:16 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

2012/10/11 Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>:

I've reviewed your patch quickly. I noticed that the patch has been created in
a slightly different way from the guidelines:
http://www.postgresql.org/docs/devel/static/fdw-planning.html The guidelines
say the following, but the patch identifies the clauses in
baserel->baserestrictinfo in GetForeignRelSize, not GetForeignPaths. Also, it
has been implemented so that most sub_expressions are evaluated at the remote
end, not the local end, though I'm missing something. For postgresql_fdw to be
a good reference for FDW developers, ISTM it would be better that it be
consistent with the guidelines. I think it would be needed to update the
following document or redesign the function to be consistent with the following
document.

Hmm. It seems to me Fujita-san's comment is right.

Indeed postgresql_fdw touches baserestrictinfo in GetForeignRelSize, but
it's because of optimization for better width estimate. The doc
Fujita-san pointed says that:

The actual identification of such a clause should happen during
GetForeignPaths, since it would affect the cost estimate for the
path.

I understood this description says that "you need to touch baserestrict
info *before* GetForeignPlan to estimate costs of optimized path". I
don't feel that this description prohibits FDW to touch baserestrictinfo
in GetForeignRelSize, but mentioning it clearly might be better.

Regards,
--
Shigeru HANADA

2012/11/6 Shigeru HANADA <shigeru.hanada@gmail.com>:

Sorry for delayed response.

On Sun, Oct 21, 2012 at 3:16 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

2012/10/11 Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>:

I've reviewed your patch quickly. I noticed that the patch has been
created in
a slightly different way from the guidelines:
http://www.postgresql.org/docs/devel/static/fdw-planning.html The
guidelines
say the following, but the patch identifies the clauses in
baserel->baserestrictinfo in GetForeignRelSize, not GetForeignPaths.
Also, it
has been implemented so that most sub_expressions are evaluated at the
remote
end, not the local end, though I'm missing something. For postgresql_fdw
to be
a good reference for FDW developers, ISTM it would be better that it be
consistent with the guidelines. I think it would be needed to update the
following document or redesign the function to be consistent with the
following
document.

Hmm. It seems to me Fujita-san's comment is right.

Indeed postgresql_fdw touches baserestrictinfo in GetForeignRelSize, but
it's because of optimization for better width estimate. The doc Fujita-san
pointed says that:

The actual identification of such a clause should happen during
GetForeignPaths, since it would affect the cost estimate for the
path.

I understood this description says that "you need to touch baserestrict info
*before* GetForeignPlan to estimate costs of optimized path". I don't feel
that this description prohibits FDW to touch baserestrictinfo in
GetForeignRelSize, but mentioning it clearly might be better.

Hanada-san,

Isn't it possible to pick-up only columns to be used in targetlist or
local qualifiers,
without modification of baserestrictinfo?
Unless we put WHERE clause on EXPLAIN statement for cost estimation on
GetForeignRelSize, all we have to know is list of columns to be fetched using
underlying queries. Once we construct a SELECT statement without WHERE
clause on GetForeignRelSize stage, it is never difficult to append it later
according to the same criteria being implemented at classifyConditions.

I'd like to see committer's opinion here.
Please give Hanada-san some directions.
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

Kohei KaiGai <kaigai@kaigai.gr.jp> writes:

2012/11/6 Shigeru HANADA <shigeru.hanada@gmail.com>:

Indeed postgresql_fdw touches baserestrictinfo in GetForeignRelSize, but
it's because of optimization for better width estimate. The doc Fujita-san
pointed says that:

The actual identification of such a clause should happen during
GetForeignPaths, since it would affect the cost estimate for the
path.

I understood this description says that "you need to touch baserestrict info
*before* GetForeignPlan to estimate costs of optimized path". I don't feel
that this description prohibits FDW to touch baserestrictinfo in
GetForeignRelSize, but mentioning it clearly might be better.

Isn't it possible to pick-up only columns to be used in targetlist or
local qualifiers, without modification of baserestrictinfo?

What the doc means to suggest is that you can look through the
baserestrictinfo list and then record information elsewhere about
interesting clauses you find. If the FDW is actually *modifying* that
list, I agree that seems like a bad idea. I don't recall anything in
the core system that does that, so it seems fragile. The closest
parallel I can think of in the core system is indexscans pulling out
restriction clauses to use as index quals. That code doesn't modify
the baserestrictinfo list, only make new lists with some of the same
entries.

regards, tom lane

On 2012/11/07, at 1:35, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Isn't it possible to pick-up only columns to be used in targetlist or
local qualifiers, without modification of baserestrictinfo?

What the doc means to suggest is that you can look through the
baserestrictinfo list and then record information elsewhere about
interesting clauses you find. If the FDW is actually *modifying* that
list, I agree that seems like a bad idea.

Kaigai-san might have misunderstood that postgresql_fdw changes
baserestrictinfo, since it did so in old implementation.

ClassifyConditions creates new lists, local_conds and remote_conds,
which have cells which point RestrictInfo(s) in baserestrictinfo.
It doesn't copy RestrictInfo for new lists, but I think it's ok
because baserestrictinfo list itself and RestrictInfo(s) pointed by
it are never modified by postgresql_fdw.

I don't recall anything in
the core system that does that, so it seems fragile. The closest
parallel I can think of in the core system is indexscans pulling out
restriction clauses to use as index quals. That code doesn't modify
the baserestrictinfo list, only make new lists with some of the same
entries.

Thanks for the advise. I found relation_excluded_by_constraints
which is called by set_rel_size() creates new RestrictInfo list from
baserestrictinfo, and this seems like what postgresql_fdw does in
GetForeignRelSize, from the perspective of relation size estimation.

Regards,
--
Shigeru HANADA
shigeru.hanada@gmail.com

=?iso-2022-jp?B?GyRCMlZFRBsoQiAbJEJMUBsoQg==?= <shigeru.hanada@gmail.com> writes:

ClassifyConditions creates new lists, local_conds and remote_conds,
which have cells which point RestrictInfo(s) in baserestrictinfo.
It doesn't copy RestrictInfo for new lists, but I think it's ok
because baserestrictinfo list itself and RestrictInfo(s) pointed by
it are never modified by postgresql_fdw.

That's good. I think there are actually some assumptions that
RestrictInfo nodes are not copied once created. You can link them into
new lists all you want, but don't copy them.

regards, tom lane

Hi Kaigai-san,

Sorry for delayed response. I updated the patch, although I didn't change
any about timing issue you and Fujita-san concern.

1) add some FDW options for cost estimation. Default behavior is not
changed.
2) get rid of array of libpq option names, similary to recent change of
dblink
3) enhance document, especially remote query optimization
4) rename to postgres_fdw, to avoid naming conflict with the validator
which exists in core
5) cope with changes about error context handling

On Tue, Nov 6, 2012 at 7:36 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

Isn't it possible to pick-up only columns to be used in targetlist or
local qualifiers,
without modification of baserestrictinfo?

IMO, it's possible. postgres_fdw doesn't modify baserestrictinfo at all;
it just create two new lists which exclusively point RestrictInfo elements
in baserestrictinfo. Pulling vars up from conditions which can't be pushed
down would gives us list of necessary columns. Am I missing something?

--
Shigeru HANADA

2012/11/15 Shigeru Hanada <shigeru.hanada@gmail.com>:

Hi Kaigai-san,

Sorry for delayed response. I updated the patch, although I didn't change
any about timing issue you and Fujita-san concern.

1) add some FDW options for cost estimation. Default behavior is not
changed.
2) get rid of array of libpq option names, similary to recent change of
dblink
3) enhance document, especially remote query optimization
4) rename to postgres_fdw, to avoid naming conflict with the validator which
exists in core
5) cope with changes about error context handling

On Tue, Nov 6, 2012 at 7:36 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

Isn't it possible to pick-up only columns to be used in targetlist or
local qualifiers,
without modification of baserestrictinfo?

IMO, it's possible. postgres_fdw doesn't modify baserestrictinfo at all; it
just create two new lists which exclusively point RestrictInfo elements in
baserestrictinfo. Pulling vars up from conditions which can't be pushed
down would gives us list of necessary columns. Am I missing something?

Hanada-san,

Let me comments on several points randomly.

I also think the new "use_remote_explain" option is good. It works fine
when we try to use this fdw over the network with latency more or less.
It seems to me its default is "false", thus, GetForeignRelSize will return
the estimated cost according to ANALYZE, instead of remote EXPLAIN.
Even though you mention the default behavior was not changed, is it
an expected one? My preference is the current one, as is.

The deparseFuncExpr() still has case handling whether it is explicit case,
implicit cast or regular functions. If my previous proposition has no flaw,
could you fix up it using regular function invocation manner? In case when
remote node has incompatible implicit-cast definition, this logic can make
a problem.

At InitPostgresFdwOptions(), the source comment says we don't use
malloc() here for simplification of code. Hmm. I'm not sure why it is more
simple. It seems to me we have no reason why to avoid malloc here, even
though libpq options are acquired using malloc().

Regarding to the regression test.

[kaigai@iwashi sepgsql]$ cat contrib/postgres_fdw/regression.diffs
*** /home/kaigai/repo/sepgsql/contrib/postgres_fdw/expected/postgres_fdw.out
   Sat Nov 17 21:31:19 2012
--- /home/kaigai/repo/sepgsql/contrib/postgres_fdw/results/postgres_fdw.out
    Tue Nov 20 13:53:32 2012
***************
*** 621,627 ****
  -- ===================================================================
  ALTER FOREIGN TABLE ft1 ALTER COLUMN c5 TYPE int;
  SELECT * FROM ft1 WHERE c1 = 1;  -- ERROR
! ERROR:  invalid input syntax for integer: "1970-01-02 00:00:00"
  CONTEXT:  column c5 of foreign table ft1
  ALTER FOREIGN TABLE ft1 ALTER COLUMN c5 TYPE timestamp;
  -- ===================================================================
--- 621,627 ----
  -- ===================================================================
  ALTER FOREIGN TABLE ft1 ALTER COLUMN c5 TYPE int;
  SELECT * FROM ft1 WHERE c1 = 1;  -- ERROR
! ERROR:  invalid input syntax for integer: "Fri Jan 02 00:00:00 1970"
  CONTEXT:  column c5 of foreign table ft1
  ALTER FOREIGN TABLE ft1 ALTER COLUMN c5 TYPE timestamp;
  -- ===================================================================

======================================================================

I guess this test tries to check a case when remote column has incompatible
data type with local side. Please check timestamp_out(). Its output
format follows
"datestyle" setting of GUC, and it affects OS configuration on initdb time.
(Please grep "datestyle" at initdb.c !) I'd like to recommend to use
another data type
for this regression test to avoid false-positive detection.

Elsewhere, I could not find problems right now.

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

Thank for the comment!

On Tue, Nov 20, 2012 at 10:23 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

I also think the new "use_remote_explain" option is good. It works fine
when we try to use this fdw over the network with latency more or less.
It seems to me its default is "false", thus, GetForeignRelSize will return
the estimated cost according to ANALYZE, instead of remote EXPLAIN.
Even though you mention the default behavior was not changed, is it
an expected one? My preference is the current one, as is.

Oops, I must have focused on only cost factors.
I too think that using local statistics is better as default behavior,
because foreign tables would be used for relatively stable tables.
If target tables are updated often, it would cause problems about
consistency, unless we provide full-fledged transaction mapping.

The deparseFuncExpr() still has case handling whether it is explicit case,
implicit cast or regular functions. If my previous proposition has no flaw,
could you fix up it using regular function invocation manner? In case when
remote node has incompatible implicit-cast definition, this logic can make
a problem.

Sorry, I overlooked this issue. Fixed to use function call notation
for all of explicit function calls, explicit casts, and implicit casts.

At InitPostgresFdwOptions(), the source comment says we don't use

malloc() here for simplification of code. Hmm. I'm not sure why it is more
simple. It seems to me we have no reason why to avoid malloc here, even
though libpq options are acquired using malloc().

I used "simple" because using palloc avoids null-check and error handling.
However, many backend code use malloc to allocate memory which lives
as long as backend process itself, so I fixed.

Regarding to the regression test.

[snip]

I guess this test tries to check a case when remote column has incompatible
data type with local side. Please check timestamp_out(). Its output
format follows
"datestyle" setting of GUC, and it affects OS configuration on initdb time.
(Please grep "datestyle" at initdb.c !) I'd like to recommend to use
another data type
for this regression test to avoid false-positive detection.

Good catch. :)
I fixed the test to use another data type, user defined enum.

Regards,
--
Shigeru HANADA

2012/11/21 Shigeru Hanada <shigeru.hanada@gmail.com>:

Thank for the comment!

On Tue, Nov 20, 2012 at 10:23 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

I also think the new "use_remote_explain" option is good. It works fine
when we try to use this fdw over the network with latency more or less.
It seems to me its default is "false", thus, GetForeignRelSize will return
the estimated cost according to ANALYZE, instead of remote EXPLAIN.
Even though you mention the default behavior was not changed, is it
an expected one? My preference is the current one, as is.

Oops, I must have focused on only cost factors.
I too think that using local statistics is better as default behavior,
because foreign tables would be used for relatively stable tables.
If target tables are updated often, it would cause problems about
consistency, unless we provide full-fledged transaction mapping.

The deparseFuncExpr() still has case handling whether it is explicit case,
implicit cast or regular functions. If my previous proposition has no
flaw,
could you fix up it using regular function invocation manner? In case when
remote node has incompatible implicit-cast definition, this logic can make
a problem.

Sorry, I overlooked this issue. Fixed to use function call notation
for all of explicit function calls, explicit casts, and implicit casts.

At InitPostgresFdwOptions(), the source comment says we don't use
malloc() here for simplification of code. Hmm. I'm not sure why it is more
simple. It seems to me we have no reason why to avoid malloc here, even
though libpq options are acquired using malloc().

I used "simple" because using palloc avoids null-check and error handling.
However, many backend code use malloc to allocate memory which lives
as long as backend process itself, so I fixed.

Regarding to the regression test.

[snip]

I guess this test tries to check a case when remote column has
incompatible
data type with local side. Please check timestamp_out(). Its output
format follows
"datestyle" setting of GUC, and it affects OS configuration on initdb
time.
(Please grep "datestyle" at initdb.c !) I'd like to recommend to use
another data type
for this regression test to avoid false-positive detection.

Good catch. :)
I fixed the test to use another data type, user defined enum.

One other thing I noticed.

At execute_query(), it stores the retrieved rows onto tuplestore of
festate->tuples at once. Doesn't it make problems when remote-
table has very big number of rows?

IIRC, the previous code used cursor feature to fetch a set of rows
to avoid over-consumption of local memory. Do we have some
restriction if we fetch a certain number of rows with FETCH?
It seems to me, we can fetch 1000 rows for example, and tentatively
store them onto the tuplestore within one PG_TRY() block (so, no
need to worry about PQclear() timing), then we can fetch remote
rows again when IterateForeignScan reached end of tuplestore.

Please point out anything if I missed something.

Anyway, I'll check this v4 patch simultaneously.

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

On Wed, Nov 21, 2012 at 7:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

At execute_query(), it stores the retrieved rows onto tuplestore of
festate->tuples at once. Doesn't it make problems when remote-
table has very big number of rows?

No. postgres_fdw uses single-row processing mode of libpq when
retrieving query results in execute_query, so memory usage will
be stable at a certain level.

IIRC, the previous code used cursor feature to fetch a set of rows
to avoid over-consumption of local memory. Do we have some
restriction if we fetch a certain number of rows with FETCH?
It seems to me, we can fetch 1000 rows for example, and tentatively
store them onto the tuplestore within one PG_TRY() block (so, no
need to worry about PQclear() timing), then we can fetch remote
rows again when IterateForeignScan reached end of tuplestore.

As you say, postgres_fdw had used cursor to avoid possible memory
exhaust on large result set. I switched to single-row processing mode
(it could be said "protocol-level cursor"), which was added in 9.2,
because it accomplish same task with less SQL calls than cursor.

Regards,
--
Shigeru HANADA

After playing with some big SQLs for testing, I came to feel that
showing every remote query in EXPLAIN output is annoying, especially
when SELECT * is unfolded to long column list.

AFAIK no plan node shows so many information in a line, so I'm
inclined to make postgres_fdw to show it only when VERBOSE was
specified. This would make EXPLAIN output easy to read, even if many
foreign tables are used in a query.

Thoughts?

--
Shigeru HANADA

2012/11/22 Shigeru Hanada <shigeru.hanada@gmail.com>:

After playing with some big SQLs for testing, I came to feel that
showing every remote query in EXPLAIN output is annoying, especially
when SELECT * is unfolded to long column list.

AFAIK no plan node shows so many information in a line, so I'm
inclined to make postgres_fdw to show it only when VERBOSE was
specified. This would make EXPLAIN output easy to read, even if many
foreign tables are used in a query.

Thoughts?

Indeed, I also think it is reasonable solution.
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

Kohei KaiGai wrote:

2012/11/22 Shigeru Hanada <shigeru.hanada@gmail.com>:

After playing with some big SQLs for testing, I came to feel that
showing every remote query in EXPLAIN output is annoying, especially
when SELECT * is unfolded to long column list.

AFAIK no plan node shows so many information in a line, so I'm
inclined to make postgres_fdw to show it only when VERBOSE was
specified. This would make EXPLAIN output easy to read, even if many
foreign tables are used in a query.

Thoughts?

Indeed, I also think it is reasonable solution.

+1

That's the way I do it for oracle_fdw.

Yours,
Laurenz Albe