Re: pgsql_fdw, FDW for PostgreSQL server

I'd like to propose pgsql_fdw, FDW for external PostgreSQL server, as a
contrib module. I think that this module would be the basis of further
SQL/MED development for core, e.g. join-push-down and ANALYZE support.

I attached three patches for this new FDW. They should be applied in
the order below. I separated these patches so that first (or first two)
can be committed separately.

* fdw_helper_doc.patch provides documents for FDW developers about
helper functions existing in 9.1, so this can be back-patched.
* fdw_helper_funcs.patch provides additional helper functions which
would make manipulation of FDW options easier.
* pgsql_fdw.patch provides new FDW for external PG server.

Here are details of pgsql_fdw.

Name of the wrapper
===================
I used the name "pgsql_fdw" for the wrapper and its derivatives. I
think it would be better to leave contrib/dblink and built-in
postgresql_fdw_validator for backward compatibility, and use new name
for new wrapper.

Or, it might be OK to rename postgresql_fdw_validator to
dblink_validator or something, or fix dblink to use validator of new
wrapper. I'm not sure that dblink should be alone or integrated with
pgsql_fdw...

Connection management
=====================
The pgsql_fdw establishes a new connection when a foreign server is
accessed first for the local session. Established connection is shared
between all foreign scans in the local query, and shared between even
scans in following queries. Connections are discarded when the current
transaction aborts so that unexpected failure won't cause connection
leak. This is implemented with resource owner mechanism.

User can see active connections via pgsql_fdw_connections view, and
discard arbitrary connection via pgsql_fdw_disconnect() function. These
can be done from only same local session.

If local role has changed via SET ROLE or SET SESSION AUTHENTICATION,
pgsql_fdw ignores old role's connections and looks up appropriate
connection for the new role from the pool. If there wasn't suitable
one, pgsql_fdw establishes new connection. When local role has changed
to old role again, pooled connection will be used again.

Unlike contrib/dblink, one foreign server can have only one connection
at a time for one local role. This is because pgsql_fdw doesn't support
named connections.

Cost estimation
===============
The pgsql_fdw executes an EXPLAIN command on remote side for each
PlanForeignScan call. Returned costs and rows are used as local
estimation for the Path with adding connection costs and data transfer
costs.

SELECT optimization
===================
To reduce amount of data transferred from remote server, references to
unnecessary columns are replaced with NULL literal in remote query.

WHERE clause push-down
======================
Some kind of qualifiers in WHERE clause are pushed down to remote server
so that the query result can be reduced. Currently qualifiers which
include any volatile or stable element can't be pushed down. Even with
these limitations, most qualifiers would be pushed down in usual cases.

Cursor mode
===========
The pgsql_fdw switches the way to retrieve result records according to
estimated result rows; use simple SELECT for small result, and use
cursor with DECLARE/FETCH statements for large result. The threshold
is default to 1000, and configurable with FDW option "min_cursor_rows".
In cursor mode, number of rows fetched at once can be controlled by FDW
option "fetch_count".

EXPLAIN output
==============
The pgsql_fdw shows a remote query used for each foreign scan node in
the output of EXPLAIN command with title "Remote SQL". If pgsql_fdw
decided to use cursor for the scan, DECLARE statement is shown.

Regards,
--
Shigeru Hanada

2011/10/25 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> I'd like to propose pgsql_fdw, FDW for external PostgreSQL server, as a
> contrib module.  I think that this module would be the basis of further
> SQL/MED development for core, e.g. join-push-down and ANALYZE support.

I have not looked at the code itself, but I wonder if we shouldn't
consider making this a part of core-proper, not just a contrib module.
The fact that it isn't *already* available in core surprises a lot of
people...

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> 2011/10/25 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
>> I'd like to propose pgsql_fdw, FDW for external PostgreSQL server, as a
>> contrib module. I think that this module would be the basis of further
>> SQL/MED development for core, e.g. join-push-down and ANALYZE support.

> I have not looked at the code itself, but I wonder if we shouldn't
> consider making this a part of core-proper, not just a contrib module.
> The fact that it isn't *already* available in core surprises a lot of
> people...

We've just spent a whole lot of blood and sweat on making the extension
mechanism work nicely. I don't understand this urge to not use it.

ATM I'm not sure it's even a good idea to push pgsql_fdw into contrib.
Once we do that its release schedule will get locked to core's ---
wouldn't it be better to keep flexibility for now, while it's in such
active development?

regards, tom lane

On Tue, Oct 25, 2011 at 14:08, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> 2011/10/25 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
>>> I'd like to propose pgsql_fdw, FDW for external PostgreSQL server, as a
>>> contrib module.  I think that this module would be the basis of further
>>> SQL/MED development for core, e.g. join-push-down and ANALYZE support.
>
>> I have not looked at the code itself, but I wonder if we shouldn't
>> consider making this a part of core-proper, not just a contrib module.
>> The fact that it isn't *already* available in core surprises a lot of
>> people...
>
> We've just spent a whole lot of blood and sweat on making the extension
> mechanism work nicely.  I don't understand this urge to not use it.

We're back to the old discussion, I guess.. I'm happy to see it as an
extension, but I think it should be included with the standard
installation. Like we do with for example pl/pgsql (which I realize
has a dependency on the backend anyway, so it can't be done another
way easily) and pl/perl (which doesn't, AFAIK, so it's a better
example)

> ATM I'm not sure it's even a good idea to push pgsql_fdw into contrib.
> Once we do that its release schedule will get locked to core's ---
> wouldn't it be better to keep flexibility for now, while it's in such
> active development?

I would be happy to keep it outside, and integrate it in the final CF
for example :)

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

>> ATM I'm not sure it's even a good idea to push pgsql_fdw into contrib.
>> Once we do that its release schedule will get locked to core's ---
>> wouldn't it be better to keep flexibility for now, while it's in such
>> active development?
>
> I would be happy to keep it outside, and integrate it in the final CF
> for example :)
>
Right now, file_fdw is the only FDW module that we have in the core,
however, it is inadequacy to proof the new concept of FDW feature
to utilize external RDBMS, such as join push-down of foreign tables.

I think the pgsql-fdw module also should be included in the core
distribution as a basis of future enhancement, unless we don't
need any working modules when an enhancement of FDW is
proposed.

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

* Kohei KaiGai (kaigai(at)kaigai(dot)gr(dot)jp) wrote:
> Right now, file_fdw is the only FDW module that we have in the core,

Erm, guess I'm a bit confused why we've got that in core while not
putting pgsql_fdw in core. This all gets back to previous discussions
around 'recommended' contrib modules (which should really be installed
by default on the filesystem through the distros, ala Debian's
"recommends:" approach) and 'other' contrib modules.

I'm in favor of making that distinction. I would still have pgsql_fdw,
file_fdw, etc, be packaged more-or-less the same way and still use the
CREATE EXTENTION framework, of course.

It would be nice if we didn't have to lock the release schedule of those
recommended modules to the core release schedule, or even to each other,
but that's a separate issue, imv.

Thanks,

Stephen

On Tue, Oct 25, 2011 at 3:08 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> 2011/10/25 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
>>> I'd like to propose pgsql_fdw, FDW for external PostgreSQL server, as a
>>> contrib module.  I think that this module would be the basis of further
>>> SQL/MED development for core, e.g. join-push-down and ANALYZE support.
>
>> I have not looked at the code itself, but I wonder if we shouldn't
>> consider making this a part of core-proper, not just a contrib module.
>> The fact that it isn't *already* available in core surprises a lot of
>> people...
>
> We've just spent a whole lot of blood and sweat on making the extension
> mechanism work nicely.  I don't understand this urge to not use it.
>
> ATM I'm not sure it's even a good idea to push pgsql_fdw into contrib.
> Once we do that its release schedule will get locked to core's ---
> wouldn't it be better to keep flexibility for now, while it's in such
> active development?

Simple question - do FDW internals need work?

--
marko

(2011/10/25 19:15), Magnus Hagander wrote:
> 2011/10/25 Shigeru Hanada<shigeru(dot)hanada(at)gmail(dot)com>:
>> I'd like to propose pgsql_fdw, FDW for external PostgreSQL server, as a
>> contrib module. I think that this module would be the basis of further
>> SQL/MED development for core, e.g. join-push-down and ANALYZE support.
>
> I have not looked at the code itself, but I wonder if we shouldn't
> consider making this a part of core-proper, not just a contrib module.
> The fact that it isn't *already* available in core surprises a lot of
> people...

Do you mean that pgsql_fdw should be a built-in extension like plpgsql
so that it's available just after initdb? It would be accomplished with
some more changes:

* Move pgsql_fdw into core, say src/backend/foreign/libpgsql_fdw, and
install dynamically loadable module during "make install" for core. The
pgsql_fdw_handler function can't be included into core binary because we
must avoid liking libpq with server binary directly. This method is
also used for libwalreceiver of replication module.
* Create pgsql_fdw extension during initdb invocation, like plpgsql.

These are not trivial, but not difficult so much. However, I think
contrib would be the appropriate place for pgsql_fdw because it's
(relatively) special feature.
--
Shigeru Hanada

Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com> writes:
> (2011/10/25 19:15), Magnus Hagander wrote:
>> I have not looked at the code itself, but I wonder if we shouldn't
>> consider making this a part of core-proper, not just a contrib module.
>> The fact that it isn't *already* available in core surprises a lot of
>> people...

> Do you mean that pgsql_fdw should be a built-in extension like plpgsql
> so that it's available just after initdb?

If that was what he meant, I'd vote against it. There are way too many
people who will *not* want their databases configured to be able to
reach out onto the net. This feature should be something that has to be
installed by explicit user action.

regards, tom lane

2011/10/26 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> (2011/10/25 19:15), Magnus Hagander wrote:
>> 2011/10/25 Shigeru Hanada<shigeru(dot)hanada(at)gmail(dot)com>:
>>> I'd like to propose pgsql_fdw, FDW for external PostgreSQL server, as a
>>> contrib module.  I think that this module would be the basis of further
>>> SQL/MED development for core, e.g. join-push-down and ANALYZE support.
>>
>> I have not looked at the code itself, but I wonder if we shouldn't
>> consider making this a part of core-proper, not just a contrib module.
>> The fact that it isn't *already* available in core surprises a lot of
>> people...
>
> Do you mean that pgsql_fdw should be a built-in extension like plpgsql
> so that it's available just after initdb?  It would be accomplished with
> some more changes:
>
> * Move pgsql_fdw into core, say src/backend/foreign/libpgsql_fdw, and
> install dynamically loadable module during "make install" for core.  The
> pgsql_fdw_handler function can't be included into core binary because we
> must avoid liking libpq with server binary directly.  This method is
> also used for libwalreceiver of replication module.
> * Create pgsql_fdw extension during initdb invocation, like plpgsql.
>
> These are not trivial, but not difficult so much.  However, I think
> contrib would be the appropriate place for pgsql_fdw because it's
> (relatively) special feature.

I agree. pgsql_fdw will be a nice feature, but there's no reason to
think that everyone will want it installed by default, and there are
some security reasons to think that they might not. On the flip side,
pushing it out of contrib and onto pgfoundry or whatever makes it
unnecessarily difficult to install, and not as many people will
benefit from it. So contrib seems exactly right to me.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> I'm in favor of making that distinction. I would still have pgsql_fdw,
> file_fdw, etc, be packaged more-or-less the same way and still use the
> CREATE EXTENTION framework, of course.

We called that idea “core extension” at the latest hackers meeting, and
Greg Smith had a patch with a first selections of extensions to package
this way.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

2011/10/26 Robert Haas <robertmhaas(at)gmail(dot)com>:
> 2011/10/26 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
>> (2011/10/25 19:15), Magnus Hagander wrote:
>>> 2011/10/25 Shigeru Hanada<shigeru(dot)hanada(at)gmail(dot)com>:
>>>> I'd like to propose pgsql_fdw, FDW for external PostgreSQL server, as a
>>>> contrib module.  I think that this module would be the basis of further
>>>> SQL/MED development for core, e.g. join-push-down and ANALYZE support.
>>>
>>> I have not looked at the code itself, but I wonder if we shouldn't
>>> consider making this a part of core-proper, not just a contrib module.
>>> The fact that it isn't *already* available in core surprises a lot of
>>> people...
>>
>> Do you mean that pgsql_fdw should be a built-in extension like plpgsql
>> so that it's available just after initdb?  It would be accomplished with
>> some more changes:
>>
>> * Move pgsql_fdw into core, say src/backend/foreign/libpgsql_fdw, and
>> install dynamically loadable module during "make install" for core.  The
>> pgsql_fdw_handler function can't be included into core binary because we
>> must avoid liking libpq with server binary directly.  This method is
>> also used for libwalreceiver of replication module.
>> * Create pgsql_fdw extension during initdb invocation, like plpgsql.
>>
>> These are not trivial, but not difficult so much.  However, I think
>> contrib would be the appropriate place for pgsql_fdw because it's
>> (relatively) special feature.
>
> I agree.  pgsql_fdw will be a nice feature, but there's no reason to
> think that everyone will want it installed by default, and there are
> some security reasons to think that they might not.  On the flip side,
> pushing it out of contrib and onto pgfoundry or whatever makes it
> unnecessarily difficult to install, and not as many people will
> benefit from it.  So contrib seems exactly right to me.
>
I also agree. The pgsql_fdw will be worthful to locate in the main tree
as a contrib module. It will give us clear opportunity to test new
features of FDW using RDBMS characteristics; such as join-push-down.
However, it should be a separated discussion whether it shall be installed
by the default.

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

On Wed, Oct 26, 2011 at 16:37, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com> writes:
>> (2011/10/25 19:15), Magnus Hagander wrote:
>>> I have not looked at the code itself, but I wonder if we shouldn't
>>> consider making this a part of core-proper, not just a contrib module.
>>> The fact that it isn't *already* available in core surprises a lot of
>>> people...
>
>> Do you mean that pgsql_fdw should be a built-in extension like plpgsql
>> so that it's available just after initdb?
>
> If that was what he meant, I'd vote against it.  There are way too many
> people who will *not* want their databases configured to be able to
> reach out onto the net.  This feature should be something that has to be
> installed by explicit user action.

That is not what I meant.

I meant installed the shared library by defualt, but still require
CREATE EXTENSION.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On 10/26/2011 12:47 PM, Magnus Hagander wrote:
>>
>> If that was what he meant, I'd vote against it. There are way too many
>> people who will *not* want their databases configured to be able to
>> reach out onto the net. This feature should be something that has to be
>> installed by explicit user action.
> That is not what I meant.
>
> I meant installed the shared library by defualt, but still require
> CREATE EXTENSION.
>

I don't see why it should be different from other standard modules, such
as citext or hstore, both of which have pretty wide use, and less
possible security implications than this.

cheers

andrew

On Wed, Oct 26, 2011 at 19:25, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
> On 10/26/2011 12:47 PM, Magnus Hagander wrote:
>>>
>>> If that was what he meant, I'd vote against it.  There are way too many
>>> people who will *not* want their databases configured to be able to
>>> reach out onto the net.  This feature should be something that has to be
>>> installed by explicit user action.
>>
>> That is not what I meant.
>>
>> I meant installed the shared library by defualt, but still require
>> CREATE EXTENSION.
>>
>
> I don't see why it should be different from other standard modules, such as
> citext or hstore, both of which have pretty wide use, and less possible
> security implications than this.

As I stated earlier, it's really back to the old discussion of
splitting up contrib. This would be the "additional module" part, but
not the "example of how to do things" part of that...

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> On Wed, Oct 26, 2011 at 16:37, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> If that was what he meant, I'd vote against it. There are way too many
>> people who will *not* want their databases configured to be able to
>> reach out onto the net. This feature should be something that has to be
>> installed by explicit user action.

> That is not what I meant.

> I meant installed the shared library by defualt, but still require
> CREATE EXTENSION.

Whether the shlib is installed by default is a decision for packagers to
make, not us. At best we could make a recommendation.

regards, tom lane

(2011/10/26 23:57), Kohei KaiGai wrote:
> 2011/10/26 Robert Haas<robertmhaas(at)gmail(dot)com>:
>> I agree. pgsql_fdw will be a nice feature, but there's no reason to
>> think that everyone will want it installed by default, and there are
>> some security reasons to think that they might not. On the flip side,
>> pushing it out of contrib and onto pgfoundry or whatever makes it
>> unnecessarily difficult to install, and not as many people will
>> benefit from it. So contrib seems exactly right to me.
>>
> I also agree. The pgsql_fdw will be worthful to locate in the main tree
> as a contrib module. It will give us clear opportunity to test new
> features of FDW using RDBMS characteristics; such as join-push-down.
> However, it should be a separated discussion whether it shall be installed
> by the default.

There seems to be some approvals on pushing pgsql_fdw into main tree
(contrib or core extension, or something else), but not an external
module. There are still some debatable issues, but they would be
meaningless unless pgsql_fdw is qualified for a contrib module. So I'd
like to continue the development of pgsql_fdw as contrib module, at
least for a while.

Please find attached a patch for pgsql_fdw. This patch needs first two
patches attached to OP[1] to be applied. (Sorry. gathering patches from
another post must be bothersome work. Should I create new CF items for
fundamental patches?)

[1] http://archives.postgresql.org/pgsql-hackers/2011-10/msg01329.php

Changes done since last post are:
* add colname FDW option support
* allow some libpq options (authtype and tty) to be specified as server
FDW options

--
Shigeru Hanada

* ポルトガル語 - 自動検出
* 英語
* 日本語

* 英語
* 日本語

<javascript:void(0);>

2011/10/25 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
>
> Connection management
> =====================
> The pgsql_fdw establishes a new connection when a foreign server is
> accessed first for the local session.  Established connection is shared
> between all foreign scans in the local query, and shared between even
> scans in following queries.  Connections are discarded when the current
> transaction aborts so that unexpected failure won't cause connection
> leak.  This is implemented with resource owner mechanism.
>

I have a doubt here, on sharing connection for each server. What if
there are simultaneous scan on the same plan? Say,

-> Nested Loop
-> Foreign Scan to table T1 on server A
-> Foreign Scan to table T2 on server A

Okay, you are thinking about Foreign Join, so example above is too
simple. But it is always possible to execute such a query if foreign
scan nodes are separated far, isn't it? As far as I see from your
explanation, scan T1 and scan T2 share the same connection. Now join
node scans one row from left (T1) while asking rows from right (T2)
without fetching all the rows from left. If T2 requests to server A,
the connection's result (of T1) is discarded. Am I understand
correctly?

Regards,
--
Hitoshi Harada

On Sat, Oct 29, 2011 at 12:25:46AM -0700, Hitoshi Harada wrote:
> I have a doubt here, on sharing connection for each server. What if
> there are simultaneous scan on the same plan? Say,
>
> -> Nested Loop
> -> Foreign Scan to table T1 on server A
> -> Foreign Scan to table T2 on server A
>
> Okay, you are thinking about Foreign Join, so example above is too
> simple. But it is always possible to execute such a query if foreign
> scan nodes are separated far, isn't it? As far as I see from your
> explanation, scan T1 and scan T2 share the same connection. Now join
> node scans one row from left (T1) while asking rows from right (T2)
> without fetching all the rows from left. If T2 requests to server A,
> the connection's result (of T1) is discarded. Am I understand
> correctly?

This would need to be factored in in the cost calculations. For remote
servers there is an overhead per tuple transmitted. So in the above
case it might actually be quicker to do the nested loop locally.

To handle the parallel case you might need to materialise in the inner
loop, that would avoid the double scan. Or we could fix the protocol so
you can stream multiple queries at once.

Actually, you can already do this is you use DECLARE CURSOR for all the
queries upfront and then FETCH as needed. That way you can do it all
over one connection.

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> He who writes carelessly confesses thereby at the very outset that he does
> not attach much importance to his own thoughts.
-- Arthur Schopenhauer

Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com> writes:
> I have a doubt here, on sharing connection for each server. What if
> there are simultaneous scan on the same plan? Say,

> -> Nested Loop
> -> Foreign Scan to table T1 on server A
> -> Foreign Scan to table T2 on server A

> Okay, you are thinking about Foreign Join, so example above is too
> simple. But it is always possible to execute such a query if foreign
> scan nodes are separated far, isn't it? As far as I see from your
> explanation, scan T1 and scan T2 share the same connection. Now join
> node scans one row from left (T1) while asking rows from right (T2)
> without fetching all the rows from left. If T2 requests to server A,
> the connection's result (of T1) is discarded. Am I understand
> correctly?

I have not looked at the code, but ISTM the way that this has to work is
that you set up a portal for each active scan. Then you can fetch a few
rows at a time from any one of them.

If you're doing this through libpq, it'd be necessary to implement each
scan using a cursor. I'm not sure whether it'd be worth our time to
add more functions to libpq to allow more-direct access to the protocol
portal feature.

regards, tom lane

On Sat, Oct 29, 2011 at 8:13 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com> writes:
>> I have a doubt here, on sharing connection for each server. What if
>> there are simultaneous scan on the same plan? Say,
>
>> -> Nested Loop
>>   -> Foreign Scan to table T1 on server A
>>   -> Foreign Scan to table T2 on server A
>
>> Okay, you are thinking about Foreign Join, so example above is too
>> simple. But it is always possible to execute such a query if foreign
>> scan nodes are separated far, isn't it? As far as I see from your
>> explanation, scan T1 and scan T2 share the same connection. Now join
>> node scans one row from left (T1) while asking rows from right (T2)
>> without fetching all the rows from left. If T2 requests to server A,
>> the connection's result (of T1) is discarded. Am I understand
>> correctly?
>
> I have not looked at the code, but ISTM the way that this has to work is
> that you set up a portal for each active scan.  Then you can fetch a few
> rows at a time from any one of them.

Hmm, true. Looking back at the original proposal (neither did I look
at the code,) there seems to be a cursor mode. ISTM it is hard for fdw
to know how the whole plan tree looks, so consequently do we always
cursor regardless of estimated row numbers? I haven't had much
experiences around cursor myself, but is it as efficient as
non-cursor?

Regards,
--
Hitoshi Harada

Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com> writes:
> On Sat, Oct 29, 2011 at 8:13 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I have not looked at the code, but ISTM the way that this has to work is
>> that you set up a portal for each active scan. Then you can fetch a few
>> rows at a time from any one of them.

> Hmm, true. Looking back at the original proposal (neither did I look
> at the code,) there seems to be a cursor mode. ISTM it is hard for fdw
> to know how the whole plan tree looks, so consequently do we always
> cursor regardless of estimated row numbers?

I think we have to. Even if we estimate that a given scan will return
only a few rows, what happens if we're wrong? We don't want to blow out
memory on the local server by retrieving gigabytes in one go.

> I haven't had much experiences around cursor myself, but is it as
> efficient as non-cursor?

No, but if you need max efficiency you shouldn't be using foreign tables
in the first place; they're always going to be expensive to access.

It's likely that making use of native protocol portals (instead of
executing a lot of FETCH commands) would help. But I think we'd be well
advised to do the first pass with just the existing libpq facilities,
and then measure to see where to improve performance.

regards, tom lane

2011/10/29 Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com>:
> I have a doubt here, on sharing connection for each server. What if
> there are simultaneous scan on the same plan? Say,
>
> -> Nested Loop
>  -> Foreign Scan to table T1 on server A
>  -> Foreign Scan to table T2 on server A
>
> Okay, you are thinking about Foreign Join, so example above is too
> simple. But it is always possible to execute such a query if foreign
> scan nodes are separated far, isn't it? As far as I see from your
> explanation, scan T1 and scan T2 share the same connection. Now join
> node scans one row from left (T1) while asking rows from right (T2)
> without fetching all the rows from left. If T2 requests to server A,
> the connection's result (of T1) is discarded. Am I understand
> correctly?

I think that sharing a connection doesn't cause any problem.

In cursor mode, using multiple cursors concurrently through one connection
is OK. In SELECT mode, pgsql_fdw executes SELECT statement with
PQexecParams and retrieves whole result *inside* the first Iterate call for
an outer tuple. So libpq connection is already available when another scan
needs to call Iterate function.

--
Shigeru Hanada

2011/10/30 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com> writes:
>> On Sat, Oct 29, 2011 at 8:13 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> I have not looked at the code, but ISTM the way that this has to work is
>>> that you set up a portal for each active scan.  Then you can fetch a few
>>> rows at a time from any one of them.
>
>> Hmm, true. Looking back at the original proposal (neither did I look
>> at the code,) there seems to be a cursor mode. ISTM it is hard for fdw
>> to know how the whole plan tree looks, so consequently do we always
>> cursor regardless of estimated row numbers?
>
> I think we have to.  Even if we estimate that a given scan will return
> only a few rows, what happens if we're wrong?  We don't want to blow out
> memory on the local server by retrieving gigabytes in one go.

Oh, I overlooked the possibility of wrong estimation. Old PostgreSQL uses
1000 as default estimation, so big table which has not been analyzed may
crashes the backend.

To ensure the data retrieving safe, we need to get actual amount of result,
maybe by executing SELECT COUNT(*) in planning phase. It sounds too heavy
to do for every scan, and it still lacks actual width.

One possible idea is to change default value of min_cursur_rows option to 0
so that pgsql_fdw uses CURSOR by default, but it seems not enough. I'll
drop simple SELECT mode from first version of pgsql_fdw for safety.

>> I haven't had much experiences around cursor myself, but is it as
>> efficient as non-cursor?
>
> No, but if you need max efficiency you shouldn't be using foreign tables
> in the first place; they're always going to be expensive to access.
>
> It's likely that making use of native protocol portals (instead of
> executing a lot of FETCH commands) would help.  But I think we'd be well
> advised to do the first pass with just the existing libpq facilities,
> and then measure to see where to improve performance.

I long for protocol-level cursor. :)

--
Shigeru Hanada

(2011/10/30 11:34), Shigeru Hanada wrote:
> 2011/10/30 Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> I think we have to. Even if we estimate that a given scan will return
>> only a few rows, what happens if we're wrong? We don't want to blow out
>> memory on the local server by retrieving gigabytes in one go.
>
> Oh, I overlooked the possibility of wrong estimation. Old PostgreSQL uses
> 1000 as default estimation, so big table which has not been analyzed may
> crashes the backend.
>
> To ensure the data retrieving safe, we need to get actual amount of result,
> maybe by executing SELECT COUNT(*) in planning phase. It sounds too heavy
> to do for every scan, and it still lacks actual width.
>
> One possible idea is to change default value of min_cursur_rows option to 0
> so that pgsql_fdw uses CURSOR by default, but it seems not enough. I'll
> drop simple SELECT mode from first version of pgsql_fdw for safety.

I removed simple SELECT mode from pgsql_fdw, and consequently also
removed min_cursor_rows FDW option. This fix avoids possible memory
exhaustion due to wrong estimation gotten from remote side.

Once libpq has had capability to retrieve arbitrary number of rows from
remote portal at a time without server-side cursor in future, then we
will be able to revive simple SELECT. Then it's enough safe even if we
don't have actual data size, but (maybe) faster than cursor mode because
we can reduce # of SQL commands. Though of course proof of performance
advantage should be shown before such development.

--
Shigeru Hanada

2011/10/31 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> (2011/10/30 11:34), Shigeru Hanada wrote:
>> 2011/10/30 Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>>> I think we have to.  Even if we estimate that a given scan will return
>>> only a few rows, what happens if we're wrong?  We don't want to blow out
>>> memory on the local server by retrieving gigabytes in one go.
>>
>> Oh, I overlooked the possibility of wrong estimation.  Old PostgreSQL uses
>> 1000 as default estimation, so big table which has not been analyzed may
>> crashes the backend.
>>
>> To ensure the data retrieving safe, we need to get actual amount of result,
>> maybe by executing SELECT COUNT(*) in planning phase.  It sounds too heavy
>> to do for every scan, and it still lacks actual width.
>>
>> One possible idea is to change default value of min_cursur_rows option to 0
>> so that pgsql_fdw uses CURSOR by default, but it seems not enough.  I'll
>> drop simple SELECT mode from first version of pgsql_fdw for safety.
>
> I removed simple SELECT mode from pgsql_fdw, and consequently also
> removed min_cursor_rows FDW option.  This fix avoids possible memory
> exhaustion due to wrong estimation gotten from remote side.
>
> Once libpq has had capability to retrieve arbitrary number of rows from
> remote portal at a time without server-side cursor in future, then we
> will be able to revive simple SELECT.  Then it's enough safe even if we
> don't have actual data size, but (maybe) faster than cursor mode because
> we can reduce # of SQL commands.  Though of course proof of performance
> advantage should be shown before such development.

If you need a less SQL commands, then you can increase fetch_count
parameter - default 1000 is maybe too small, maybe 10000 lines as
default (not more).

For more complex queries can be interesting to set a cursor_tuple_fraction

Pavel

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

Hi,

Attached are revised version of pgsql_fdw patches.

fdw_helper_funcs_v2.patch provides some functions which would be useful
to implement FDW, and document about FDW helper functions including
those which exist in 9.1. They are not specific to pgsql_fdw, so I
separated it from pgsql_fdw patch.

pgsql_fdw_v4.patch provides a FDW for PostgreSQL. This patch requires
fdw_helper_funcs_v2.patch has been applied. Changes done since last
version are:

* Default of fetch_count option is increased to 10000, as suggested by
Pavel Stehule.
* Remove unnecessary NULL check before PQresultStatus.
* Evaluate all conditions on local side even if some of them has been
pushed down to remote side, to ensure that results are correct.
* Use fixed costs for queries which contain external parameter, because
such query can't be used in EXPLAIN command.

Regards,
--
Shigeru Hanada

Hanada-san,

I'm still under reviewing of your patch, so the comment is not overall, sorry.

I'm not sure whether the logic of is_foreign_expr() is appropriate.
It checks oid of the function within FuncExpr and OpExpr to disallow to push
down user-defined functions.
However, if a user-defined operator is implemented based on built-in functions
with different meaning, is it really suitable to push-down?
E.g) It is available to define '=' operator using int4ne, even though
quite nonsense.
So, I'd like to suggest to check oid of the operator; whether it is a
built-in, or not.

On the other hand, this hard-wired restriction may damage to the purpose of
this module; that enables to handle a query on multiple nodes in parallel.
I'm not sure whether it is right design that is_foreign_expr() returns false
when the supplied expr contains mutable functions.

Probably, here are two perspectives. The one want to make sure functions works
with same manner in all nodes. The other want to distribute processing
of queries
as possible as we can. Here is a trade-off between these two perspectives.
So, how about an idea to add a guc variable to control the criteria of
pushing-down.

Thanks,

2011年11月15日17:55 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> Hi,
>
> Attached are revised version of pgsql_fdw patches.
>
> fdw_helper_funcs_v2.patch provides some functions which would be useful
> to implement FDW, and document about FDW helper functions including
> those which exist in 9.1. They are not specific to pgsql_fdw, so I
> separated it from pgsql_fdw patch.
>
> pgsql_fdw_v4.patch provides a FDW for PostgreSQL. This patch requires
> fdw_helper_funcs_v2.patch has been applied. Changes done since last
> version are:
>
> * Default of fetch_count option is increased to 10000, as suggested by
> Pavel Stehule.
> * Remove unnecessary NULL check before PQresultStatus.
> * Evaluate all conditions on local side even if some of them has been
> pushed down to remote side, to ensure that results are correct.
> * Use fixed costs for queries which contain external parameter, because
> such query can't be used in EXPLAIN command.
>
> Regards,
> --
> Shigeru Hanada
>

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

Hi Hanada-san,

(2011/11/16 1:55), Shigeru Hanada wrote:
> Attached are revised version of pgsql_fdw patches.

I'm still under reviewing, so the following is not all. I'm sorry.
estimate_costs() have been implemented to ask a remote postgres server
for the result of EXPLAIN for a remote query to get its costs such as
startup_cost and total_cost. I think this approach is the most accurate
way to get its costs. However, I think it would be rather costly. And
I'm afraid of that it might work only for pgsql_fdw. Because, even if we
are able to obtain such a cost information by EXPLAINing a remote query
at a remote server where a DBMS different from postgres runs, it might
be difficult to incorporate such a cost information with the postgres
cost model due to their possible inconsistency that such a cost
information provided by the EXPLAIN command in the other DBMS might have
different meanings (or different scales) from that provided by the
EXPLAIN command in postgres. So, I think it might be better to estimate
such costs by pgsql_fdw itself without EXPLAINing on the assumption that
a remote postgres server has the same abilities for query optimization,
which is less costly and widely applicable to the other DBMSs, while it,
of course, only works once we have statistics and/or index information
for foreign tables. But AFAIK we eventually want to have those, so I'd
like to propose to use the proposed approach until that time.

Best regards,
Etsuro Fujita

Hi Kaigai-san,

(2011/11/20 2:42), Kohei KaiGai wrote:
> I'm still under reviewing of your patch, so the comment is not overall, sorry.

Thanks for the review!

> I'm not sure whether the logic of is_foreign_expr() is appropriate.
> It checks oid of the function within FuncExpr and OpExpr to disallow to push
> down user-defined functions.
> However, if a user-defined operator is implemented based on built-in functions
> with different meaning, is it really suitable to push-down?
> E.g) It is available to define '=' operator using int4ne, even though
> quite nonsense.
> So, I'd like to suggest to check oid of the operator; whether it is a
> built-in, or not.
>
> On the other hand, this hard-wired restriction may damage to the purpose of
> this module; that enables to handle a query on multiple nodes in parallel.
> I'm not sure whether it is right design that is_foreign_expr() returns false
> when the supplied expr contains mutable functions.
>
> Probably, here are two perspectives. The one want to make sure functions works
> with same manner in all nodes. The other want to distribute processing
> of queries
> as possible as we can. Here is a trade-off between these two perspectives.
> So, how about an idea to add a guc variable to control the criteria of
> pushing-down.

I agree that allowing users to control which function/operator should be
pushed down is useful, but GUC seems too large as unit of switching
behavior. "Routine Mapping", a mechanism which is defined in SQL/MED
standard, would be the answer for this issue. It can be used to map a
local routine (a procedure or a function) to something on a foreign
server. It is like user mapping, but it has mapping name. Probably it
would have these attributes:

pg_catalog.pg_routine_mapping
rmname name
rmprocid regproc
rmserverid oid
rmfdwoptions text[]

If we have routine mapping, FDW authors can provide default mappings
within extension installation, and users can customize them. Maybe FDWs
will want to push down only functions/operators which have routine
mapping entries, so providing common routine which returns mapping
information of given function/operator, say GetRoutineMapping(procid,
serverid), is useful.

Unfortunately we don't have it at the moment, I'll fix pgsql_fdw so that
it pushes down only built-in operators, including scalar-array operators.

Regards,
--
Shigeru Hanada

Hi Fujita-san,

(2011/11/25 17:27), Etsuro Fujita wrote:
> I'm still under reviewing, so the following is not all. I'm sorry.
> estimate_costs() have been implemented to ask a remote postgres server
> for the result of EXPLAIN for a remote query to get its costs such as
> startup_cost and total_cost. I think this approach is the most accurate
> way to get its costs. However, I think it would be rather costly. And
> I'm afraid of that it might work only for pgsql_fdw.

Indeed. In addition, this approach assumes that cost factors of target
PG server are same as local's ones. pgsql_fdw might have to have cost
factors as FDW options of foreign server.

> Because, even if we
> are able to obtain such a cost information by EXPLAINing a remote query
> at a remote server where a DBMS different from postgres runs, it might
> be difficult to incorporate such a cost information with the postgres
> cost model due to their possible inconsistency that such a cost
> information provided by the EXPLAIN command in the other DBMS might have
> different meanings (or different scales) from that provided by the
> EXPLAIN command in postgres.

Yes, so implementing cost estimation for other DBMSs accurately would be
very difficult, but AFAIS rows estimation is the most important factor,
so reasonable row count and relatively high startup cost would produce
not-so-bad plan.

> So, I think it might be better to estimate
> such costs by pgsql_fdw itself without EXPLAINing on the assumption that
> a remote postgres server has the same abilities for query optimization,
> which is less costly and widely applicable to the other DBMSs, while it,
> of course, only works once we have statistics and/or index information
> for foreign tables. But AFAIK we eventually want to have those, so I'd
> like to propose to use the proposed approach until that time.

Knowledge of foreign indexes also provide information of sort order.
Planner will be able to consider merge join without local sort with such
information. Without foreign index, we have to enumerate possible sort
keys with Blute-Force approach for same result, as mentioned by
Itagaki-san before.

Regards,
--
Shigeru Hanada

2011/11/28 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> I agree that allowing users to control which function/operator should be
> pushed down is useful, but GUC seems too large as unit of switching
> behavior.  "Routine Mapping", a mechanism which is defined in SQL/MED
> standard, would be the answer for this issue.  It can be used to map a
> local routine (a procedure or a function) to something on a foreign
> server.  It is like user mapping, but it has mapping name.  Probably it
> would have these attributes:
>
> pg_catalog.pg_routine_mapping
>    rmname              name
>    rmprocid            regproc
>    rmserverid          oid
>    rmfdwoptions        text[]
>
> If we have routine mapping, FDW authors can provide default mappings
> within extension installation, and users can customize them.  Maybe FDWs
> will want to push down only functions/operators which have routine
> mapping entries, so providing common routine which returns mapping
> information of given function/operator, say GetRoutineMapping(procid,
> serverid), is useful.
>
> Unfortunately we don't have it at the moment, I'll fix pgsql_fdw so that
> it pushes down only built-in operators, including scalar-array operators.

One difficulty here is that even very simple operators don't
necessarily mean the same thing on both sides. In my last job we had
a Microsoft SQL database where string equality was case insensitive,
and a PostgreSQL database where it wasn't.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

(2011/11/28 20:50), Shigeru Hanada wrote:
> (2011/11/25 17:27), Etsuro Fujita wrote:
>> So, I think it might be better to estimate
>> such costs by pgsql_fdw itself without EXPLAINing on the assumption that
>> a remote postgres server has the same abilities for query optimization,
>> which is less costly and widely applicable to the other DBMSs, while it,
>> of course, only works once we have statistics and/or index information
>> for foreign tables. But AFAIK we eventually want to have those, so I'd
>> like to propose to use the proposed approach until that time.
>
> Knowledge of foreign indexes also provide information of sort order.
> Planner will be able to consider merge join without local sort with such
> information. Without foreign index, we have to enumerate possible sort
> keys with Blute-Force approach for same result, as mentioned by
> Itagaki-san before.

Yes, with the knowledge of foreign indexes, I think we can take the
approach of thinking multiple plans for a foreign table; the cheapest
unordered plan and the cheapest plan with a given sort order. In
addition, it would be also possible to support
nestloop-with-inner-foreign-indexscans on a foreign table as pointed out
as future work by Tom Lane at PGCon 2011[1].

[1] http://www.pgcon.org/2011/schedule/attachments/188_Planner%20talk.pdf

Best regards,
Etsuro Fujita

Robert Haas wrote:
> 2011/11/28 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
>> I agree that allowing users to control which function/operator should be
>> pushed down is useful, but GUC seems too large as unit of switching
>> behavior.  "Routine Mapping", a mechanism which is defined in SQL/MED
>> standard, would be the answer for this issue.  It can be used to map a
>> local routine (a procedure or a function) to something on a foreign
>> server.  It is like user mapping, but it has mapping name.  Probably it
>> would have these attributes:
>>
>> pg_catalog.pg_routine_mapping
>>    rmname              name
>>    rmprocid            regproc
>>    rmserverid          oid
>>    rmfdwoptions        text[]
>>
>> If we have routine mapping, FDW authors can provide default mappings
>> within extension installation, and users can customize them.  Maybe FDWs
>> will want to push down only functions/operators which have routine
>> mapping entries, so providing common routine which returns mapping
>> information of given function/operator, say GetRoutineMapping(procid,
>> serverid), is useful.
>>
>> Unfortunately we don't have it at the moment, I'll fix pgsql_fdw so that
>> it pushes down only built-in operators, including scalar-array operators.
>
> One difficulty here is that even very simple operators don't
> necessarily mean the same thing on both sides. In my last job we had
> a Microsoft SQL database where string equality was case insensitive,
> and a PostgreSQL database where it wasn't.

I think that this is not always safe even from PostgreSQL to PostgreSQL.
If two databases have different collation, "<" on strings will behave
differently.

Yours,
Laurenz Albe

Sorry for delayed response.

2011/11/29 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>:
> I think that this is not always safe even from PostgreSQL to PostgreSQL.
> If two databases have different collation, "<" on strings will behave
> differently.

Indeed.  I think that only the owner of foreign table can keep collation
consistent between foreign and local, like data type of column.  We need to
support per-column-collation on foreign tables too, or should deny pushing
down condition which is collation-sensitive...

Regards,--
Shigeru Hanada

On 12/07/2011 02:34 AM, Shigeru Hanada wrote:
> I think that only the owner of foreign table can keep collation
> consistent between foreign and local, like data type of column. We need to
> support per-column-collation on foreign tables too, or should deny pushing
> down condition which is collation-sensitive...
>

I am not sure about what next step you were planning here. Are you
thinking to block this sort of push-down in an update to your feature
patch, or does some more serious work on foreign table collation need to
happen first instead?

It looks like there has been some good discussion of this feature here,
but there is still some work needed before it will be ready to commit.
Hanada-san, did you get the feedback you were looking for here yet, or
are there things you still wanted to discuss? It is not clear to me
what happens next; I would appreciate your comment on that.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us

On Wed, Dec 7, 2011 at 2:34 AM, Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com> wrote:
> Sorry for delayed response.
>
> 2011/11/29 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>:
>> I think that this is not always safe even from PostgreSQL to PostgreSQL.
>> If two databases have different collation, "<" on strings will behave
>> differently.
>
> Indeed.  I think that only the owner of foreign table can keep collation
> consistent between foreign and local, like data type of column.

+1.

> We need to
> support per-column-collation on foreign tables too, or should deny pushing
> down condition which is collation-sensitive...

It seems that we already do:

rhaas=# create foreign table ft1 (a text collate "de_DE") server s1;
CREATE FOREIGN TABLE

It does seem like this might not be enough information for the FDW to
make good decisions about pushdown. Even supposing the server on the
other hand is also PostgreSQL, the collation names might not match
(if, say, one is running Windows, and the other, Linux). And even if
they do, there is no guarantee that two collations with the same name
have the same behavior on two different machines; they probably
should, but who knows? And if we're using an FDW to talk to some
other database server, the problem is much worse; it's not clear that
we'll even begin to be able to guess whether the remote side has
compatible semantics. I feel like we might need a system here that
allows for more explicit user control about what to push down vs. not,
rather than assuming we'll be able to figure it out behind the scenes.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

(2011/12/12 22:59), Robert Haas wrote:
> It does seem like this might not be enough information for the FDW to
> make good decisions about pushdown. Even supposing the server on the
> other hand is also PostgreSQL, the collation names might not match
> (if, say, one is running Windows, and the other, Linux). And even if
> they do, there is no guarantee that two collations with the same name
> have the same behavior on two different machines; they probably
> should, but who knows? And if we're using an FDW to talk to some
> other database server, the problem is much worse; it's not clear that
> we'll even begin to be able to guess whether the remote side has
> compatible semantics. I feel like we might need a system here that
> allows for more explicit user control about what to push down vs. not,
> rather than assuming we'll be able to figure it out behind the scenes.

Agreed. How about to add a per-column boolean FDW option, say
"pushdown", to pgsql_fdw? Users can tell pgsql_fdw that the column can
be pushed down safely by setting this option to true. IMO default
should be false (no push down).

In most cases, columns with numeric/time-related types are safe to be
pushed down because they are free from collations issue, so users would
want to set to true. OTOH, columns with string types would need some
considerations. Once users have ensured that the column has compatible
semantics, they can set "pushdown=true" for efficiency.

If a condition contains any columns with pushdown=false, that condition
should NOT be pushed down.

This idea is only for pgsql_fdw now, but it can be used for other FDWs
which support push-down, and it would be also useful for ORDER BY
push-down support in future, which is apparently contains collation issue.

Regards,
--
Shigeru Hanada

Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com> writes:
> (2011/12/12 22:59), Robert Haas wrote:
>> ... I feel like we might need a system here that
>> allows for more explicit user control about what to push down vs. not,
>> rather than assuming we'll be able to figure it out behind the scenes.

> Agreed. How about to add a per-column boolean FDW option, say
> "pushdown", to pgsql_fdw? Users can tell pgsql_fdw that the column can
> be pushed down safely by setting this option to true.

[ itch... ] That doesn't seem like the right level of granularity.
ISTM the problem is with whether specific operators have the same
meaning at the far end as they do locally. If you try to attach the
flag to columns, you have to promise that *every* operator on that
column means what it does locally, which is likely to not be the
case ever if you look hard enough. Plus, having to set the flag on
each individual column of the same datatype seems pretty tedious.

I don't have a better idea to offer at the moment though. Trying
to attach such a property to operators seems impossibly messy too.
If it weren't for the collations issue, I might think that labeling
datatypes as being compatible would be a workable approximation.

regards, tom lane

Tom Lane wrote:
> Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com> writes:
>> (2011/12/12 22:59), Robert Haas wrote:
>>> ... I feel like we might need a system here that
>>> allows for more explicit user control about what to push down vs.
not,
>>> rather than assuming we'll be able to figure it out behind the
scenes.

>> Agreed. How about to add a per-column boolean FDW option, say
>> "pushdown", to pgsql_fdw? Users can tell pgsql_fdw that the column
can
>> be pushed down safely by setting this option to true.

> [ itch... ] That doesn't seem like the right level of granularity.
> ISTM the problem is with whether specific operators have the same
> meaning at the far end as they do locally. If you try to attach the
> flag to columns, you have to promise that *every* operator on that
> column means what it does locally, which is likely to not be the
> case ever if you look hard enough. Plus, having to set the flag on
> each individual column of the same datatype seems pretty tedious.
>
> I don't have a better idea to offer at the moment though. Trying
> to attach such a property to operators seems impossibly messy too.
> If it weren't for the collations issue, I might think that labeling
> datatypes as being compatible would be a workable approximation.

Maybe I'm missing something, but if pushdown worked as follows:

- Push down only system functions and operators on system types.
- Only push down what is guaranteed to work.

then the only things we would miss out on are encoding- or
collation-sensitive string operations.

Is that loss so big that it warrants a lot of effort?

Yours,
Laurenz Albe

On 13.12.2011 11:57, Albe Laurenz wrote:
> Tom Lane wrote:
>> Shigeru Hanada<shigeru(dot)hanada(at)gmail(dot)com> writes:
>>> (2011/12/12 22:59), Robert Haas wrote:
>>>> ... I feel like we might need a system here that
>>>> allows for more explicit user control about what to push down vs.
> not,
>>>> rather than assuming we'll be able to figure it out behind the
> scenes.
>
>>> Agreed. How about to add a per-column boolean FDW option, say
>>> "pushdown", to pgsql_fdw? Users can tell pgsql_fdw that the column
> can
>>> be pushed down safely by setting this option to true.
>
>> [ itch... ] That doesn't seem like the right level of granularity.
>> ISTM the problem is with whether specific operators have the same
>> meaning at the far end as they do locally. If you try to attach the
>> flag to columns, you have to promise that *every* operator on that
>> column means what it does locally, which is likely to not be the
>> case ever if you look hard enough. Plus, having to set the flag on
>> each individual column of the same datatype seems pretty tedious.
>>
>> I don't have a better idea to offer at the moment though. Trying
>> to attach such a property to operators seems impossibly messy too.
>> If it weren't for the collations issue, I might think that labeling
>> datatypes as being compatible would be a workable approximation.
>
> Maybe I'm missing something, but if pushdown worked as follows:
>
> - Push down only system functions and operators on system types.
> - Only push down what is guaranteed to work.
>
> then the only things we would miss out on are encoding- or
> collation-sensitive string operations.
>
> Is that loss so big that it warrants a lot of effort?

The SQL/MED spec handles this with the concept of "routine mappings".
There is syntax for defining which remote "routines", meaning functions,
correspond local functions:

CREATE ROUTINE MAPPING <routine mapping name> FOR <specific routine
designator>
SERVER <foreign server name> [ <generic options> ]

<generic options> is FDW-specific, I'd imagine the idea is to give the
name of the corresponding function in the remote server. It doesn't say
anything about collations, but you could have extra options to specify
that a function can only be mapped under C collation, or whatever.

It seems tedious to specify that per-server, though, so we'll probably
still want to have some smarts in the pgsql_fdw to handle the built-in
functions and types that we know to be safe.

I've been talking about functions here, not operators, on the assumption
that we can look up the function underlying the operator and make the
decisions based on that.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

(2011/12/13 20:04), Heikki Linnakangas wrote:
> The SQL/MED spec handles this with the concept of "routine mappings".
> There is syntax for defining which remote "routines", meaning functions,
> correspond local functions:
>
> CREATE ROUTINE MAPPING <routine mapping name> FOR <specific routine
> designator>
> SERVER <foreign server name> [ <generic options> ]
>
> <generic options> is FDW-specific, I'd imagine the idea is to give the
> name of the corresponding function in the remote server. It doesn't say
> anything about collations, but you could have extra options to specify
> that a function can only be mapped under C collation, or whatever.

I considered ROUTINE MAPPING for other RDBMS before, and thought that
having order of parameter in generic options would be necessary. It's
also useful for pgsql_fdw to support pushing down user-defined
functions. Maybe built-in format() function suits for this purpose?

> It seems tedious to specify that per-server, though, so we'll probably
> still want to have some smarts in the pgsql_fdw to handle the built-in
> functions and types that we know to be safe.

One possible idea is having default mapping with serverid = InvalidOid,
and override them with entries which has valid server oid. Such default
mappings can be loaded during CREATE EXTENSION.

> I've been talking about functions here, not operators, on the assumption
> that we can look up the function underlying the operator and make the
> decisions based on that.

It's interesting viewpoint to think operator notation is syntax sugar of
function notation, e.g. "A = B" -> "int4eq(A, B)". Routine mappings
seem to work for operators too.

Regards,
--
Shigeru Hanada

(2011/12/13 18:57), Albe Laurenz wrote:
> Maybe I'm missing something, but if pushdown worked as follows:
>
> - Push down only system functions and operators on system types.
> - Only push down what is guaranteed to work.

Oh, I didn't care whether system data types. Indeed user defined types
would not be safe to push down.

> then the only things we would miss out on are encoding- or
> collation-sensitive string operations.
>
> Is that loss so big that it warrants a lot of effort?

It depends on the definition of "collation-sensitive". If we define it
as "all operations which might handle any collation-sensitive element",
all functions/operators which take any of character data types (text,
varchar, bpchar, sql_identifier, etc.) are unable to be pushed down.

Regards,
--
Shigeru Hanada

(2011/12/13 14:46), Tom Lane wrote:
> Shigeru Hanada<shigeru(dot)hanada(at)gmail(dot)com> writes:
>> Agreed. How about to add a per-column boolean FDW option, say
>> "pushdown", to pgsql_fdw? Users can tell pgsql_fdw that the column can
>> be pushed down safely by setting this option to true.
>
> [ itch... ] That doesn't seem like the right level of granularity.
> ISTM the problem is with whether specific operators have the same
> meaning at the far end as they do locally. If you try to attach the
> flag to columns, you have to promise that *every* operator on that
> column means what it does locally, which is likely to not be the
> case ever if you look hard enough. Plus, having to set the flag on
> each individual column of the same datatype seems pretty tedious.

Indeed, I too think that labeling on each columns is not the best way,
but at that time I thought that it's a practical way, in a way. IOW, I
chose per-column FDW options as a compromise between never-push-down and
indiscriminate-push-down.

Anyway, ISTM that we should consider various mapping for
functions, operators and collations to support push-down in general
way, but it would be hard to accomplish in this CF.

Here I'd like to propose three incremental patches:

1) fdw_helper_funcs_v3.patch: This is not specific to pgsql_fdw, but
probably useful for every FDWs which use FDW options. This patch
provides some functions which help retrieving FDW options from catalogs.
This patch also enhances document about existing FDW helper functions.

2) pgsql_fdw_v5.patch: This patch provides simple pgsql_fdw
which does *NOT* support any push-down. All data in remote table are
retrieved for each foreign scan, and conditions are always evaluated on
local side. This is safe about semantics difference between local and
remote, but very inefficient especially for large remote tables.

3) pgsql_fdw_pushdown_v1.patch: This patch adds limited push-down
capability to pgsql_fdw which is implemented by previous patch. The
criteria for pushing down is little complex. I modified pgsql_fdw to
*NOT* push down conditions which contain any of:

a) expression whose result collation is valid
b) expression whose input collation is valid
c) expression whose result type is user-defined
d) expression which uses user-defined function
e) array expression whose elements has user-defined type
f) expression which uses user-defined operator
g) expression which uses mutable function

As the result, pgsql_fdw can push down very limited conditions such as
numeric comparisons, but it would be still useful. I hope that these
restriction are enough to avoid problems about semantics difference
between remote and local.

To implement d), I added exprFunction to nodefuncs.c which returns Oid
of function which is used in the expression node, but I'm not sure that
it should be there. Should we have it inside pgsql_fdw?

I'd like to thank everyone who commented on this topic!

Regards,
--
Shigeru Hanada

On 12/14/2011 09:02 AM, Shigeru Hanada wrote:
> Here I'd like to propose three incremental patches:
>
> 1) fdw_helper_funcs_v3.patch...: This is not specific to pgsql_fdw, but
> probably useful for every FDWs which use FDW options...
> 2) pgsql_fdw_v5.patch: This patch provides simple pgsql_fdw
> which does *NOT* support any push-down...
> 3) pgsql_fdw_pushdown_v1.patch: This patch adds limited push-down
> capability to pgsql_fdw which is implemented by previous patch...
> ...
> To implement [expression which uses user-defined function], I added exprFunction to nodefuncs.c which returns Oid
> of function which is used in the expression node, but I'm not sure that
> it should be there. Should we have it inside pgsql_fdw?

After failing to bring some light onto this during my general update,
will try again here. We now have 3 updated patches that refactor things
from how this was originally presented, with one asked implementation
question. There's also a spawned off "Join push-down for foreign tables"
patch off in another thread.

I don't think it's really clear to everyone what state this feature
proposal is in. We've gotten bits of review here from KaiGai and Heikki,
big picture comments from Robert and Tom. Given how these are
structured, is fdw_helper_funcs_v3.patch at the point where it should be
considered for committer review? Maybe pgsql_fdw_v5.patch too?

The others seem to be more in flux to me, due to all the recent pushdown
changes.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us

Hi Harada-san,

I checked the "fdw_helper_funcs_v3.patch", "pgsql_fdw_v5.patch" and
"pgsql_fdw_pushdown_v1.patch". My comments are below.

[BUG]
Even though pgsql_fdw tries to push-down qualifiers being executable
on the remove side at the deparseSql(), it does not remove qualifiers
being pushed down from the baserel->baserestrictinfo, thus, these
qualifiers are eventually executed twice.

See the result of this EXPLAIN.
postgres=# EXPLAIN SELECT * FROM ft1 WHERE a > 2 AND f_leak(b);
QUERY PLAN
------------------------------------------------------------------------------------------------------
Foreign Scan on ft1 (cost=107.43..122.55 rows=410 width=36)
Filter: (f_leak(b) AND (a > 2))
Remote SQL: DECLARE pgsql_fdw_cursor_0 SCROLL CURSOR FOR SELECT
a, b FROM public.t1 WHERE (a > 2)
(3 rows)

My expectation is (a > 2) being executed on the remote-side and f_leak(b)
being executed on the local-side. But, filter of foreign-scan on ft1 has both
of qualifiers. It has to be removed, if a RestrictInfo get pushed-down.

[Design comment]
I'm not sure the reason why store_result() uses MessageContext to save
the Tuplestorestate within PgsqlFdwExecutionState.
The source code comment says it is used to avoid memory leaks in error
cases. I also have a similar experience on implementation of my fdw module,
so, I could understand per-scan context is already cleared at the timing of
resource-release-callback, thus, handlers to external resource have to be
saved on separated memory context.
In my personal opinion, the right design is to declare a memory context for
pgsql_fdw itself, instead of the abuse of existing memory context.
(More wise design is to define sub-memory-context for each foreign-scan,
then, remove the sub-memory-context after release handlers.)

[Design comment]
When "BEGIN" should be issued on the remote-side?
The connect_pg_server() is an only chance to issue "BEGIN" command
at the remote-side on connection being opened. However, the connection
shall be kept unless an error is not raised. Thus, the remote-side will
continue to work within a single transaction block, even if local-side iterates
a pair of "BEGIN" and "COMMIT".
I'd like to suggest to close the transaction block at the timing of either
end of the scan, transaction or sub-transaction.

[Comment to Improve]
Also, which transaction isolation level should be specified in this case?
An idea is its isolation level is specified according to the current isolation
level on the local-side.
(Of course, it is your choice if it is not supported right now.)

[Comment to improve]
It seems to me the design of exprFunction is not future-proof, if we add
a new node type that contains two or more function calls, because it can
return an oid of functions.
I think, the right design is to handle individual node-types within the
large switch statement at foreign_expr_walker().
Of course, it is just my sense.

[Comment to improve]
The pgsql_fdw_handler() allocates FdwRoutine using makeNode(),
then it set function-pointers on each fields.
Why don't you return a pointer to statically declared FdwRoutine
variable being initialized at compile time, like:

static FdwRoutine pgsql_fdw_handler = {
.type = T_FdwRoutine,
.PlanForeignScan = pgsqlPlanForeignScan,
.ExplainForeignScan = pgsqlExplainForeignScan,
.BeginForeignScan = pgsqlBeginForeignScan,
.IterateForeignScan = pgsqlIterateForeignScan,
.ReScanForeignScan = pgsqlReScanForeignScan,
.EndForeignScan = pgsqlEndForeignScan,
};

Datum
pgsql_fdw_handler(PG_FUNCTION_ARGS)
{
PG_RETURN_POINTER(&pgsql_fdw_handler);
}

[Question to implementation]
At pgsqlIterateForeignScan(), it applies null-check on festate->tuples
and bool-checks on festete->cursor_opened.
Do we have a possible scenario that festate->tuples is not null, but
festate->cursor_opened, or an opposite combination?
If null-check on festate->tuples is enough to detect the first call of
the iterate callback, it is not my preference to have redundant flag.

Thanks,

2011年12月14日15:02 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> (2011/12/13 14:46), Tom Lane wrote:
>> Shigeru Hanada<shigeru(dot)hanada(at)gmail(dot)com> writes:
>>> Agreed. How about to add a per-column boolean FDW option, say
>>> "pushdown", to pgsql_fdw? Users can tell pgsql_fdw that the column can
>>> be pushed down safely by setting this option to true.
>>
>> [ itch... ] That doesn't seem like the right level of granularity.
>> ISTM the problem is with whether specific operators have the same
>> meaning at the far end as they do locally. If you try to attach the
>> flag to columns, you have to promise that *every* operator on that
>> column means what it does locally, which is likely to not be the
>> case ever if you look hard enough. Plus, having to set the flag on
>> each individual column of the same datatype seems pretty tedious.
>
> Indeed, I too think that labeling on each columns is not the best way,
> but at that time I thought that it's a practical way, in a way. IOW, I
> chose per-column FDW options as a compromise between never-push-down and
> indiscriminate-push-down.
>
> Anyway, ISTM that we should consider various mapping for
> functions, operators and collations to support push-down in general
> way, but it would be hard to accomplish in this CF.
>
> Here I'd like to propose three incremental patches:
>
> 1) fdw_helper_funcs_v3.patch: This is not specific to pgsql_fdw, but
> probably useful for every FDWs which use FDW options. This patch
> provides some functions which help retrieving FDW options from catalogs.
> This patch also enhances document about existing FDW helper functions.
>
> 2) pgsql_fdw_v5.patch: This patch provides simple pgsql_fdw
> which does *NOT* support any push-down. All data in remote table are
> retrieved for each foreign scan, and conditions are always evaluated on
> local side. This is safe about semantics difference between local and
> remote, but very inefficient especially for large remote tables.
>
> 3) pgsql_fdw_pushdown_v1.patch: This patch adds limited push-down
> capability to pgsql_fdw which is implemented by previous patch. The
> criteria for pushing down is little complex. I modified pgsql_fdw to
> *NOT* push down conditions which contain any of:
>
> a) expression whose result collation is valid
> b) expression whose input collation is valid
> c) expression whose result type is user-defined
> d) expression which uses user-defined function
> e) array expression whose elements has user-defined type
> f) expression which uses user-defined operator
> g) expression which uses mutable function
>
> As the result, pgsql_fdw can push down very limited conditions such as
> numeric comparisons, but it would be still useful. I hope that these
> restriction are enough to avoid problems about semantics difference
> between remote and local.
>
> To implement d), I added exprFunction to nodefuncs.c which returns Oid
> of function which is used in the expression node, but I'm not sure that
> it should be there. Should we have it inside pgsql_fdw?
>
> I'd like to thank everyone who commented on this topic!
>
> Regards,
> --
> Shigeru Hanada

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

2012/1/29 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
>     Remote SQL: DECLARE pgsql_fdw_cursor_0 SCROLL CURSOR FOR SELECT
> a, b FROM public.t1 WHERE (a > 2)
>  (3 rows)

Shouldn't we be using protocol-level cursors rather than SQL-level cursors?

> [Design comment]
> When "BEGIN" should be issued on the remote-side?
> The connect_pg_server() is an only chance to issue "BEGIN" command
> at the remote-side on connection being opened. However, the connection
> shall be kept unless an error is not raised. Thus, the remote-side will
> continue to work within a single transaction block, even if local-side iterates
> a pair of "BEGIN" and "COMMIT".
> I'd like to suggest to close the transaction block at the timing of either
> end of the scan, transaction or sub-transaction.

I suspect this is ultimately going to need to be configurable. Some
people might want to close the transaction on the remote side ASAP,
while other people might want to hold it open until commit. For a
first version I think it's most likely best to do whatever seems
simplest to code, planning to add more options later.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

(2012/01/30 4:39), Kohei KaiGai wrote:
> I checked the "fdw_helper_funcs_v3.patch", "pgsql_fdw_v5.patch" and
> "pgsql_fdw_pushdown_v1.patch". My comments are below.

Thanks for the review!

> [BUG]
> Even though pgsql_fdw tries to push-down qualifiers being executable
> on the remove side at the deparseSql(), it does not remove qualifiers
> being pushed down from the baserel->baserestrictinfo, thus, these
> qualifiers are eventually executed twice.
>
> See the result of this EXPLAIN.
> postgres=# EXPLAIN SELECT * FROM ft1 WHERE a> 2 AND f_leak(b);
> QUERY PLAN
> ------------------------------------------------------------------------------------------------------
> Foreign Scan on ft1 (cost=107.43..122.55 rows=410 width=36)
> Filter: (f_leak(b) AND (a> 2))
> Remote SQL: DECLARE pgsql_fdw_cursor_0 SCROLL CURSOR FOR SELECT
> a, b FROM public.t1 WHERE (a> 2)
> (3 rows)
>
> My expectation is (a> 2) being executed on the remote-side and f_leak(b)
> being executed on the local-side. But, filter of foreign-scan on ft1 has both
> of qualifiers. It has to be removed, if a RestrictInfo get pushed-down.

It's intentional that pgsql_fdw keeps pushed-down qualifier in
baserestrictinfo, because I saw some undesirable behavior when I
implemented so that with such optimization when plan is reused, but it's
not clear to me now. I'll try to recall what I saw...

BTW, I think evaluating pushed-down qualifiers again on local side is
safe and has no semantic problem, though we must pay little for such
overhead. Do you have concern about performance?

> [Design comment]
> I'm not sure the reason why store_result() uses MessageContext to save
> the Tuplestorestate within PgsqlFdwExecutionState.
> The source code comment says it is used to avoid memory leaks in error
> cases. I also have a similar experience on implementation of my fdw module,
> so, I could understand per-scan context is already cleared at the timing of
> resource-release-callback, thus, handlers to external resource have to be
> saved on separated memory context.
> In my personal opinion, the right design is to declare a memory context for
> pgsql_fdw itself, instead of the abuse of existing memory context.
> (More wise design is to define sub-memory-context for each foreign-scan,
> then, remove the sub-memory-context after release handlers.)

I simply chose built-in context which has enough lifespan, but now I
think that using MessageContext directly is not recommended way. As you
say, creating new context as child of MessageContext for each scan in
BeginForeignScan (or first IterateForeignScan) would be better. Please
see attached patch.

One other option is getting rid of tuplestore by holding result rows as
PGresult, and track it for error cases which might happen.
ResourceOwner callback can be used to release PGresult on error, similar
to PGconn.

> [Design comment]
> When "BEGIN" should be issued on the remote-side?
> The connect_pg_server() is an only chance to issue "BEGIN" command
> at the remote-side on connection being opened. However, the connection
> shall be kept unless an error is not raised. Thus, the remote-side will
> continue to work within a single transaction block, even if local-side iterates
> a pair of "BEGIN" and "COMMIT".
> I'd like to suggest to close the transaction block at the timing of either
> end of the scan, transaction or sub-transaction.

Indeed, remote transactions should be terminated at some timing.
Terminating at the end of a scan seems troublesome because a connection
might be shared by multiple scans in a query. I'd prefer aborting
remote transaction at the end of local query. Please see
abort_remote_tx in attached patch.

> [Comment to Improve]
> Also, which transaction isolation level should be specified in this case?
> An idea is its isolation level is specified according to the current isolation
> level on the local-side.
> (Of course, it is your choice if it is not supported right now.)

Choosing same as local seems better. Please see start_remote_tx
function in attached patch.

> [Comment to improve]
> It seems to me the design of exprFunction is not future-proof, if we add
> a new node type that contains two or more function calls, because it can
> return an oid of functions.
> I think, the right design is to handle individual node-types within the
> large switch statement at foreign_expr_walker().
> Of course, it is just my sense.

You mean that exprFunction should have capability to handle multiple
Oids for one node, maybe return List<oid> or something? IMO it's
overkill at this time.
Though I'm not sure that it's reasonable, but exprInputCollation too
seems to not assume that multiple input collation might be stored in one
node.

> [Comment to improve]
> The pgsql_fdw_handler() allocates FdwRoutine using makeNode(),
> then it set function-pointers on each fields.
> Why don't you return a pointer to statically declared FdwRoutine
> variable being initialized at compile time, like:
>
> static FdwRoutine pgsql_fdw_handler = {
> .type = T_FdwRoutine,
> .PlanForeignScan = pgsqlPlanForeignScan,
> .ExplainForeignScan = pgsqlExplainForeignScan,
> .BeginForeignScan = pgsqlBeginForeignScan,
> .IterateForeignScan = pgsqlIterateForeignScan,
> .ReScanForeignScan = pgsqlReScanForeignScan,
> .EndForeignScan = pgsqlEndForeignScan,
> };
>
> Datum
> pgsql_fdw_handler(PG_FUNCTION_ARGS)
> {
> PG_RETURN_POINTER(&pgsql_fdw_handler);
> }

Fixed to static variable without designated initializers, because it's
one of C99 feature. Is there any written policy about using C99
features in PG development? I've read Developer's FAQ (wiki) and code
formatting section of document, but I couldn't find any mention.

> [Question to implementation]
> At pgsqlIterateForeignScan(), it applies null-check on festate->tuples
> and bool-checks on festete->cursor_opened.
> Do we have a possible scenario that festate->tuples is not null, but
> festate->cursor_opened, or an opposite combination?
> If null-check on festate->tuples is enough to detect the first call of
> the iterate callback, it is not my preference to have redundant flag.

[ checking... ] No such scenario. It's undesired remain of obsolete
support of simple SELECT mode; pgsql_fdw once had two fetching mode;
simple SELECT mode for small result and CURSOR mode for huge result.
I've removed cursor_opened from PgsqlFdwExecutionState structure.

[Extra change]
In addition to your comments, I found that some regression tests fail
because current planner produces different plan tree from expected
results, and fixed such tests.

Regards,
--
Shigeru Hanada

(2012/02/01 3:56), Robert Haas wrote:
> 2012/1/29 Kohei KaiGai<kaigai(at)kaigai(dot)gr(dot)jp>:
>> Remote SQL: DECLARE pgsql_fdw_cursor_0 SCROLL CURSOR FOR SELECT
>> a, b FROM public.t1 WHERE (a> 2)
>> (3 rows)
>
> Shouldn't we be using protocol-level cursors rather than SQL-level cursors?

Yes, we should, if we have protocol-level cursor :)
I checked libpq interface but I couldn't find any function for
protocol-level cursor.

>> [Design comment]
>> When "BEGIN" should be issued on the remote-side?
>> The connect_pg_server() is an only chance to issue "BEGIN" command
>> at the remote-side on connection being opened. However, the connection
>> shall be kept unless an error is not raised. Thus, the remote-side will
>> continue to work within a single transaction block, even if local-side iterates
>> a pair of "BEGIN" and "COMMIT".
>> I'd like to suggest to close the transaction block at the timing of either
>> end of the scan, transaction or sub-transaction.
>
> I suspect this is ultimately going to need to be configurable. Some
> people might want to close the transaction on the remote side ASAP,
> while other people might want to hold it open until commit. For a
> first version I think it's most likely best to do whatever seems
> simplest to code, planning to add more options later.

I fixed pgsql_fdw to abort remote transaction at the end of each local
query. I chose this timing because local query might include multiple
scans on same foreign server. I think this would be "ASAP" timing in
your comment.

It would be useful to make length of remote transaction same as local's,
I'll try RegisterXactCallback for this purpose, though we need to
preload FDW module to catch BEGIN preceding query using foreign tables.

--
Shigeru Hanada

On Tue, Jan 31, 2012 at 8:56 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> 2012/1/29 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
>>     Remote SQL: DECLARE pgsql_fdw_cursor_0 SCROLL CURSOR FOR SELECT
>> a, b FROM public.t1 WHERE (a > 2)
>>  (3 rows)
>
> Shouldn't we be using protocol-level cursors rather than SQL-level cursors?

I think you want this instead:

https://commitfest.postgresql.org/action/patch_view?id=769

--
marko

2012年2月1日12:15 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> (2012/01/30 4:39), Kohei KaiGai wrote:
>> I checked the "fdw_helper_funcs_v3.patch", "pgsql_fdw_v5.patch" and
>> "pgsql_fdw_pushdown_v1.patch". My comments are below.
>
> Thanks for the review!
>
>> [BUG]
>> Even though pgsql_fdw tries to push-down qualifiers being executable
>> on the remove side at the deparseSql(), it does not remove qualifiers
>> being pushed down from the baserel->baserestrictinfo, thus, these
>> qualifiers are eventually executed twice.
>>
>> See the result of this EXPLAIN.
>> postgres=# EXPLAIN SELECT * FROM ft1 WHERE a> 2 AND f_leak(b);
>> QUERY PLAN
>> ------------------------------------------------------------------------------------------------------
>> Foreign Scan on ft1 (cost=107.43..122.55 rows=410 width=36)
>> Filter: (f_leak(b) AND (a> 2))
>> Remote SQL: DECLARE pgsql_fdw_cursor_0 SCROLL CURSOR FOR SELECT
>> a, b FROM public.t1 WHERE (a> 2)
>> (3 rows)
>>
>> My expectation is (a> 2) being executed on the remote-side and f_leak(b)
>> being executed on the local-side. But, filter of foreign-scan on ft1 has both
>> of qualifiers. It has to be removed, if a RestrictInfo get pushed-down.
>
> It's intentional that pgsql_fdw keeps pushed-down qualifier in
> baserestrictinfo, because I saw some undesirable behavior when I
> implemented so that with such optimization when plan is reused, but it's
> not clear to me now. I'll try to recall what I saw...
>
> BTW, I think evaluating pushed-down qualifiers again on local side is
> safe and has no semantic problem, though we must pay little for such
> overhead. Do you have concern about performance?
>
Yes. In my opinion, one significant benefit of pgsql_fdw is to execute
qualifiers on the distributed nodes; that enables to utilize multiple
CPU resources efficiently.
Duplicate checks are reliable way to keep invisible tuples being filtered
out, indeed. But it shall degrade one competitive characteristics of the
pgsql_fdw.

https://github.com/kaigai/pg_strom/blob/master/plan.c#L693
In my module, qualifiers being executable on device side are detached
from the baserel->baserestrictinfo, and remaining qualifiers are chained
to the list.
The is_device_executable_qual() is equivalent to is_foreign_expr() in
the pgsql_fdw module.

Of course, it is your decision, and I might miss something.

BTW, what is the undesirable behavior on your previous implementation?

>> [Design comment]
>> I'm not sure the reason why store_result() uses MessageContext to save
>> the Tuplestorestate within PgsqlFdwExecutionState.
>> The source code comment says it is used to avoid memory leaks in error
>> cases. I also have a similar experience on implementation of my fdw module,
>> so, I could understand per-scan context is already cleared at the timing of
>> resource-release-callback, thus, handlers to external resource have to be
>> saved on separated memory context.
>> In my personal opinion, the right design is to declare a memory context for
>> pgsql_fdw itself, instead of the abuse of existing memory context.
>> (More wise design is to define sub-memory-context for each foreign-scan,
>> then, remove the sub-memory-context after release handlers.)
>
> I simply chose built-in context which has enough lifespan, but now I
> think that using MessageContext directly is not recommended way. As you
> say, creating new context as child of MessageContext for each scan in
> BeginForeignScan (or first IterateForeignScan) would be better. Please
> see attached patch.
>
> One other option is getting rid of tuplestore by holding result rows as
> PGresult, and track it for error cases which might happen.
> ResourceOwner callback can be used to release PGresult on error, similar
> to PGconn.
>
If we could have set of results on per-query memory context (thus,
no need to explicit release on error timing), it is more ideal design.
It it possible to implement based on the libpq APIs?

Please note that per-query memory context is already released on
ResourceOwner callback is launched, so, it is unavailable to implement
if libpq requires to release some resource.

>> [Design comment]
>> When "BEGIN" should be issued on the remote-side?
>> The connect_pg_server() is an only chance to issue "BEGIN" command
>> at the remote-side on connection being opened. However, the connection
>> shall be kept unless an error is not raised. Thus, the remote-side will
>> continue to work within a single transaction block, even if local-side iterates
>> a pair of "BEGIN" and "COMMIT".
>> I'd like to suggest to close the transaction block at the timing of either
>> end of the scan, transaction or sub-transaction.
>
> Indeed, remote transactions should be terminated at some timing.
> Terminating at the end of a scan seems troublesome because a connection
> might be shared by multiple scans in a query. I'd prefer aborting
> remote transaction at the end of local query. Please see
> abort_remote_tx in attached patch.
>
It seems to me abort_remote_tx in ReleaseConnection() is reasonable.
However, isn't it needed to have ABORT in GetConnection() at first time?

>> [Comment to Improve]
>> Also, which transaction isolation level should be specified in this case?
>> An idea is its isolation level is specified according to the current isolation
>> level on the local-side.
>> (Of course, it is your choice if it is not supported right now.)
>
> Choosing same as local seems better. Please see start_remote_tx
> function in attached patch.
>
It seems to me reasonable.

>> [Comment to improve]
>> It seems to me the design of exprFunction is not future-proof, if we add
>> a new node type that contains two or more function calls, because it can
>> return an oid of functions.
>> I think, the right design is to handle individual node-types within the
>> large switch statement at foreign_expr_walker().
>> Of course, it is just my sense.
>
> You mean that exprFunction should have capability to handle multiple
> Oids for one node, maybe return List<oid> or something? IMO it's
> overkill at this time.
> Though I'm not sure that it's reasonable, but exprInputCollation too
> seems to not assume that multiple input collation might be stored in one
> node.
>
I'm still skeptical on the current logic to determine whether qualifier
is available to push down, or not.

For example, it checks oid of operator and oid of function being
invoked on this operator at OpExpr.
However, the purpose of this check is to ensure same result on
execution of qualifier, thus it restricts qualifiers to be pushed down
built-in database objects.
So, isn't it enough to check whether oid of OpExpr is built-in, or not?
(If oid of operator is built-in, its function is also built-in. Right?)

>> [Comment to improve]
>> The pgsql_fdw_handler() allocates FdwRoutine using makeNode(),
>> then it set function-pointers on each fields.
>> Why don't you return a pointer to statically declared FdwRoutine
>> variable being initialized at compile time, like:
>>
>> static FdwRoutine pgsql_fdw_handler = {
>> .type = T_FdwRoutine,
>> .PlanForeignScan = pgsqlPlanForeignScan,
>> .ExplainForeignScan = pgsqlExplainForeignScan,
>> .BeginForeignScan = pgsqlBeginForeignScan,
>> .IterateForeignScan = pgsqlIterateForeignScan,
>> .ReScanForeignScan = pgsqlReScanForeignScan,
>> .EndForeignScan = pgsqlEndForeignScan,
>> };
>>
>> Datum
>> pgsql_fdw_handler(PG_FUNCTION_ARGS)
>> {
>> PG_RETURN_POINTER(&pgsql_fdw_handler);
>> }
>
> Fixed to static variable without designated initializers, because it's
> one of C99 feature. Is there any written policy about using C99
> features in PG development? I've read Developer's FAQ (wiki) and code
> formatting section of document, but I couldn't find any mention.
>
OK. It was my wrong suggestion.

>> [Question to implementation]
>> At pgsqlIterateForeignScan(), it applies null-check on festate->tuples
>> and bool-checks on festete->cursor_opened.
>> Do we have a possible scenario that festate->tuples is not null, but
>> festate->cursor_opened, or an opposite combination?
>> If null-check on festate->tuples is enough to detect the first call of
>> the iterate callback, it is not my preference to have redundant flag.
>
> [ checking... ] No such scenario. It's undesired remain of obsolete
> support of simple SELECT mode; pgsql_fdw once had two fetching mode;
> simple SELECT mode for small result and CURSOR mode for huge result.
> I've removed cursor_opened from PgsqlFdwExecutionState structure.
>
OK, I understood the background of this design.

> [Extra change]
> In addition to your comments, I found that some regression tests fail
> because current planner produces different plan tree from expected
> results, and fixed such tests.
>

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

2012/2/2 Marko Kreen <markokr(at)gmail(dot)com>:
> I think you want this instead:
>
> Â https://commitfest.postgresql.org/action/patch_view?id=769

Somehow I've missed this cool feature. Thanks for the suggestion!

--
Shigeru Hanada

Thanks for the comments!

(2012/02/06 5:08), Kohei KaiGai wrote:
> Yes. In my opinion, one significant benefit of pgsql_fdw is to execute
> qualifiers on the distributed nodes; that enables to utilize multiple
> CPU resources efficiently.
> Duplicate checks are reliable way to keep invisible tuples being filtered
> out, indeed. But it shall degrade one competitive characteristics of the
> pgsql_fdw.
>
> https://github.com/kaigai/pg_strom/blob/master/plan.c#L693
> In my module, qualifiers being executable on device side are detached
> from the baserel->baserestrictinfo, and remaining qualifiers are chained
> to the list.
> The is_device_executable_qual() is equivalent to is_foreign_expr() in
> the pgsql_fdw module.

Agreed, I too think that pushed-down qualifiers should not be evaluated
on local side again from the viewpoint of performance.

> Of course, it is your decision, and I might miss something.
>
> BTW, what is the undesirable behavior on your previous implementation?

In early development, maybe during testing PREPARE/EXECUTE or DECALRE, I
saw that iterated execution of foreign scan produce wrong result which
includes rows which are NOT match pushed-down qualifiers. And, at last,
I could recall what happened at that time. It was just trivial bug I
made. Perhaps I've removed pushed-down qualifiers in Path generation
phase, so generated plan node has lost qualifiers permanently.

In short, I'll remove remote qualifiers from baserestrictinfo, like
pg_storm.

>>> [Design comment]
>>> I'm not sure the reason why store_result() uses MessageContext to save
>>> the Tuplestorestate within PgsqlFdwExecutionState.
>>> The source code comment says it is used to avoid memory leaks in error
>>> cases. I also have a similar experience on implementation of my fdw module,
>>> so, I could understand per-scan context is already cleared at the timing of
>>> resource-release-callback, thus, handlers to external resource have to be
>>> saved on separated memory context.
>>> In my personal opinion, the right design is to declare a memory context for
>>> pgsql_fdw itself, instead of the abuse of existing memory context.
>>> (More wise design is to define sub-memory-context for each foreign-scan,
>>> then, remove the sub-memory-context after release handlers.)
>>
>> I simply chose built-in context which has enough lifespan, but now I
>> think that using MessageContext directly is not recommended way. As you
>> say, creating new context as child of MessageContext for each scan in
>> BeginForeignScan (or first IterateForeignScan) would be better. Please
>> see attached patch.
>>
>> One other option is getting rid of tuplestore by holding result rows as
>> PGresult, and track it for error cases which might happen.
>> ResourceOwner callback can be used to release PGresult on error, similar
>> to PGconn.
>>
> If we could have set of results on per-query memory context (thus,
> no need to explicit release on error timing), it is more ideal design.
> It it possible to implement based on the libpq APIs?

Currently no, so I used tuplestore even though it needs coping results.
However, Kyotaro Horiguchi's patch might make it possible. I'm reading
his patch to determine whether it suits pgsql_fdw.

http://archives.postgresql.org/message-id/20120202143057.GA12434@gmail.com

> Please note that per-query memory context is already released on
> ResourceOwner callback is launched, so, it is unavailable to implement
> if libpq requires to release some resource.

I see. We need to use context which has longer lifetime if we want to
track malloc'ed PQresult. I already use CacheContext for connection
pooling, so linking PGreslts to its source connection would be a solutions.

>>> [Design comment]
>>> When "BEGIN" should be issued on the remote-side?
>>> The connect_pg_server() is an only chance to issue "BEGIN" command
>>> at the remote-side on connection being opened. However, the connection
>>> shall be kept unless an error is not raised. Thus, the remote-side will
>>> continue to work within a single transaction block, even if local-side iterates
>>> a pair of "BEGIN" and "COMMIT".
>>> I'd like to suggest to close the transaction block at the timing of either
>>> end of the scan, transaction or sub-transaction.
>>
>> Indeed, remote transactions should be terminated at some timing.
>> Terminating at the end of a scan seems troublesome because a connection
>> might be shared by multiple scans in a query. I'd prefer aborting
>> remote transaction at the end of local query. Please see
>> abort_remote_tx in attached patch.
>>
> It seems to me abort_remote_tx in ReleaseConnection() is reasonable.
> However, isn't it needed to have ABORT in GetConnection() at first time?

Hm, forcing overhead of aborting transaction to all local queries is
unreasonable. Redundant BEGIN doesn't cause error but just generate
WARNING, so I'll remove abort_remote_tx preceding begin_remote_tx.

>>> [Comment to improve]
>>> It seems to me the design of exprFunction is not future-proof, if we add
>>> a new node type that contains two or more function calls, because it can
>>> return an oid of functions.
>>> I think, the right design is to handle individual node-types within the
>>> large switch statement at foreign_expr_walker().
>>> Of course, it is just my sense.
>>
>> You mean that exprFunction should have capability to handle multiple
>> Oids for one node, maybe return List<oid> or something? IMO it's
>> overkill at this time.
>> Though I'm not sure that it's reasonable, but exprInputCollation too
>> seems to not assume that multiple input collation might be stored in one
>> node.
>>
> I'm still skeptical on the current logic to determine whether qualifier
> is available to push down, or not.
>
> For example, it checks oid of operator and oid of function being
> invoked on this operator at OpExpr.
> However, the purpose of this check is to ensure same result on
> execution of qualifier, thus it restricts qualifiers to be pushed down
> built-in database objects.
> So, isn't it enough to check whether oid of OpExpr is built-in, or not?
> (If oid of operator is built-in, its function is also built-in. Right?)

Basically yes. If a built-in operator has different semantics on remote
side (probably pg_operator catalog has been changed manually by
superuser), it can be said that nothing is reliable in the context of
pgsql_fdw. I added checking function's oid of operators just in case,
and now it seems to me paranoid concern...

I'll post revised patches soon.

Regards,
--
Shigeru Hanada

(2012/02/06 17:37), Shigeru Hanada wrote:
> I'll post revised patches soon.

Attached revised patches. Changes from last version are below.

[fdw_helper_v3.patch]
none

[pgsql_fdw_v7.patch]
* Don't abort remote transaction before starting remote transaction.
* Add _PQ_init/_PQ_fini functions
* Fix some typos in comments.
* Revise transaction management section in document.
* Update EXPLAIN sample in document.

[pgsql_fdw_]
* Avoid redundant evaluation of pushed-down quals.
* Fix some typos in comments.
* Update EXPLAIN sample in document.

Regards,
--
Shigeru Hanada

(2012/02/02 18:24), Marko Kreen wrote:
> I think you want this instead:
>
> https://commitfest.postgresql.org/action/patch_view?id=769

With modified version of pgsql_fdw which uses row processor to retrieve
result tuples, I found significant performance gain on simple read-only
pgbench, though scale factor was very small (-s 3). Executed command
was "pgbench -S -n -c 5 T 30".

Average tps (excluding connections establishing) of 3 times measurements
are:

pgsql_fdw with SQL-cursor : 622
pgsql_fdw with Row Processor : 1219 - 2.0x faster than SQL-cursor
w/o pgsql_fdw(direct access) : 7719 - 6.3x faster than Row Processor

Row processor was almost 2x faster than SQL-cursor! I'm looking forward
to this feature.

In addition to performance gain, of course memory usage was kept at very
low level. :)

Regards,
--
Shigeru Hanada

(2012/02/08 20:51), Shigeru Hanada wrote:
> Attached revised patches. Changes from last version are below.
<snip>

I've found and fixed a bug which generates wrong remote query when any
column of a foreign table has been dropped. Also regression test for
this case is added.

I attached only pgsql_fdw_v8.patch, because pgsql_fdw_pushdown_v3.patch
in last post still can be applied onto v8 patch.

Regards,
--
Shigeru Hanada

(2012/02/10 20:39), Shigeru Hanada wrote:
> (2012/02/08 20:51), Shigeru Hanada wrote:
>> Attached revised patches. Changes from last version are below.
> <snip>
>
> I've found and fixed a bug which generates wrong remote query when any
> column of a foreign table has been dropped. Also regression test for
> this case is added.
>
> I attached only pgsql_fdw_v8.patch, because pgsql_fdw_pushdown_v3.patch
> in last post still can be applied onto v8 patch.
>
> Regards,

The patches have been applied, but role-related regression tests failed
in my environment. I fixed it in a similar fashion of
/src/test/regress/sql/foreign_data.sql. Please find attached a updated
patch for the regression tests.

BTW, What do you think about this?

http://archives.postgresql.org/pgsql-hackers/2012-01/msg00229.php

Best regards,
Etsuro Fujita

(2012/02/13 20:50), Etsuro Fujita wrote:
> The patches have been applied, but role-related regression tests failed
> in my environment. I fixed it in a similar fashion of
> /src/test/regress/sql/foreign_data.sql. Please find attached a updated
> patch for the regression tests.

Good catch, thanks. I'll revise pgsql_fdw tests little more.

> BTW, What do you think about this?
>
> http://archives.postgresql.org/pgsql-hackers/2012-01/msg00229.php

I'm sorry that I've left the thread unfinished... I've given up to
propose Join-push-down of foreign tables for 9.2, because it will take a
while to achieve general semantics mapping for join push-down and WHERE
clause push-down. For 9.2, I'm proposing pgsql_fdw which has WHERE
clause push-down for built-in elements which are free from collation.
I'd like to go back to that item after 9.2 development enters beta or
RC, hopefully :)

--
Shigeru Hanada

(2012/02/14 15:15), Shigeru Hanada wrote:
> (2012/02/13 20:50), Etsuro Fujita wrote:
>> The patches have been applied, but role-related regression tests failed
>> in my environment. I fixed it in a similar fashion of
>> /src/test/regress/sql/foreign_data.sql. Please find attached a updated
>> patch for the regression tests.
>
> Good catch, thanks. I'll revise pgsql_fdw tests little more.
>
>> BTW, What do you think about this?
>>
>> http://archives.postgresql.org/pgsql-hackers/2012-01/msg00229.php
>
> I'm sorry that I've left the thread unfinished... I've given up to
> propose Join-push-down of foreign tables for 9.2, because it will take a
> while to achieve general semantics mapping for join push-down and WHERE
> clause push-down. For 9.2, I'm proposing pgsql_fdw which has WHERE
> clause push-down for built-in elements which are free from collation.
> I'd like to go back to that item after 9.2 development enters beta or
> RC, hopefully :)

OK. But my question was about the PlanForeignScan API. As discussed at
that thread, it would have to change the PlanForeignScan API to let the
FDW generate multiple paths and dump them all to add_path instead of
returning a FdwPlan struct. With this change, I think it would also
have to add a new FDW API that is called from create_foreignscan_plan()
and lets the FDW generate foreignscan plan for the base relation scanned
by the best path choosed by postgres optimizer for itself. What do you
think about it?

Best regards,
Etsuro Fujita

(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

(2012/02/14 17:40), Etsuro Fujita wrote:
> OK. But my question was about the PlanForeignScan API.

Sorry for misunderstanding. :(

> As discussed at
> that thread, it would have to change the PlanForeignScan API to let the
> FDW generate multiple paths and dump them all to add_path instead of
> returning a FdwPlan struct. With this change, I think it would also
> have to add a new FDW API that is called from create_foreignscan_plan()
> and lets the FDW generate foreignscan plan for the base relation scanned
> by the best path choosed by postgres optimizer for itself. What do you
> think about it?

Though I have only random thoughts about this issue at the moment...

Multiple valuable Paths for a scan of a foreign table by FDW, but
changing PlanForeignScan to return list of FdwPlan in 9.2 seems too
hasty. It would need more consideration about general interface for
possible results such as:

* Full output (no WHERE push-down) is expensive on both remote and transfer.
* Filtered output (WHERE push-down) has cheap total costs when only few
rows come through the filter.
* Ordered output (ORDER BY push-down) is expensive on remote, but has
chance to omit upper Sort node.
* Aggregated output (GROUP BY push-down) is expensive on remote, but
have chance to omit upper Agg node, and reduces data transfer.
* Limited output (LIMIT/OFFSET push-down) can reduce data transfer, and
have chance to omit upper Limit node.

Currently FDWs can consider only first two, AFAIK. If FDW generates
multiple FdwPlan (Full and Filtered) and sets different start-up costs
and total costs to them (may be former has higher start-up and lower
total than latter), planner would choose better for the whole plan.

In addition to changing FdwRoutine, it seems worth changing FdwPlan too
so that FDWs can return more information to planner, such as pathkeys
and rows, for each possible path.

In short, I have some ideas to enhance foreign table scans, but IMO they
are half-baked and we don't have enough time to achieve them for 9.2.

Regards,
--
Shigeru Hanada

(2012/02/14 19:42), Shigeru Hanada wrote:
> (2012/02/14 17:40), Etsuro Fujita wrote:
>> As discussed at
>> that thread, it would have to change the PlanForeignScan API to let the
>> FDW generate multiple paths and dump them all to add_path instead of
>> returning a FdwPlan struct. With this change, I think it would also
>> have to add a new FDW API that is called from create_foreignscan_plan()
>> and lets the FDW generate foreignscan plan for the base relation scanned
>> by the best path choosed by postgres optimizer for itself. What do you
>> think about it?

> In short, I have some ideas to enhance foreign table scans, but IMO they
> are half-baked and we don't have enough time to achieve them for 9.2.

OK. Thank you for your answer.

Best regards,
Etsuro Fujita

Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com> writes:
> (2012/02/14 17:40), Etsuro Fujita wrote:
>> As discussed at
>> that thread, it would have to change the PlanForeignScan API to let the
>> FDW generate multiple paths and dump them all to add_path instead of
>> returning a FdwPlan struct.

> Multiple valuable Paths for a scan of a foreign table by FDW, but
> changing PlanForeignScan to return list of FdwPlan in 9.2 seems too
> hasty.

I would really like to see that happen in 9.2, because the longer we let
that mistake live, the harder it will be to change. More and more FDWs
are getting written. I don't think it's that hard to do: we just have
to agree that PlanForeignScan should return void and call add_path for
itself, possibly more than once. If we do that, I'm inclined to think
we cou;d get rid of the separate Node type FdwPlan, and just incorporate
"List *fdw_private" into ForeignPath and ForeignScan.

This does mean that FDWs will be a bit more tightly coupled to the
planner, because they'll have to change whenever we add new fields to
struct Path; but that is not really something that happens often.

regards, tom lane

(2012/02/14 23:50), Tom Lane wrote:
> Shigeru Hanada<shigeru(dot)hanada(at)gmail(dot)com> writes:
>> (2012/02/14 17:40), Etsuro Fujita wrote:
>>> As discussed at
>>> that thread, it would have to change the PlanForeignScan API to let the
>>> FDW generate multiple paths and dump them all to add_path instead of
>>> returning a FdwPlan struct.
>
>> Multiple valuable Paths for a scan of a foreign table by FDW, but
>> changing PlanForeignScan to return list of FdwPlan in 9.2 seems too
>> hasty.
>
> I would really like to see that happen in 9.2, because the longer we let
> that mistake live, the harder it will be to change. More and more FDWs
> are getting written. I don't think it's that hard to do: we just have
> to agree that PlanForeignScan should return void and call add_path for
> itself, possibly more than once.

Agreed. I fixed the PlanForeignScan API. Please find attached a patch.

> If we do that, I'm inclined to think
> we cou;d get rid of the separate Node type FdwPlan, and just incorporate
> "List *fdw_private" into ForeignPath and ForeignScan.

+1 While the patch retains the struct FdwPlan, I would like to get rid
of it at next version of the patch.

Best regards,
Etsuro Fujita

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(dot)hanada(at)gmail(dot)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(at)kaigai(dot)gr(dot)jp>

Kaigai-san,

Thanks for the review. Attached patches are revised version, though
only fdw_helper_v5.patch is unchanged.

(2012/02/16 0:09), Kohei KaiGai wrote:
> [memory context of tuple store]
> It calls tuplestore_begin_heap() under the memory context of
> festate->scan_cxt at pgsqlBeginForeignScan.

Yes, it's because tuplestore uses a context which was current when
tuplestore_begin_heap was called. I want to use per-scan context for
tuplestore, to keep its content tuples alive through the scan.

> On the other hand, tuplestore_gettupleslot() is called under the
> memory context of festate->tuples.

Yes, result tuples to be returned to executor should be allocated in
per-scan context and live until next IterateForeignScan (or
EndForeignScan), because such tuple will be released via ExecClearTuple
in next IterateForeignScan call. If we don't switch context to per-scan
context, result tuple is allocated in per-tuple context and cause
double-free and server crash.

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

How do you confirmed that no callback function is invoked on errors? I
think that memory objects acquired within tuplestore_begin_heap (I guess
you mean excluding stored tuples, right?) are released during cleanup of
aborted transaction. I tested that by adding elog(ERROR) to the tail of
store_result() for intentional error, and execute large query 100 times
in a session. I saw VIRT value (via top command) comes down to constant
level after every query.

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

Ah, I've confused context management of pgsql_fdw... I fixed pgsql_fdw
to create per-scan context as a child of es_query_cxt in
BeginForeignScan, and use it for tuplestore of the scan. So, tuplestore
and its contents are released correctly at EndForeignScan, or cleanup of
aborted transaction in error case.

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

During thinking about this comment, I found double-free bug of PGresult
in execute_query, thanks :)

But, sorry, I'm not sure what the concern you show here is. The reason
why copying tuples from malloc'ed area to palloc'ed area is to release
PGresult before returning from the IterateForeingScan call. The reason
why using PG_TRY block is to sure that PGresult is released before jump
back to upstream in error case.

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

Fixed. BTW, just out of curiosity, which compiler do you use? My
compiler ,gcc (GCC) 4.6.0 20110603 (Red Hat 4.6.0-10) on Fedora 15,
doesn't warn it.

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

Fixed.

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

Got rid of exprFunction and fixed foreign_expr_walker to check function
oid in each case label.

Regards,
--
Shigeru Hanada

Shigeru Hanada wrote:
> Thanks for the review. Attached patches are revised version, though
> only fdw_helper_v5.patch is unchanged.

Two questions:
- Is it on purpose that you can specify all SSL client options
except "sslcompression"?
- Since a rescan is done by rewinding the cursor, is it necessary
to have any other remote isolation level than READ COMMITED?
There is only one query issued per transaction.

Yours,
Laurenz Albe

I found a strange behavior with v10. Is it available to reproduce?

In case of "ftbl" is declared as follows:
postgres=# select * FROM ftbl;
a | b
---+-----
1 | aaa
2 | bbb
3 | ccc
4 | ddd
5 | eee
(5 rows)

I tried to raise an error on remote side.

postgres=# select * FROM ftbl WHERE 100 / (a - 3) > 0;
The connection to the server was lost. Attempting reset: Failed.
The connection to the server was lost. Attempting reset: Failed.
!> \q

Its call-trace was:

(gdb) bt
#0 0x00000031030810a4 in free () from /lib64/libc.so.6
#1 0x00007f2caa620bd9 in PQclear (res=0x2102500) at fe-exec.c:679
#2 0x00007f2caa83c4db in execute_query (node=0x20f20a0) at pgsql_fdw.c:722
#3 0x00007f2caa83c64a in pgsqlIterateForeignScan (node=0x20f20a0)
at pgsql_fdw.c:402
#4 0x00000000005c120f in ForeignNext (node=0x20f20a0) at nodeForeignscan.c:50
#5 0x00000000005a9b37 in ExecScanFetch (recheckMtd=0x5c11c0 <ForeignRecheck>,
accessMtd=0x5c11d0 <ForeignNext>, node=0x20f20a0) at execScan.c:82
#6 ExecScan (node=0x20f20a0, accessMtd=0x5c11d0 <ForeignNext>,
recheckMtd=0x5c11c0 <ForeignRecheck>) at execScan.c:132
#7 0x00000000005a2128 in ExecProcNode (node=0x20f20a0) at execProcnode.c:441
#8 0x000000000059edc2 in ExecutePlan (dest=0x210f280,
direction=<optimized out>, numberTuples=0, sendTuples=1 '\001',
operation=CMD_SELECT, planstate=0x20f20a0, estate=0x20f1f88)
at execMain.c:1449

This is the PG_CATCH block at execute_query(). fetch_result() raises
an error, then it shall be catched to release PGresult.
Although "res" should be NULL at this point, PQclear was called with
a non-zero value according to the call trace.

More strangely, I tried to inject elog(INFO, ...) to show the value of "res"
at this point. Then, it become unavailable to reproduce when I tried to
show the pointer of "res" with elog(INFO, "&res = %p", &res);

Why the "res" has a non-zero value, even though it was cleared prior
to fetch_result() and an error was raised within this function?

Thanks,

2012年2月16日13:41 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> Kaigai-san,
>
> Thanks for the review. Attached patches are revised version, though
> only fdw_helper_v5.patch is unchanged.
>
> (2012/02/16 0:09), Kohei KaiGai wrote:
>> [memory context of tuple store]
>> It calls tuplestore_begin_heap() under the memory context of
>> festate->scan_cxt at pgsqlBeginForeignScan.
>
> Yes, it's because tuplestore uses a context which was current when
> tuplestore_begin_heap was called. I want to use per-scan context for
> tuplestore, to keep its content tuples alive through the scan.
>
>> On the other hand, tuplestore_gettupleslot() is called under the
>> memory context of festate->tuples.
>
> Yes, result tuples to be returned to executor should be allocated in
> per-scan context and live until next IterateForeignScan (or
> EndForeignScan), because such tuple will be released via ExecClearTuple
> in next IterateForeignScan call. If we don't switch context to per-scan
> context, result tuple is allocated in per-tuple context and cause
> double-free and server crash.
>
>> 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.
>
> How do you confirmed that no callback function is invoked on errors? I
> think that memory objects acquired within tuplestore_begin_heap (I guess
> you mean excluding stored tuples, right?) are released during cleanup of
> aborted transaction. I tested that by adding elog(ERROR) to the tail of
> store_result() for intentional error, and execute large query 100 times
> in a session. I saw VIRT value (via top command) comes down to constant
> level after every query.
>
>> 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?
>
> Ah, I've confused context management of pgsql_fdw... I fixed pgsql_fdw
> to create per-scan context as a child of es_query_cxt in
> BeginForeignScan, and use it for tuplestore of the scan. So, tuplestore
> and its contents are released correctly at EndForeignScan, or cleanup of
> aborted transaction in error case.
>
>> 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.
>
> During thinking about this comment, I found double-free bug of PGresult
> in execute_query, thanks :)
>
> But, sorry, I'm not sure what the concern you show here is. The reason
> why copying tuples from malloc'ed area to palloc'ed area is to release
> PGresult before returning from the IterateForeingScan call. The reason
> why using PG_TRY block is to sure that PGresult is released before jump
> back to upstream in error case.
>
>> [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.
>
> Fixed. BTW, just out of curiosity, which compiler do you use? My
> compiler ,gcc (GCC) 4.6.0 20110603 (Red Hat 4.6.0-10) on Fedora 15,
> doesn't warn it.
>
>> 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.
>
> Fixed.
>
>> 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.
>
> Got rid of exprFunction and fixed foreign_expr_walker to check function
> oid in each case label.
>
> Regards,
> --
> Shigeru Hanada

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

2012年2月16日13:41 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> Kaigai-san,
>
> Thanks for the review. Attached patches are revised version, though
> only fdw_helper_v5.patch is unchanged.
>
> (2012/02/16 0:09), Kohei KaiGai wrote:
>> [memory context of tuple store]
>> It calls tuplestore_begin_heap() under the memory context of
>> festate->scan_cxt at pgsqlBeginForeignScan.
>
> Yes, it's because tuplestore uses a context which was current when
> tuplestore_begin_heap was called. I want to use per-scan context for
> tuplestore, to keep its content tuples alive through the scan.
>
>> On the other hand, tuplestore_gettupleslot() is called under the
>> memory context of festate->tuples.
>
> Yes, result tuples to be returned to executor should be allocated in
> per-scan context and live until next IterateForeignScan (or
> EndForeignScan), because such tuple will be released via ExecClearTuple
> in next IterateForeignScan call. If we don't switch context to per-scan
> context, result tuple is allocated in per-tuple context and cause
> double-free and server crash.
>
>> 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.
>
> How do you confirmed that no callback function is invoked on errors? I
> think that memory objects acquired within tuplestore_begin_heap (I guess
> you mean excluding stored tuples, right?) are released during cleanup of
> aborted transaction. I tested that by adding elog(ERROR) to the tail of
> store_result() for intentional error, and execute large query 100 times
> in a session. I saw VIRT value (via top command) comes down to constant
> level after every query.
>
Oops, I overlooked the point where MessageContext and its children get
reset. However, as its name, I don't believe it was right usage of memory
context.
As the latest version doing, es_query_cxt is the right way to acquire
memory object with per-query duration.

>> 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?
>
> Ah, I've confused context management of pgsql_fdw... I fixed pgsql_fdw
> to create per-scan context as a child of es_query_cxt in
> BeginForeignScan, and use it for tuplestore of the scan. So, tuplestore
> and its contents are released correctly at EndForeignScan, or cleanup of
> aborted transaction in error case.
>
I believe it is right direction.

>> 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.
>
> During thinking about this comment, I found double-free bug of PGresult
> in execute_query, thanks :)
>
Unfortunately, I found the strange behavior around this code.
I doubt an interaction between longjmp and compiler optimization,
but it is not certain right now.

I'd like to push this patch to committer reviews after this problem got closed.

Right now, I don't have comments on this patch any more.

> But, sorry, I'm not sure what the concern you show here is. The reason
> why copying tuples from malloc'ed area to palloc'ed area is to release
> PGresult before returning from the IterateForeingScan call. The reason
> why using PG_TRY block is to sure that PGresult is released before jump
> back to upstream in error case.
>
>> [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.
>
> Fixed. BTW, just out of curiosity, which compiler do you use? My
> compiler ,gcc (GCC) 4.6.0 20110603 (Red Hat 4.6.0-10) on Fedora 15,
> doesn't warn it.
>
I uses Fedora 16, and GCC 4.6.2.

[kaigai(at)iwashi pgsql_fdw]$ gcc --version
gcc (GCC) 4.6.2 20111027 (Red Hat 4.6.2-1)

It is not a matter related to compiler version, but common manner in
PostgreSQL code. You can likely found source code comments
like "/* keep compiler quiet */"

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

(2012/02/17 0:15), Albe Laurenz wrote:
> Shigeru Hanada wrote:
>> Thanks for the review. Attached patches are revised version, though
>> only fdw_helper_v5.patch is unchanged.
>
> Two questions:
> - Is it on purpose that you can specify all SSL client options
> except "sslcompression"?

No, just an oversight. Good catch.

> - Since a rescan is done by rewinding the cursor, is it necessary
> to have any other remote isolation level than READ COMMITED?
> There is only one query issued per transaction.

If multiple foreign tables on a foreign server is used in a local query,
multiple queries are executed in a remote transaction. So IMO isolation
levels are useful even if remote query is executed only once.

Regards,
--
Shigeru Hanada

(2012/02/17 2:02), Kohei KaiGai wrote:
> I found a strange behavior with v10. Is it available to reproduce?
<snip>
> I tried to raise an error on remote side.
>
> postgres=# select * FROM ftbl WHERE 100 / (a - 3)> 0;
> The connection to the server was lost. Attempting reset: Failed.
> The connection to the server was lost. Attempting reset: Failed.
> !> \q

I could reproduce the error by omitting CFLAGS=-O0 from configure
option. I usually this for coding environment so that gdb debugging
works correctly, so I haven't noticed this issue. I should test
optimized environment too...

Expected result in that case is:

postgres=# select * from pgbench_accounts where 100 / (aid - 3) > 0;
ERROR: could not fetch rows from foreign server
DETAIL: ERROR: division by zero

HINT: FETCH 10000 FROM pgsql_fdw_cursor_0
postgres=#

> This is the PG_CATCH block at execute_query(). fetch_result() raises
> an error, then it shall be catched to release PGresult.
> Although "res" should be NULL at this point, PQclear was called with
> a non-zero value according to the call trace.
>
> More strangely, I tried to inject elog(INFO, ...) to show the value of "res"
> at this point. Then, it become unavailable to reproduce when I tried to
> show the pointer of "res" with elog(INFO, "&res = %p",&res);
>
> Why the "res" has a non-zero value, even though it was cleared prior
> to fetch_result() and an error was raised within this function?

I've found the the problem is uninitialized PGresult variables.
Uninitialized PGresult pointer is used in some places, so its value is
garbage in PG_CATCH block when assignment code has been interrupted by
longjmp.

Probably recommended style would be like this:

<pseudo_code>
PGresult *res = NULL; /* must be NULL in PG_CATCH */

PG_TRY();
{
res = func_might_throw_exception();
if (PQstatus(res) != PGRES_xxx_OK)
{
/* error handling, pass message to caller */
ereport(ERROR, ...);
}

/* success case, use result of query and release it */
...
PQclear(res);
}
PG_CATCH();
{
PQclear(res);
PG_RE_THROW();
/* caller should catch this exception. */
}
</pseudo_code>

I misunderstood that PGresult pointer always has valid value after that
line, because I had wrote assignment of PGresult pointer before PG_TRY
block. Fixes for this issue are:

(1) Initialize PGresult pointer with NULL, if it is used in PG_CATCH.
(2) Move PGresult assignment into PG_TRY block so that we can get
compiler warning of uninitialized variable, just in case.

Please find attached a patch including fixes for this issue.

Regards,
--
Shigeru Hanada

Shigeru Hanada wrote:
>> - Since a rescan is done by rewinding the cursor, is it necessary
>> to have any other remote isolation level than READ COMMITED?
>> There is only one query issued per transaction.
>
> If multiple foreign tables on a foreign server is used in a local
query,
> multiple queries are executed in a remote transaction. So IMO
isolation
> levels are useful even if remote query is executed only once.

Oh, I see. You are right.

Yours,
Laurenz Albe

2012年2月17日6:08 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> (2012/02/17 2:02), Kohei KaiGai wrote:
>> I found a strange behavior with v10. Is it available to reproduce?
> <snip>
>> I tried to raise an error on remote side.
>>
>> postgres=# select * FROM ftbl WHERE 100 / (a - 3)> 0;
>> The connection to the server was lost. Attempting reset: Failed.
>> The connection to the server was lost. Attempting reset: Failed.
>> !> \q
>
> I could reproduce the error by omitting CFLAGS=-O0 from configure
> option. I usually this for coding environment so that gdb debugging
> works correctly, so I haven't noticed this issue. I should test
> optimized environment too...
>
> Expected result in that case is:
>
> postgres=# select * from pgbench_accounts where 100 / (aid - 3) > 0;
> ERROR: could not fetch rows from foreign server
> DETAIL: ERROR: division by zero
>
> HINT: FETCH 10000 FROM pgsql_fdw_cursor_0
> postgres=#
>
>> This is the PG_CATCH block at execute_query(). fetch_result() raises
>> an error, then it shall be catched to release PGresult.
>> Although "res" should be NULL at this point, PQclear was called with
>> a non-zero value according to the call trace.
>>
>> More strangely, I tried to inject elog(INFO, ...) to show the value of "res"
>> at this point. Then, it become unavailable to reproduce when I tried to
>> show the pointer of "res" with elog(INFO, "&res = %p",&res);
>>
>> Why the "res" has a non-zero value, even though it was cleared prior
>> to fetch_result() and an error was raised within this function?
>
> I've found the the problem is uninitialized PGresult variables.
> Uninitialized PGresult pointer is used in some places, so its value is
> garbage in PG_CATCH block when assignment code has been interrupted by
> longjmp.
>
> Probably recommended style would be like this:
>
> <pseudo_code>
> PGresult *res = NULL; /* must be NULL in PG_CATCH */
>
> PG_TRY();
> {
> res = func_might_throw_exception();
> if (PQstatus(res) != PGRES_xxx_OK)
> {
> /* error handling, pass message to caller */
> ereport(ERROR, ...);
> }
>
> /* success case, use result of query and release it */
> ...
> PQclear(res);
> }
> PG_CATCH();
> {
> PQclear(res);
> PG_RE_THROW();
> /* caller should catch this exception. */
> }
> </pseudo_code>
>
> I misunderstood that PGresult pointer always has valid value after that
> line, because I had wrote assignment of PGresult pointer before PG_TRY
> block. Fixes for this issue are:
>
> (1) Initialize PGresult pointer with NULL, if it is used in PG_CATCH.
> (2) Move PGresult assignment into PG_TRY block so that we can get
> compiler warning of uninitialized variable, just in case.
>
> Please find attached a patch including fixes for this issue.
>
I marked this patch as "Ready for Committer", since I have nothing to
comment any more.

I'd like committer help to review this patch and it get merged.

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

I wrote:
> Shigeru Hanada wrote:
>>> - Since a rescan is done by rewinding the cursor, is it necessary
>>> to have any other remote isolation level than READ COMMITED?
>>> There is only one query issued per transaction.
>>
>> If multiple foreign tables on a foreign server is used in a local
query,
>> multiple queries are executed in a remote transaction. So IMO
isolation
>> levels are useful even if remote query is executed only once.
>
> Oh, I see. You are right.

I thought some more about this and changed my mind.

If your query involves foreign scans on two foreign tables on the same
foreign server, these should always see the same snapshot, because
that's how it works with two scans in one query on local tables.

So I think it should be REPEATABLE READ in all cases - SERIALIZABLE
is not necessary as long as all you do is read.

Yours,
Laurenz Albe

"Albe Laurenz" <laurenz(dot)albe(at)wien(dot)gv(dot)at> wrote:

> If your query involves foreign scans on two foreign tables on the
> same foreign server, these should always see the same snapshot,
> because that's how it works with two scans in one query on local
> tables.

That makes sense.

> So I think it should be REPEATABLE READ in all cases -
> SERIALIZABLE is not necessary as long as all you do is read.

That depends on whether you only want to see states of the database
which are consistent with later states of the database and any
invariants enforced by triggers or other software. See this example
of how a read-only transaction can see a bogus state at REPEATABLE
READ or less strict transaction isolation:

http://wiki.postgresql.org/wiki/SSI#Read_Only_Transactions

Perhaps if the transaction using the pgsql_fdw is running at the
SERIALIZABLE transaction isolation level, it should run the queries
at the that level, otherwise at REPEATABLE READ.

-Kevin

Kevin Grittner wrote:
> > If your query involves foreign scans on two foreign tables on the
> > same foreign server, these should always see the same snapshot,
> > because that's how it works with two scans in one query on local
> > tables.
>
> That makes sense.

> > So I think it should be REPEATABLE READ in all cases -
> > SERIALIZABLE is not necessary as long as all you do is read.
>
> That depends on whether you only want to see states of the database
> which are consistent with later states of the database and any
> invariants enforced by triggers or other software. See this example
> of how a read-only transaction can see a bogus state at REPEATABLE
> READ or less strict transaction isolation:
>
> http://wiki.postgresql.org/wiki/SSI#Read_Only_Transactions
>
> Perhaps if the transaction using the pgsql_fdw is running at the
> SERIALIZABLE transaction isolation level, it should run the queries
> at the that level, otherwise at REPEATABLE READ.

I read the example carefully, and it seems to me that it is necessary
for the read-only transaction (T3) to be SERIALIZABLE so that
T1 is aborted and the state that T3 saw remains valid.

If I understand right, I agree with your correction.

Yours,
Laurenz Albe

"Albe Laurenz" <laurenz(dot)albe(at)wien(dot)gv(dot)at> wrote:

> I read the example carefully, and it seems to me that it is
> necessary for the read-only transaction (T3) to be SERIALIZABLE so
> that T1 is aborted and the state that T3 saw remains valid.

Correct.

> If I understand right, I agree with your correction.

:-)

-Kevin

2012/02/21 0:58 "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov>:
>
> "Albe Laurenz" <laurenz(dot)albe(at)wien(dot)gv(dot)at> wrote:
>
> > I read the example carefully, and it seems to me that it is
> > necessary for the read-only transaction (T3)v to be SERIALIZABLE so
> > that T1 is aborted and the state that T3 saw remains valid.
>
> Correct.
Hm, agreed that isolation levels < REPEATABLE READ are not sufficient for
pgsql_fdw's usage. I'll examine the example and fix pgsql_fdw.

Thanks.

(2012/02/15 20:50), Etsuro Fujita wrote:
> (2012/02/14 23:50), Tom Lane wrote:
>>> (2012/02/14 17:40), Etsuro Fujita wrote:
>>>> As discussed at
>>>> that thread, it would have to change the PlanForeignScan API to let the
>>>> FDW generate multiple paths and dump them all to add_path instead of
>>>> returning a FdwPlan struct.

>> I would really like to see that happen in 9.2, because the longer we let
>> that mistake live, the harder it will be to change. More and more FDWs
>> are getting written. I don't think it's that hard to do: we just have
>> to agree that PlanForeignScan should return void and call add_path for
>> itself, possibly more than once.
>
> Agreed. I fixed the PlanForeignScan API. Please find attached a patch.
>
>> If we do that, I'm inclined to think
>> we cou;d get rid of the separate Node type FdwPlan, and just incorporate
>> "List *fdw_private" into ForeignPath and ForeignScan.
>
> +1 While the patch retains the struct FdwPlan, I would like to get rid
> of it at next version of the patch.

Please find attached an updated version of the patch.

Best regards,
Etsuro Fujita

(2012/02/21 8:07), Shigeru Hanada wrote:
> Hm, agreed that isolation levels< REPEATABLE READ are not sufficient for
> pgsql_fdw's usage. I'll examine the example and fix pgsql_fdw.

Attached patch uses "safe" isolation level for remote transactions.
After this change, pgsql_fdw uses levels below:

local | remote
------------------+-----------------
SERIALIZABLE | SERIALIZABLE
REPEATABLE READ | REPEATABLE READ
READ COMMITTED | REPEATABLE READ
READ UNCOMMITTED | REPEATABLE READ

Please review document of pgsql_fdw too.

In addition, I've removed #ifdef from options.c, so that we can specify
libpq options about SSL and Kerberos even if such feature has not been
configured on the environment where pgsql_fdw was built. This change
also avoids regression test failure on environment where any of
--with-openssl or --with-krb5 was specified for configure script.

Regards,
--
Shigeru Hanada

Could we name this "postgresql_fdw" instead? We already have several
${productname}_fdw out there, and I don't want to get in the business of
having to guess variant spellings.

(2012/02/25 7:31), Peter Eisentraut wrote:
> Could we name this "postgresql_fdw" instead? We already have several
> ${productname}_fdw out there, and I don't want to get in the business of
> having to guess variant spellings.

I worry name conflict with existing postgresql_fdw_validator, which is
implemented in backend binary and used by contrib/dblink. I thought
that we should use another name for PostgreSQL FDW unless we can change
specification of dblink connection string.

--
Shigeru Hanada

On Fri, Feb 24, 2012 at 5:31 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> Could we name this "postgresql_fdw" instead?  We already have several
> ${productname}_fdw out there, and I don't want to get in the business of
> having to guess variant spellings.

If you don't like variant spellings, having anything to do with
PostgreSQL, aka Postgres, and usually discussed on the pgsql-* mailing
lists, is probably a bad idea.

Go Postgre!

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Feb 24, 2012 at 5:31 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>> Could we name this "postgresql_fdw" instead? We already have several
>> ${productname}_fdw out there, and I don't want to get in the business of
>> having to guess variant spellings.

> If you don't like variant spellings, having anything to do with
> PostgreSQL, aka Postgres, and usually discussed on the pgsql-* mailing
> lists, is probably a bad idea.

[ snicker ] But still, Peter has a point: pgsql is not a name for the
product, it's at best an abbreviation. We aren't calling the other
thing orcl_fdw or ora_fdw.

I think either postgres_fdw or postgresql_fdw would be fine.

regards, tom lane

On Tue, Feb 28, 2012 at 11:02 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Fri, Feb 24, 2012 at 5:31 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>>> Could we name this "postgresql_fdw" instead?  We already have several
>>> ${productname}_fdw out there, and I don't want to get in the business of
>>> having to guess variant spellings.
>
>> If you don't like variant spellings, having anything to do with
>> PostgreSQL, aka Postgres, and usually discussed on the pgsql-* mailing
>> lists, is probably a bad idea.
>
> [ snicker ]  But still, Peter has a point: pgsql is not a name for the
> product, it's at best an abbreviation.  We aren't calling the other
> thing orcl_fdw or ora_fdw.
>
> I think either postgres_fdw or postgresql_fdw would be fine.

I liked the shorter name, myself, but I'm not going to make a big deal about it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On tis, 2012-02-28 at 11:20 -0500, Robert Haas wrote:
> > [ snicker ] But still, Peter has a point: pgsql is not a name for
> the
> > product, it's at best an abbreviation. We aren't calling the other
> > thing orcl_fdw or ora_fdw.
> >
> > I think either postgres_fdw or postgresql_fdw would be fine.
>
> I liked the shorter name, myself, but I'm not going to make a big deal
> about it.

Let's at least be clear about the reasons here. The fact that
postgresql_fdw_validator exists means (a) there is a possible naming
conflict that has not been discussed yet, and/or (b) the name is already
settled and we need to think of a way to make postgresql_fdw_validator
work with the new actual FDW.

On Feb 28, 2012, at 8:20 AM, Robert Haas wrote:

> I liked the shorter name, myself, but I'm not going to make a big deal about it.

pg_ is used quite a bit. what about pg_fdw?

David

(2012/02/29 4:07), Peter Eisentraut wrote:
> Let's at least be clear about the reasons here. The fact that
> postgresql_fdw_validator exists means (a) there is a possible naming
> conflict that has not been discussed yet, and/or (b) the name is already
> settled and we need to think of a way to make postgresql_fdw_validator
> work with the new actual FDW.

We can avoid conflict of name by using postgres_fdw or pgsql_fdw, but it
doesn't solve fundamental issue. ISTM that maintaining two similar
validators is wasteful and confusing, and FDW for PostgreSQL should be
just one, at least in the context of core distribution.

Current pgsql_fdw_validator accepts every FDW options which is accepted
by postgresql_fdw_validator, and additionally accepts FDW specific
options such as fetch_count. So, if dblink can ignore unknown FDW
options, pgsql_fdw_validator can be used to create foreign servers for
dblink connection.

How about removing postgresql_fdw_validator from backend binary, and
changing dblink to use contrib/postgresql_fdw's validator? It breaks
some backward compatibility and requires contrib/postgresql_fdw to be
installed before using contrib/dblink with foreign servers, but ISTM
that it doesn't become so serious.

Of course dblink is still available by itself if user specifies
connection information with "key = value" string, not with server name.

One concern is how to avoid duplicated list of valid libpq options.
Adding new libpq function, like below, which returns 1 when given name
is a valid libpq option would help.

int PQisValidOption(const char *keyword);

--
Shigeru Hanada

2012/2/29 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> (2012/02/29 4:07), Peter Eisentraut wrote:
>> Let's at least be clear about the reasons here.  The fact that
>> postgresql_fdw_validator exists means (a) there is a possible naming
>> conflict that has not been discussed yet, and/or (b) the name is already
>> settled and we need to think of a way to make postgresql_fdw_validator
>> work with the new actual FDW.
>
> We can avoid conflict of name by using postgres_fdw or pgsql_fdw, but it
> doesn't solve fundamental issue.  ISTM that maintaining two similar
> validators is wasteful and confusing, and FDW for PostgreSQL should be
> just one, at least in the context of core distribution.
>
> Current pgsql_fdw_validator accepts every FDW options which is accepted
> by postgresql_fdw_validator, and additionally accepts FDW specific
> options such as fetch_count.  So, if dblink can ignore unknown FDW
> options, pgsql_fdw_validator can be used to create foreign servers for
> dblink connection.
>
> How about removing postgresql_fdw_validator from backend binary, and
> changing dblink to use contrib/postgresql_fdw's validator?  It breaks
> some backward compatibility and requires contrib/postgresql_fdw to be
> installed before using contrib/dblink with foreign servers, but ISTM
> that it doesn't become so serious.
>

+1

pavel stehule

> Of course dblink is still available by itself if user specifies
> connection information with "key = value" string, not with server name.
>
> One concern is how to avoid duplicated list of valid libpq options.
> Adding new libpq function, like below, which returns 1 when given name
> is a valid libpq option would help.
>
>    int PQisValidOption(const char *keyword);
>
> --
> 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

Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com> writes:
> How about removing postgresql_fdw_validator from backend binary, and
> changing dblink to use contrib/postgresql_fdw's validator? It breaks
> some backward compatibility and requires contrib/postgresql_fdw to be
> installed before using contrib/dblink with foreign servers, but ISTM
> that it doesn't become so serious.

I don't think that creating such a dependency is acceptable.
Even if we didn't mind the dependency, you said yourself that
contrib/postgresql_fdw's validator will accept stuff that's not
appropriate for dblink.

If we don't think postgresql_fdw_validator belongs in core after all,
we should just move it to dblink.

regards, tom lane

(2012/03/01 0:33), Tom Lane wrote:
> I don't think that creating such a dependency is acceptable.
> Even if we didn't mind the dependency, you said yourself that
> contrib/postgresql_fdw's validator will accept stuff that's not
> appropriate for dblink.

Agreed. I think that these two contrib modules (and all FDW modules)
should have individual validator for each to avoid undesirable
dependency and naming conflict, and such validator function should be
inside each module, but not in core.

How about moving postgresql_fdw_validator into dblink, with renaming to
dblink_fdw_validator? Attached patch achieves such changes. I've left
postgresql_fdw_validator" in foreign_data regression test section, so
that foreign_data section can still check whether FDW DDLs invoke
validator function. I used the name "postgresql_fdw_validator" for test
validator to make change as little as possible.

This change requires dblink to have new function, so its version should
be bumped to 1.1.

These changes have no direct relation to PostgreSQL FDW, so this patch
can be applied by itself. If this patch has been applied, I'll rename
pgsql_fdw to postgresql_fdw which contains product name fully spelled out.

--
Shigeru Hanada

On tor, 2012-03-01 at 20:56 +0900, Shigeru Hanada wrote:
> How about moving postgresql_fdw_validator into dblink,

That's probably a good move. If this were C++, we might try to subclass
this whole thing a bit, to avoid code duplication, but I don't see an
easy way to do that here.

> with renaming to dblink_fdw_validator?

Well, it's not the validator of the dblink_fdw, so maybe something like
basic_postgresql_fdw_validator.

2012/3/2 Peter Eisentraut <peter_e(at)gmx(dot)net>:
>> with renaming to dblink_fdw_validator?
>
> Well, it's not the validator of the dblink_fdw, so maybe something like
> basic_postgresql_fdw_validator.

-1 for same reason. It's not the validator of basic_postgresql_fdw.

Using "fdw" in the name of validator which doesn't have actual FDW might
confuse users. Rather dblink_validator or libpq_option_validator is better?

One possible another idea is creating dblink_fdw which uses the
validator during "CREATE EXTENSION dblink" for users who store
connection information in FDW objects.

--
Shigeru Hanada

(2012/02/21 20:25), Etsuro Fujita wrote:
> Please find attached an updated version of the patch.

This v2 patch can be applied on HEAD cleanly. Compile completed with
only one expected warning of scan.c, and all regression tests for both
core and contrib modules passed.

This patch allows FDWs to return multiple ForeignPath nodes per a
PlanForeignScan call. It also get rid of FdwPlan, FDW-private
information container, by replacing with simple List.

I've reviewed the patch closely, and have some comments about its design.

Basically a create_foo_path is responsible for creating a node object
with a particular Path-derived type, but this patch changes
create_foreignscan_path to just call PlanForeignScan and return void.
This change seems breaking module design. IMO create_foreignscan_path
should return just one ForeignPath node per a call, so calling add_path
multiple times should be done in somewhere else. I think
set_foreign_pathlist suites for it, because set_foo_pathlist functions
are responsible for building possible paths for a RangeTblEntry, as
comment of set_foreign_pathlist says.

/*
* set_foreign_pathlist
* Build one or more access paths for a foreign table RTE
*/

In this design, FDW authors can implement PlanForeignScan by repeating
steps below for each possible scan path for a foreign table:

(1) create a template ForeignPath node with create_foreignscan_path
(2) customize the path as FDW wants, e.g. push down WHERE clause
(3) store FDW-private info
(4) estimate costs of the path
(5) call add_path to add the path to RelOptInfo

Current design doesn't allow FDWs to provide multiple paths which have
different local filtering from each other, because all paths share a
RelOptInfo and baserestrictinfo in it. I think this restriction
wouldn't be a serious problem.

Please find attached a patch implementing the design above.

--
Shigeru Hanada

(2012/03/05 18:21), Shigeru Hanada wrote:
> (2012/02/21 20:25), Etsuro Fujita wrote:
>> Please find attached an updated version of the patch.
>
> This v2 patch can be applied on HEAD cleanly. Compile completed with
> only one expected warning of scan.c, and all regression tests for both
> core and contrib modules passed.
>
> This patch allows FDWs to return multiple ForeignPath nodes per a
> PlanForeignScan call. It also get rid of FdwPlan, FDW-private
> information container, by replacing with simple List.
>
> I've reviewed the patch closely, and have some comments about its design.

Thank you for your review.

> Basically a create_foo_path is responsible for creating a node object
> with a particular Path-derived type, but this patch changes
> create_foreignscan_path to just call PlanForeignScan and return void.
> This change seems breaking module design.

create_index_path builds multiple index paths for a plain relation. How
about renaming the function to create_foreign_paths?

Best regards,
Etsuro Fujita

(2012/03/05 21:00), Etsuro Fujita wrote:
> (2012/03/05 18:21), Shigeru Hanada wrote:
>> (2012/02/21 20:25), Etsuro Fujita wrote:
>>> Please find attached an updated version of the patch.
>>
>> This v2 patch can be applied on HEAD cleanly. Compile completed with
>> only one expected warning of scan.c, and all regression tests for both
>> core and contrib modules passed.
>>
>> This patch allows FDWs to return multiple ForeignPath nodes per a
>> PlanForeignScan call. It also get rid of FdwPlan, FDW-private
>> information container, by replacing with simple List.
>>
>> I've reviewed the patch closely, and have some comments about its design.
>
> Thank you for your review.
>
>> Basically a create_foo_path is responsible for creating a node object
>> with a particular Path-derived type, but this patch changes
>> create_foreignscan_path to just call PlanForeignScan and return void.
>> This change seems breaking module design.
>
> create_index_path builds multiple index paths for a plain relation. How
> about renaming the function to create_foreign_paths?

I meant "create_foreignscan_paths". I'm sorry about that.

Best regards,
Etsuro Fujita

(2012/03/05 21:05), Etsuro Fujita wrote:
> (2012/03/05 21:00), Etsuro Fujita wrote:
>> create_index_path builds multiple index paths for a plain relation. How
>> about renaming the function to create_foreign_paths?
>
> I meant "create_foreignscan_paths". I'm sorry about that.

Perhaps you are confusing create_index_path with create_index_paths.
Former creates a IndexScan path node (so it's similar to
create_foreignscan_path), and latter builds multiple IndexScan paths for
a plain relation.

So, just renaming create_foreignscan_path to plural form seems missing
the point.

--
Shigeru Hanada

Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com> writes:
> So, just renaming create_foreignscan_path to plural form seems missing
> the point.

I agree that that wouldn't be an improvement. What bothers me about the
patch's version of this function is that it just creates a content-free
Path node and leaves it to the caller to fill in everything. That
doesn't accomplish much, and it leaves the caller very exposed to errors
of omission. It's also unlike the other create_xxx_path functions,
which generally hand back a completed Path ready to pass to add_path.

I'm inclined to think that if we provide this function in core at all,
it should take a parameter list long enough to let it fill in the Path
completely. That would imply that any future changes in Path structs
would result in a change in the parameter list, which would break
callers --- but it would break them in an obvious way that the C
compiler would complain about. If we leave it as-is, those same callers
would be broken silently, because they'd just be failing to fill in
the new Path fields.

regards, tom lane

I wrote:
> I'm inclined to think that if we provide this function in core at all,
> it should take a parameter list long enough to let it fill in the Path
> completely. That would imply that any future changes in Path structs
> would result in a change in the parameter list, which would break
> callers --- but it would break them in an obvious way that the C
> compiler would complain about. If we leave it as-is, those same callers
> would be broken silently, because they'd just be failing to fill in
> the new Path fields.

I've committed the PlanForeignScan API change, with that change and
some other minor editorialization. The pgsql_fdw patch now needs an
update, so I set it back to Waiting On Author state.

regards, tom lane

Shigeru Hanada wrote:
[pgsql_fdw_v12.patch]

I know this is not the latest version, but I played around with it and
tickled a bug.
It seems to have a problem with rolled back subtransactions.

test=> \d+ remote
Foreign table "laurenz.remote"
Column | Type | Modifiers | FDW Options | Storage | Description
--------+---------+-----------+-------------+----------+-------------
id | integer | not null | | plain |
val | text | not null | | extended |
Server: loopback
FDW Options: (nspname 'laurenz', relname 'local')
Has OIDs: no
test=> BEGIN;
test=> DECLARE x CURSOR FOR SELECT * FROM remote;
DEBUG: Remote SQL: SELECT id, val FROM laurenz.local
DEBUG: relid=16423 fetch_count=10000
DEBUG: starting remote transaction with "START TRANSACTION ISOLATION
LEVEL REPEATABLE READ"
test=> FETCH x;
id | val
----+-----
1 | one
(1 row)
test=> SAVEPOINT z;
test=> ERROR OUT;
ERROR: syntax error at or near "ERROR"
LINE 1: ERROR OUT;
test=> ROLLBACK TO SAVEPOINT z;
test=> FETCH x;
id | val
----+-----
2 | two
(1 row)
test=> COMMIT;
ERROR: could not close cursor
DETAIL: no connection to the server

HINT: CLOSE pgsql_fdw_cursor_0

The error message reported is not consistent, at one attempt the backend
crashed.

Yours,
Laurenz Albe

(2012/03/06 6:19), Tom Lane wrote:
> I've committed the PlanForeignScan API change, with that change and
> some other minor editorialization. The pgsql_fdw patch now needs an
> update, so I set it back to Waiting On Author state.

Thanks.

I've revised pgsql_fdw to catch up to this change, but I'll post those
patches after fixing the bug reported by Albe Laurenz.

BTW, what I did for this change is also needed for other existing FDWs
to make them available on 9.2. So, I'd like to add how to change FDWs
for 9.2 to SQL/MED wiki page, where probably most of the FDW authors check.

Regards,
--
Shigeru Hanada

(2012/03/06 19:09), Albe Laurenz wrote:
> I know this is not the latest version, but I played around with it and
> tickled a bug.
> It seems to have a problem with rolled back subtransactions.

Thanks for the report!

The problem was in cleanup_connection, which is called at end of
transactions. Connection should be closed only when the trigger is a
top level transaction and it's aborting, but isTopLevel flag was not
checked. I fixed the bug and added regression tests for such cases.

Attached patches also contains changes to catch up to the redesign of
PlanForeignScan.

Regards,
--
Shigeru Hanada

Shigeru Hanada wrote:
> Connection should be closed only when the trigger is a
> top level transaction and it's aborting, but isTopLevel flag was not
> checked. I fixed the bug and added regression tests for such cases.

I wondered about that - is it really necessary to close the remote
connection? Wouldn't a ROLLBACK on the remote connection be good enough?

Yours,
Laurenz Albe

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On tor, 2012-03-01 at 20:56 +0900, Shigeru Hanada wrote:
>> How about moving postgresql_fdw_validator into dblink,

> That's probably a good move. If this were C++, we might try to subclass
> this whole thing a bit, to avoid code duplication, but I don't see an
> easy way to do that here.

>> with renaming to dblink_fdw_validator?

> Well, it's not the validator of the dblink_fdw, so maybe something like
> basic_postgresql_fdw_validator.

I don't understand this objection. If we move it into dblink, then it
*is* dblink's validator, and nobody else's.

A bigger issue with postgresql_fdw_validator is that it supposes that
the core backend is authoritative as to what options libpq supports,
which is bad design on its face. It would be much more sensible for
dblink to be asking libpq what options libpq supports, say via
PQconndefaults().

We might find that we have to leave postgresql_fdw_validator as-is
for backwards compatibility reasons (in particular, being able to load
existing FDW definitions) but I think we should migrate away from using
it.

regards, tom lane

Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com> writes:
> Attached patches also contains changes to catch up to the redesign of
> PlanForeignScan.

I started to look at this patch, which soon led me back to the
prerequisite patch fdw_helper_v5.patch (that is the last version you
posted of that one, right?). I can see the value of
GetForeignColumnOptions, but ISTM that GetFdwOptionValue is poorly
designed and will accomplish little except to encourage inefficient
searching. The original version was even worse, but even in the current
version there is no way to avoid a useless scan of table-level options
when you are looking for a column-level option. Also, it's inefficient
when looking for several option values, as in this extract from
pgsql_fdw_v13.patch,

+ nspname = GetFdwOptionValue(InvalidOid, InvalidOid, relid,
+ InvalidAttrNumber, "nspname");
+ if (nspname == NULL)
+ nspname = get_namespace_name(get_rel_namespace(relid));
+ q_nspname = quote_identifier(nspname);
+
+ relname = GetFdwOptionValue(InvalidOid, InvalidOid, relid,
+ InvalidAttrNumber, "relname");
+ if (relname == NULL)
+ relname = get_rel_name(relid);
+ q_relname = quote_identifier(relname);

where we are going to uselessly run GetForeignTable twice.

If we had a lot of options that could usefully be specified at multiple
levels of the foreign-objects hierarchy, it might be appropriate to have
a search function defined like this; but the existing samples of FDWs
don't seem to support the idea that that's going to be common. It looks
to me like the vast majority of options make sense at exactly one level.

So I'm thinking we should forget GetFdwOptionValue and just expect the
callers to search the option lists for the appropriate object(s). It
might be worth providing get_options_value() as an exported function,
though surely there's not that much to it.

Another issue that get_options_value ignores defGetString() which is
what it really ought to be using, instead of assuming strVal() is
appropriate. ISTM that it would also encourage people to take shortcuts
where they should be using functions like defGetBoolean() etc. Not
quite sure what we should do about that; maybe we need to provide
several variants of the function that are appropriate for different
option datatypes.

regards, tom lane

(2012/03/07 9:01), Tom Lane wrote:
> I started to look at this patch, which soon led me back to the
> prerequisite patch fdw_helper_v5.patch (that is the last version you
> posted of that one, right?).

Thanks for the review. Yes, v5 is the last version of fdw_helper patch.

> I can see the value of
> GetForeignColumnOptions, but ISTM that GetFdwOptionValue is poorly
> designed and will accomplish little except to encourage inefficient
> searching. The original version was even worse, but even in the current
> version there is no way to avoid a useless scan of table-level options
> when you are looking for a column-level option. Also, it's inefficient
> when looking for several option values, as in this extract from
> pgsql_fdw_v13.patch,
>
> + nspname = GetFdwOptionValue(InvalidOid, InvalidOid, relid,
> + InvalidAttrNumber, "nspname");
> + if (nspname == NULL)
> + nspname = get_namespace_name(get_rel_namespace(relid));
> + q_nspname = quote_identifier(nspname);
> +
> + relname = GetFdwOptionValue(InvalidOid, InvalidOid, relid,
> + InvalidAttrNumber, "relname");
> + if (relname == NULL)
> + relname = get_rel_name(relid);
> + q_relname = quote_identifier(relname);
>
> where we are going to uselessly run GetForeignTable twice.

In addition, request for fetch_count option value via
GetFdwOptionValue() uselessly runs GetUserMapping() and
GetForeignDataWrapper() too, they are obvious waste of clocks and memory.

> If we had a lot of options that could usefully be specified at multiple
> levels of the foreign-objects hierarchy, it might be appropriate to have
> a search function defined like this; but the existing samples of FDWs
> don't seem to support the idea that that's going to be common. It looks
> to me like the vast majority of options make sense at exactly one level.

Yes, I added GetFdwOptionValue() to provide an easy way to obtain the
value of a particular FDW option which might be stored in multiple
levels of foreign-objects hierarchy without looping per object. I used
it in pgsql_fdw to get value of fetch_count option, which can be stored
in server and/or foreign table, but it seems the only one use case now.

> So I'm thinking we should forget GetFdwOptionValue and just expect the
> callers to search the option lists for the appropriate object(s). It
> might be worth providing get_options_value() as an exported function,
> though surely there's not that much to it.

Agreed. Attached fdw_helper patch doesn't contain GetFdwOptionValue()
any more, and pgsql_fdw patch accesses only necessary catalogs.

> Another issue that get_options_value ignores defGetString() which is
> what it really ought to be using, instead of assuming strVal() is
> appropriate. ISTM that it would also encourage people to take shortcuts
> where they should be using functions like defGetBoolean() etc. Not
> quite sure what we should do about that; maybe we need to provide
> several variants of the function that are appropriate for different
> option datatypes.

strVal() used in pgsql_fdw were replaced with defGetString(). It seems
to me that it's enough.

Regards,
--
Shigeru Hanada

On tis, 2012-03-06 at 13:39 -0500, Tom Lane wrote:
> A bigger issue with postgresql_fdw_validator is that it supposes that
> the core backend is authoritative as to what options libpq supports,
> which is bad design on its face. It would be much more sensible for
> dblink to be asking libpq what options libpq supports, say via
> PQconndefaults().

The validator for the proposed FDW suffers from the same problem.

Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com> writes:
> Agreed. Attached fdw_helper patch doesn't contain GetFdwOptionValue()
> any more, and pgsql_fdw patch accesses only necessary catalogs.

I've committed the fdw_helper part of this, with some very minor
improvements.

regards, tom lane

(2012/03/07 21:47), Shigeru Hanada wrote:
> Agreed. Attached fdw_helper patch doesn't contain GetFdwOptionValue()
> any more, and pgsql_fdw patch accesses only necessary catalogs.

Oops, I've missed some bugs. Attached patch fixes them.

1) foreign table's fetch_count options is always ignored
2) If given connection is not managed by connection cache,
ReleaseConnection() crashes backend process by NULL dereference.

Regards,
--
Shigeru Hanada

Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com> writes:
> [ pgsql_fdw_v15.patch ]

I've been looking at this patch a little bit over the past day or so.
I'm pretty unhappy with deparse.c --- it seems like a real kluge,
inefficient and full of corner-case bugs. After some thought I believe
that you're ultimately going to have to abandon depending on ruleutils.c
for reverse-listing services, and it would be best to bite that bullet
now and rewrite this code from scratch. ruleutils.c is serving two
masters already (rule-dumping and EXPLAIN) and it's not going to be
practical to tweak its behavior further for this usage; yet there are
all sorts of clear problems that you are going to run into, boiling down
to the fact that names on the remote end aren't necessarily the same as
names on the local end. For instance, ruleutils.c is not going to be
helpful at schema-qualifying function names in a way that's correct for
the foreign server environment. Another issue is that as soon as you
try to push down join clauses for parameterized paths, you are going to
want Vars of other relations to be printed as parameters ($n, most
likely) and ruleutils is not going to know to do that. Seeing that
semantic constraints will greatly limit the set of node types that can
ever be pushed down anyway, I think it's likely to be easiest to just
write your own node-printing code and not even try to use ruleutils.c.

There are a couple of other points that make me think we need to revisit
the PlanForeignScan API definition some more, too. First, deparse.c is
far from cheap. While this doesn't matter greatly as long as there's
only one possible path for a foreign table, as soon as you want to
create more than one it's going to be annoying to do all that work N
times and then throw away N-1 of the results. I also choked on the fact
that the pushdown patch thinks it can modify baserel->baserestrictinfo.
That might accidentally fail to malfunction right now, but it's never
going to scale to multiple paths with potentially different sets of
remotely-applied constraints. So I'm thinking we really need to let
FDWs in on the Path versus Plan distinction --- that is, a Path just
needs to be a cheap summary of a way to do things, and then at
createplan.c time you convert the selected Path into a full-fledged
Plan. Most of the work done in deparse.c could be postponed to
createplan time and done only once, even with multiple paths.
The baserestrictinfo hack would be unnecessary too if the FDW had more
direct control over generation of the ForeignScan plan node.

Another thing I'm thinking we should let FDWs in on is the distinction
between rowcount estimation and path generation. When we did the first
API design last year it was okay to expect a single call to do both,
but as of a couple months ago allpaths.c does those steps in two
separate passes over the baserels, and it'd be handy if FDWs would
cooperate.