subject:"Custom\/Foreign\-Join\-APIs \(Re\: \[HACKERS\] \[v9.5\] Custom Plan API\)"

Kohei KaiGai kai...@kaigai.gr.jp writes:
 I briefly checked your updates.
 Even though it is not described in the commit-log, I noticed a
 problematic change.

 This commit reverts create_plan_recurse() as static function.

Yes.  I am not convinced that external callers should be calling that,
and would prefer not to enlarge createplan.c's API footprint without a
demonstration that this is right and useful.  (This is one of many
ways in which this patch is suffering from having gotten committed
without submitted use-cases.)

 It means extension
 cannot have child node, even if it wants to add a custom-join logic.
 Please assume a simple case below:
   SELECT * FROM t0, t1 WHERE t0.a = t1.x;

 An extension adds a custom join path, then its PlanCustomPath method will be
 called back to create a plan node once it gets chosen by planner.
 The role of PlanCustomPath is to construct a plan-node of itself, and 
 plan-nodes
 of the source relations also.
 If create_plan_recurse() is static, we have no way to initialize
 plan-node for t0
 and t1 scan even if join-logic itself is powerful than built-in ones.

I find this argument quite unconvincing, because even granting that this
is an appropriate way to create child nodes of a CustomScan, there is a
lot of core code besides createplan.c that would not know about those
child nodes either.

As a counterexample, suppose that your cool-new-join-method is capable of
joining three inputs at once.  You could stick two of the children into
lefttree and righttree perhaps, but where are you gonna put the other?

This comes back to the fact that trying to wedge join behavior into scan
node types was a pretty bad idea (as evidenced by the entirely bogus
decision that now scanrelid can be zero, which I rather doubt you've found
all the places that that broke).  Really there should have been a new
CustomJoin node or something like that.  If we'd done that, it would be
possible to design that node type more like Append, with any number of
child nodes.  And we could have set things up so that createplan.c knows
about those child nodes and takes care of the recursion for you; it would
still not be a good idea to expose create_plan_recurse and hope that
callers of that would know how to use it correctly.

I am still more than half tempted to say we should revert this entire
patch series and hope for a better design to be submitted for 9.6.
In the meantime, though, poking random holes in the modularity of core
code is a poor substitute for having designed a well-thought-out API.

A possible compromise that we could perhaps still wedge into 9.5 is to
extend CustomPath with a List of child Paths, and CustomScan with a List
of child Plans, which createplan.c would know to build from the Paths,
and other modules would then also be aware of these children.  I find that
uglier than a separate join node type, but it would be tolerable I guess.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-05-10 Thread Andres Freund

On 2015-05-10 22:51:33 -0400, Robert Haas wrote:
  And there's definitely some things
  around that pretty much only still exist because changing them would
  break too much stuff.
 
 Such as what?

Without even thinking about it:
* linitial vs lfirst vs lnext. That thing still induces an impedance
  mismatch when reading code for me, and I believe a good number of
  other people.
* Two 'string buffer' APIs with essentially only minor differences.
* A whole bunch of libpq APIs. Admittedly that's a bit more exposed than
  lots of backend only things.
* The whole V0 calling convention that makes it so much easier to get
  odd crashes.

Admittedly that's all I could come up without having to think. But I do
vaguely remember a lot of things we did not do because of bwcompat
concerns.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-05-10 Thread Kouhei Kaigai

 On 2015-05-10 21:26:26 -0400, Robert Haas wrote:
  On Sun, May 10, 2015 at 8:41 PM, Tom Lane t...@sss.pgh.pa.us wrote:
This commit reverts create_plan_recurse() as static function.
   Yes.  I am not convinced that external callers should be calling that,
   and would prefer not to enlarge createplan.c's API footprint without a
   demonstration that this is right and useful.  (This is one of many
   ways in which this patch is suffering from having gotten committed
   without submitted use-cases.)
 
 Wasn't there a submitted use case? IIRC Kaigai had referenced some
 pg-strom (?) code using it?
 
 I'm failing to see how create_plan_recurse() being exposed externally is
 related to having gotten committed without submitted use-cases.  Even
 if submitted, presumably as simple as possible code, doesn't use it,
 that's not a proof that less simple code does not need it.

Yes, PG-Strom code uses create_plan_recurse() to construct child plan
node of the GPU accelerated custom-join logic, once it got chosen.
Here is nothing special. It calls create_plan_recurse() as built-in
join path doing on the underlying inner/outer paths. 
It is not difficult to submit as a working example, however, its total
code size (excludes GPU code) is 25KL at this moment.

I'm not certain whether it is a simple example.

  Your unwillingness to make functions global or to stick PGDLLIMPORT
  markings on variables that people want access to is hugely
  handicapping extension authors.  Many people have complained about
  that on multiple occasions.  Frankly, I find it obstructionist and
  petty.
 
 While I don't find the tone of the characterization super helpful, I do
 tend to agree that we're *far* too conservative on that end.  I've now
 seen a significant number of extension that copied large swathes of code
 just to cope with individual functions not being available.  And even
 cases where that lead to minor forks with such details changed.

I may have to join the members?

 I know that I'm fearful of asking for functions being made
 public. Because it'll invariably get into a discussion of merits that's
 completely out of proportion with the size of the change.  And if I, who
 has been on the list for a while now, am afraid in that way, you can
 be sure that others won't even dare to ask, lest argue their way
 through.
 
 I think the problem is that during development the default often is to
 create function as static if they're used only in one file. Which is
 fine. But it really doesn't work if it's a larger battle to change
 single incidences.  Besides the pain of having to wait for the next
 major release...
 
Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei kai...@ak.jp.nec.com



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-05-10 Thread Kohei KaiGai

Tom,

I briefly checked your updates.
Even though it is not described in the commit-log, I noticed a
problematic change.

This commit reverts create_plan_recurse() as static function. It means extension
cannot have child node, even if it wants to add a custom-join logic.
Please assume a simple case below:
  SELECT * FROM t0, t1 WHERE t0.a = t1.x;

An extension adds a custom join path, then its PlanCustomPath method will be
called back to create a plan node once it gets chosen by planner.
The role of PlanCustomPath is to construct a plan-node of itself, and plan-nodes
of the source relations also.
If create_plan_recurse() is static, we have no way to initialize
plan-node for t0
and t1 scan even if join-logic itself is powerful than built-in ones.

It was already discussed in the upthread, and people's consensus.
Please revert create_plan_recurse() as like initial commit.

Also, regarding of the *_scan_tlist,

 I don't have time to pursue this idea right now, but I think it would be
 a good change to squeeze into 9.5, just so that we could have some test
 coverage on those parts of this patch.

Do you want just a test purpose module and regression test cases?

Thanks,
-- 
KaiGai Kohei kai...@kaigai.gr.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

On Sun, May 10, 2015 at 9:34 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 Your unwillingness to make functions global or to stick PGDLLIMPORT
 markings on variables that people want access to is hugely
 handicapping extension authors.  Many people have complained about
 that on multiple occasions.  Frankly, I find it obstructionist and
 petty.

 Sure, we could export every last static function in the core code,
 and extension authors would rejoice ... while development on the core
 code basically stops for fear of breaking extensions.  It's important
 not to export things that we don't have to, especially when doing so
 is really just a quick-n-dirty substitute for doing things properly.

Please name EVEN ONE instance in which core development has been
prevented for fear of changing a function API.  Sure, we take those
things into consideration, like trying to ensure that there will be
type conflicts until people update their code, but I cannot recall a
single instance in six and a half years of working on this project
where that's been a real problem.  I think this concern is entirely
hypothetical.  Besides, no one has ever proposed making every static
function public.  It's been proposed a handful of times for limited
classes of functions - in this case ONE - and you've fought it every
time despite clear consensus on the other side.  I find that highly
regrettable and I'm very sure I'm not the only one.

I notice that you carefully didn't answer the other part of my
question: what gives you the right to revert my commits without
discussion or consensus, and do I have an equal right to change it
back?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

On Sun, May 10, 2015 at 8:41 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Kohei KaiGai kai...@kaigai.gr.jp writes:
 I briefly checked your updates.
 Even though it is not described in the commit-log, I noticed a
 problematic change.

 This commit reverts create_plan_recurse() as static function.

 Yes.  I am not convinced that external callers should be calling that,
 and would prefer not to enlarge createplan.c's API footprint without a
 demonstration that this is right and useful.  (This is one of many
 ways in which this patch is suffering from having gotten committed
 without submitted use-cases.)

I really think that reverting somebody else's committed change without
discussion is inappropriate.  If I don't like the fact that you
reverted this change, can I go revert it back?

Your unwillingness to make functions global or to stick PGDLLIMPORT
markings on variables that people want access to is hugely
handicapping extension authors.  Many people have complained about
that on multiple occasions.  Frankly, I find it obstructionist and
petty.

If you want to improve the design of this so that it does the same
things more elegantly, fine: I'll get out of the way.  If you just
want to make things impossible that the patch previously made
possible, I strongly object to that.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-05-10 Thread Andres Freund

On 2015-05-10 21:53:45 -0400, Robert Haas wrote:
 Please name EVEN ONE instance in which core development has been
 prevented for fear of changing a function API.

Even *moving* function declarations to a different file has been laudly
and repeatedly complained about... And there's definitely some things
around that pretty much only still exist because changing them would
break too much stuff.

But.

I don't think that's a reason to not expose more functions
externally. Because the usual consequence of not exposing them is that
either ugly workarounds will be found, or code will just copy pasted
around. That's not in any way better, and much likely to be worse.

I'm not saying that we shouldn't use judgement, but I do think that the
current approach ridicules our vaunted extensibility in many cases.

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

On Sun, May 10, 2015 at 10:37 PM, Andres Freund and...@anarazel.de wrote:
 On 2015-05-10 21:53:45 -0400, Robert Haas wrote:
 Please name EVEN ONE instance in which core development has been
 prevented for fear of changing a function API.

 Even *moving* function declarations to a different file has been laudly
 and repeatedly complained about...

Moving declarations is a lot more likely to break compiles than adding
declarations.  But even the 9.3 header file reorganizations, which
broke enough compiles to be annoying, were only annoying, not a
serious problem for anyone.  I doubted whether that stuff was worth
changing, but that's just because I don't really get excited about
partial recompiles.

 And there's definitely some things
 around that pretty much only still exist because changing them would
 break too much stuff.

Such as what?

 But.

 I don't think that's a reason to not expose more functions
 externally. Because the usual consequence of not exposing them is that
 either ugly workarounds will be found, or code will just copy pasted
 around. That's not in any way better, and much likely to be worse.

Yes.

 I'm not saying that we shouldn't use judgement, but I do think that the
 current approach ridicules our vaunted extensibility in many cases.

Double yes.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-05-10 Thread Andres Freund

On 2015-05-10 21:26:26 -0400, Robert Haas wrote:
 On Sun, May 10, 2015 at 8:41 PM, Tom Lane t...@sss.pgh.pa.us wrote:
   This commit reverts create_plan_recurse() as static function.
  Yes.  I am not convinced that external callers should be calling that,
  and would prefer not to enlarge createplan.c's API footprint without a
  demonstration that this is right and useful.  (This is one of many
  ways in which this patch is suffering from having gotten committed
  without submitted use-cases.)

Wasn't there a submitted use case? IIRC Kaigai had referenced some
pg-strom (?) code using it?

I'm failing to see how create_plan_recurse() being exposed externally is
related to having gotten committed without submitted use-cases.  Even
if submitted, presumably as simple as possible code, doesn't use it,
that's not a proof that less simple code does not need it.

 Your unwillingness to make functions global or to stick PGDLLIMPORT
 markings on variables that people want access to is hugely
 handicapping extension authors.  Many people have complained about
 that on multiple occasions.  Frankly, I find it obstructionist and
 petty.

While I don't find the tone of the characterization super helpful, I do
tend to agree that we're *far* too conservative on that end.  I've now
seen a significant number of extension that copied large swathes of code
just to cope with individual functions not being available.  And even
cases where that lead to minor forks with such details changed.

I know that I'm fearful of asking for functions being made
public. Because it'll invariably get into a discussion of merits that's
completely out of proportion with the size of the change.  And if I, who
has been on the list for a while now, am afraid in that way, you can
be sure that others won't even dare to ask, lest argue their way
through.

I think the problem is that during development the default often is to
create function as static if they're used only in one file. Which is
fine. But it really doesn't work if it's a larger battle to change
single incidences.  Besides the pain of having to wait for the next
major release...

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

Robert Haas robertmh...@gmail.com writes:
 Your unwillingness to make functions global or to stick PGDLLIMPORT
 markings on variables that people want access to is hugely
 handicapping extension authors.  Many people have complained about
 that on multiple occasions.  Frankly, I find it obstructionist and
 petty.

Sure, we could export every last static function in the core code,
and extension authors would rejoice ... while development on the core
code basically stops for fear of breaking extensions.  It's important
not to export things that we don't have to, especially when doing so
is really just a quick-n-dirty substitute for doing things properly.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-05-10 Thread Kouhei Kaigai

 Kohei KaiGai kai...@kaigai.gr.jp writes:
  I briefly checked your updates.
  Even though it is not described in the commit-log, I noticed a
  problematic change.
 
  This commit reverts create_plan_recurse() as static function.
 
 Yes.  I am not convinced that external callers should be calling that,
 and would prefer not to enlarge createplan.c's API footprint without a
 demonstration that this is right and useful.  (This is one of many
 ways in which this patch is suffering from having gotten committed
 without submitted use-cases.)

Hmm. I got it is intentional change.

  It means extension
  cannot have child node, even if it wants to add a custom-join logic.
  Please assume a simple case below:
SELECT * FROM t0, t1 WHERE t0.a = t1.x;
 
  An extension adds a custom join path, then its PlanCustomPath method will be
  called back to create a plan node once it gets chosen by planner.
  The role of PlanCustomPath is to construct a plan-node of itself, and 
  plan-nodes
  of the source relations also.
  If create_plan_recurse() is static, we have no way to initialize
  plan-node for t0
  and t1 scan even if join-logic itself is powerful than built-in ones.
 
 I find this argument quite unconvincing, because even granting that this
 is an appropriate way to create child nodes of a CustomScan, there is a
 lot of core code besides createplan.c that would not know about those
 child nodes either.

 As a counterexample, suppose that your cool-new-join-method is capable of
 joining three inputs at once.  You could stick two of the children into
 lefttree and righttree perhaps, but where are you gonna put the other?

 This comes back to the fact that trying to wedge join behavior into scan
 node types was a pretty bad idea (as evidenced by the entirely bogus
 decision that now scanrelid can be zero, which I rather doubt you've found
 all the places that that broke).  Really there should have been a new
 CustomJoin node or something like that.  If we'd done that, it would be
 possible to design that node type more like Append, with any number of
 child nodes.  And we could have set things up so that createplan.c knows
 about those child nodes and takes care of the recursion for you; it would
 still not be a good idea to expose create_plan_recurse and hope that
 callers of that would know how to use it correctly.

 I am still more than half tempted to say we should revert this entire
 patch series and hope for a better design to be submitted for 9.6.
 In the meantime, though, poking random holes in the modularity of core
 code is a poor substitute for having designed a well-thought-out API.
 
 A possible compromise that we could perhaps still wedge into 9.5 is to
 extend CustomPath with a List of child Paths, and CustomScan with a List
 of child Plans, which createplan.c would know to build from the Paths,
 and other modules would then also be aware of these children.  I find that
 uglier than a separate join node type, but it would be tolerable I guess.

At this moment, my custom-join logic add a dummy node to have two child
nodes when it tries to join more than 3 relations.
Yep, if CustomPath node (ForeignPath also?) can have a list of child-path
nodes then core backend handles its initialization job, it will be more
comfortable for extensions.
I prefer this idea, rather than agree.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei kai...@ak.jp.nec.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

On Sat, May 9, 2015 at 1:05 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 I originally wanted to go quite the other way with this and check for
 join pushdown via handler X any time at least one of the two relations
 involved used handler X, even if the other one used some other handler
 or was a plain table.  In particular, it seems to me quite plausible
 to want to teach an FDW that a certain local table is replicated on a
 remote node, allowing a join between a foreign table and a plain table
 to be pushed down.

 If we did do something like that, I think a saner implementation would
 involve substituting a foreign table for the local one earlier, via view
 expansion.  So by the time we are doing join planning, there would be no
 need to consider cross-server joins anyway.

Huh?  You can't do this at rewrite time; it's very fundamentally a
planning problem.  Suppose the user wants to join A, B, and C, where A
is a plain table, B is a plain table that is replicated on a foreign
server, and C is a foreign table on that same foreign server.  If we
decide to join B to C first, we probably want to push down the join,
although maybe not, if we estimate that B JOIN C will have more rows
than C.  If we decide to join A to B first, we want to use the local
copy of B.

 This infrastructure can't be used that way anyhow,
 so maybe there's no harm in tightening it up, but I'm wary of
 circumscribing what FDW authors can do.

 Somebody who's really intent on shooting themselves in the foot can always
 use the set_join_pathlist_hook to inject paths for arbitrary joins.
 The FDW mechanism should support reasonable use cases without undue
 complication, and I doubt that what we've got now is adding anything
 except complication and risk of bugs.

 For the archives' sake, let me lay out a couple of reasons why an FDW
 that tried to allow cross-server joins would almost certainly be broken,
 and broken in security-relevant ways.  Suppose for instance that
 postgres_fdw tried to be smart and drill down into foreign tables' server
 IDs to allow joining of any two tables that have the same effective host
 name, port, database name, user name, and anything else you think would be
 relevant to its choice of connections.  The trouble with that is that the
 user mapping is context dependent, in particular one local userID might
 map to the same remote user name for two different server OIDs, while
 another might map to different user names.  So if we plan a query under
 the first userID we might decide it's okay to do the join remotely.
 Then, if we re-use that plan while executing as another userID (which is
 entirely possible) what will probably happen is that the remote join
 query will get sent off under one or the other of the remote usernames
 associated with the second local userID.  This could lead to either a
 permission failure, or a remote table access that should not be allowed
 to the current local userID.  Admittedly, such cases might be rare in
 practice, but it's still a security hole.  Also, even if the FDW is
 defensive enough to recheck full matching of the tables' connection
 properties at execution time, there's not much it could do about the
 situation except fail; it couldn't cause a re-plan to occur.

 For another case, we do not have any mechanism whereby operations like
 ALTER SERVER OPTIONS could invalidate existing plans.  Thus, even if
 the two tables' connection properties matched at plan time, there's no
 guarantee that they still match at execution.  This is probably not a
 security hole (at least not if you assume foreign-server owners are
 trusted users), but it's still a bug that exists only if someone tries
 to allow cross-server joins.

 For these reasons, I think that if an FDW tried to be laxer than tables
 must be on the same pg_foreign_server entry to be joined, the odds
 approach unity that it would be broken, and probably dangerously broken.
 So we should just make that check for the FDWs.  Anybody who thinks
 they're smarter than the average bear can use set_join_pathlist_hook,
 but they are probably wrong.

Drilling down into postgres_fdw's connection properties seems pretty
silly; the user isn't likely to create two SERVER objects that are
identical and then choose which one to use at random, and if they do,
they deserve what they get.  The universe of FDWs, however, is
potentially bigger than that.  What does a server even mean for
file_fdw, for example?  I can't think of any reason why somebody would
want to implement joins inside file_fdw, but if they did, all the
things being joined would be local files, so the server ID doesn't
really matter.  Now you may say that's a silly use case, but it's less
obviously silly if the files contain structured data, as with
cstore_fdw, yet the server ID could still be not especially relevant.
Maybe you've got servers representing filesystem directories; that
shouldn't preclude cross server joins.

-- 
Robert Haas
EnterpriseDB:

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

Robert Haas robertmh...@gmail.com writes:
 On Sat, May 9, 2015 at 1:05 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 For these reasons, I think that if an FDW tried to be laxer than tables
 must be on the same pg_foreign_server entry to be joined, the odds
 approach unity that it would be broken, and probably dangerously broken.
 So we should just make that check for the FDWs.  Anybody who thinks
 they're smarter than the average bear can use set_join_pathlist_hook,
 but they are probably wrong.

 Drilling down into postgres_fdw's connection properties seems pretty
 silly; the user isn't likely to create two SERVER objects that are
 identical and then choose which one to use at random, and if they do,
 they deserve what they get.  The universe of FDWs, however, is
 potentially bigger than that.  What does a server even mean for
 file_fdw, for example?

Nothing, which is why you'd only ever create one per database, and so the
issue doesn't arise anyway.  It would only be sane to create multiple
servers per FDW if there were a meaningful difference between them.

In any case, since the existing code doesn't support remote joins
involving a local table unless you use the join-path hook, this argument
seems pretty academic.  If we tighten the test to be same-server, we will
benefit all but very weirdly designed FDWs.  Anybody who's not happy with
that can still use the hook (and I continue to maintain that they will
probably have subtle bugs, but whatever).

Another point worth making is that the coding I have in mind doesn't
really do anything with RelOptInfo.serverid except compare it for
equality.  So an FDW that wants to consider some servers interchangeable
for joining purposes could override the value at GetForeignPaths time
(ie replace serverid with the OID of a preferred server), and then it
would get GetForeignJoinPaths calls as desired.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

I've committed a cleanup patch along the lines discussed.

One thought is that we could test the nondefault-scan-tuple logic without
a lot of work by modifying the way postgres_fdw deals with columns it
decides don't need to be fetched.  Right now it injects NULL columns so
that the remote query results still match the foreign table's rowtype,
but that's not especially desirable or efficient.  What we could do
instead is generate an fdw_scan_tlist that just lists the columns we
intend to fetch.

I don't have time to pursue this idea right now, but I think it would be
a good change to squeeze into 9.5, just so that we could have some test
coverage on those parts of this patch.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-05-09 Thread Tom Lane

Robert Haas robertmh...@gmail.com writes:
 On Fri, May 8, 2015 at 5:48 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 I think that we'd really be better off insisting on same server (as in
 same pg_foreign_server OID), hence automatically same FDW, and what's
 even more important, same user mapping for any possible query execution
 context.  The possibility that there are some corner cases where some FDWs
 could optimize other scenarios seems to me to be poor return for the bugs
 and security holes that will arise any time typical FDWs forget to check
 this.

 I originally wanted to go quite the other way with this and check for
 join pushdown via handler X any time at least one of the two relations
 involved used handler X, even if the other one used some other handler
 or was a plain table.  In particular, it seems to me quite plausible
 to want to teach an FDW that a certain local table is replicated on a
 remote node, allowing a join between a foreign table and a plain table
 to be pushed down.

If we did do something like that, I think a saner implementation would
involve substituting a foreign table for the local one earlier, via view
expansion.  So by the time we are doing join planning, there would be no
need to consider cross-server joins anyway.

 This infrastructure can't be used that way anyhow,
 so maybe there's no harm in tightening it up, but I'm wary of
 circumscribing what FDW authors can do.

Somebody who's really intent on shooting themselves in the foot can always
use the set_join_pathlist_hook to inject paths for arbitrary joins.
The FDW mechanism should support reasonable use cases without undue
complication, and I doubt that what we've got now is adding anything
except complication and risk of bugs.

For the archives' sake, let me lay out a couple of reasons why an FDW
that tried to allow cross-server joins would almost certainly be broken,
and broken in security-relevant ways.  Suppose for instance that
postgres_fdw tried to be smart and drill down into foreign tables' server
IDs to allow joining of any two tables that have the same effective host
name, port, database name, user name, and anything else you think would be
relevant to its choice of connections.  The trouble with that is that the
user mapping is context dependent, in particular one local userID might
map to the same remote user name for two different server OIDs, while
another might map to different user names.  So if we plan a query under
the first userID we might decide it's okay to do the join remotely.
Then, if we re-use that plan while executing as another userID (which is
entirely possible) what will probably happen is that the remote join
query will get sent off under one or the other of the remote usernames
associated with the second local userID.  This could lead to either a
permission failure, or a remote table access that should not be allowed
to the current local userID.  Admittedly, such cases might be rare in
practice, but it's still a security hole.  Also, even if the FDW is
defensive enough to recheck full matching of the tables' connection
properties at execution time, there's not much it could do about the
situation except fail; it couldn't cause a re-plan to occur.

For another case, we do not have any mechanism whereby operations like
ALTER SERVER OPTIONS could invalidate existing plans.  Thus, even if
the two tables' connection properties matched at plan time, there's no
guarantee that they still match at execution.  This is probably not a
security hole (at least not if you assume foreign-server owners are
trusted users), but it's still a bug that exists only if someone tries
to allow cross-server joins.

For these reasons, I think that if an FDW tried to be laxer than tables
must be on the same pg_foreign_server entry to be joined, the odds
approach unity that it would be broken, and probably dangerously broken.
So we should just make that check for the FDWs.  Anybody who thinks
they're smarter than the average bear can use set_join_pathlist_hook,
but they are probably wrong.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-05-09 Thread Kohei KaiGai

2015-05-09 11:21 GMT+09:00 Robert Haas robertmh...@gmail.com:
 On Fri, May 8, 2015 at 5:48 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 ... btw, I just noticed something that had escaped me because it seems so
 obviously wrong that I had not even stopped to consider the possibility
 that the code was doing what it's doing.  To wit, that the planner
 supposes that two foreign tables are potentially remote-joinable if they
 share the same underlying FDW handler function.  Not the same server, and
 not even the same pg_foreign_data_wrapper entry, but the pg_proc entry for
 the handler function.  I think this is fundamentally bogus.  Under what
 circumstances are we not just laying off the need to check same server
 origin onto the FDW?  How is it that the urgent need for the FDW to check
 for that isn't even mentioned in the documentation?

 I think that we'd really be better off insisting on same server (as in
 same pg_foreign_server OID), hence automatically same FDW, and what's
 even more important, same user mapping for any possible query execution
 context.  The possibility that there are some corner cases where some FDWs
 could optimize other scenarios seems to me to be poor return for the bugs
 and security holes that will arise any time typical FDWs forget to check
 this.

 I originally wanted to go quite the other way with this and check for
 join pushdown via handler X any time at least one of the two relations
 involved used handler X, even if the other one used some other handler
 or was a plain table.  In particular, it seems to me quite plausible
 to want to teach an FDW that a certain local table is replicated on a
 remote node, allowing a join between a foreign table and a plain table
 to be pushed down.  This infrastructure can't be used that way anyhow,
 so maybe there's no harm in tightening it up, but I'm wary of
 circumscribing what FDW authors can do.  I think it's better to be
 rather expansive in terms of when we call them and let them return
 without doing anything some of them time than to define the situations
 in which we call them too narrowly and end up ruling out interesting
 use cases.

Probably, it is relatively minor case to join a foreign table and a replicated
local relation on remote side. Even if the rough check by sameness of
foreign server-id does not invoke GetForeignJoinPaths, FDW driver can
implement its arbitrary logic using set_join_pathlist_hook by its own risk,
isn't it?

The attached patch changed the logic to check joinability of two foreign
relations. As upthread, it checks foreign server-id instead of handler
function.
build_join_rel() set fdw_server of RelOptInfo if inner and outer foreign-
relations have same value, then it eventually allows to kick
GetForeignJoinPaths on add_paths_to_joinrel().

Thanks,
-- 
KaiGai Kohei kai...@kaigai.gr.jp


custom-join-fdw-pushdown-check-by-server.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-05-09 Thread Kohei KaiGai

2015-05-09 8:18 GMT+09:00 Kohei KaiGai kai...@kaigai.gr.jp:
2015-05-09 2:46 GMT+09:00 Tom Lane t...@sss.pgh.pa.us:
Kouhei Kaigai kai...@ak.jp.nec.com writes:
I've been trying to code-review this patch, because the documentation
seemed several bricks shy of a load, and I find myself entirely confused
by the fdw_ps_tlist and custom_ps_tlist fields.

Main-point of your concern is lack of documentation/comments to introduce
how does the pseudo-scan targetlist works here, isn't it??

Well, there's a bunch of omissions and outright errors in the docs and
comments, but this is the main issue that I was uncertain how to fix
from looking at the patch.

Also,
if that is what they're for (ie, to allow the FDW to redefine the scan
tuple contents) it would likely be better to decouple that feature from
whether the plan node is for a simple scan or a join.

In this version, we don't intend FDW/CSP to redefine the contents of
scan tuples, even though I want off-loads heavy targetlist calculation
workloads to external computing resources in *the future version*.

I do not think it's a good idea to introduce such a field now and then
redefine how it works and what it's for in a future version. We should
not be moving the FDW APIs around more than we absolutely have to,
especially not in ways that wouldn't throw an obvious compile error
for un-updated code. Also, the longer we wait to make a change that
we know we want, the more pain we inflict on FDW authors (simply because
there will be more of them a year from now than there are today).

Ah, above my sentence don't intend to reuse the existing field for
different works in the future version. It's just what I want to support
in the future version.
Yep, I see. It is not a good idea to redefine the existing field for
different purpose silently. It's not my plan.

The business about
resjunk columns in that list also seems a bit half baked, or at least
underdocumented.

I'll add source code comments to introduce how does it works any when
does it have resjunk=true. It will be a bit too deep to be introduced
in the SGML file.

I don't actually see a reason for resjunk marking in that list at all,
if what it's for is to define the contents of the scan tuple. I think we
should just s/ExecCleanTypeFromTL/ExecTypeFromTL/ in nodeForeignscan and
nodeCustom, and get rid of the sanity check in create_foreignscan_plan
(which is pretty pointless anyway, considering the number of other ways
you could screw up that tlist without it being detected).

http://www.postgresql.org/message-id/9a28c8860f777e439aa12e8aea7694f8010d7...@bpxm15gp.gisp.nec.co.jp

Does the introduction in above post make sense?
The *_ps_tlist is not only used for a basic of scan-tuple descriptor, but
also used to solve var-node if varno==INDEX_VAR in EXPLAIN command.
On the other hands, existence of the junk entries (which are referenced in
external computing resources only) may cause unnecessary projection.
So, I want to discriminate target-entries for basis of scan-tuple descriptor
from other ones just for EXPLAIN command.

I'm also inclined to rename the fields to
fdw_scan_tlist/custom_scan_tlist, which would better reflect what they do,
and to change the API of make_foreignscan() to add a parameter
corresponding to the scan tlist. It's utterly bizarre and error-prone
that this patch has added a field that the FDW is supposed to set and
not changed make_foreignscan to match.

OK, I'll do the both of changes. The name of ps_tlist is a shorten of
pseudo-scan target-list. So, fdw_scan_tlist/custom_scan_tlist are
almost intentional.

The attached patch renamed *_ps_tlist by *_scan_tlist according to
the suggestion.
Also, put a few detailed source code comments around this alternative
scan_tlist.

Thanks,
--
KaiGai Kohei kai...@kaigai.gr.jp

custom-join-rename-ps_tlist.patch
Description: Binary data

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-05-09 Thread Kohei KaiGai

2015-05-09 8:32 GMT+09:00 Kohei KaiGai kai...@kaigai.gr.jp:
 2015-05-09 3:51 GMT+09:00 Tom Lane t...@sss.pgh.pa.us:
 Robert Haas robertmh...@gmail.com writes:
 On Fri, May 8, 2015 at 1:46 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 That's nice, but 9.5 feature freeze is only a week away.  I don't have a
 lot of confidence that this stuff is actually in a state where we won't
 regret shipping it in 9.5.

 Yeah.  The POC you were asking for upthread certainly exists and has
 for a while, or I would not have committed this.  But I do not think
 it likely that the  postgres_fdw support will be ready for 9.5.

 Well, we have two alternatives.  I can keep hacking on this and get it
 to a state where it seems credible to me, but we won't have any proof
 that it actually works (though perhaps we could treat any problems
 as bugs that should hopefully get found before 9.5 ships, if a
 postgres_fdw patch shows up in the next few months).  Or we could
 revert the whole thing and bounce it to the 9.6 cycle.  I don't really
 like doing the latter, but I'm pretty uncomfortable with committing to
 published FDW APIs that are (a) as messy as this and (b) practically
 untested.  The odds that something slipped through the cracks are high.

 Aside from the other gripes I raised, I'm exceedingly unhappy with the
 ad-hoc APIs proposed for GetForeignJoinPaths and set_join_pathlist_hook.
 It's okay for internal calls in joinpath.c to look like that, but
 exporting that set of parameters seems like pure folly.  We've changed
 those parameter lists repeatedly (for instance in 9.2 and again in 9.3);
 the odds that they'll need to change again in future approach 100%.

 One way we could reduce the risk of code breakage there is to stuff all
 or most of those parameters into a struct.  This might result in a small
 slowdown for the internal calls, or then again maybe not --- there
 probably aren't many architectures that can pass 10 parameters in
 registers anyway.

 Is it like a following structure definition?

   typedef struct
   {
 PlannerInfo *root;
 RelOptInfo *joinrel;
 RelOptInfo *outerrel;
 RelOptInfo *innerrel;
 List *restrictlist;
 JoinType jointype;
 SpecialJoinInfo *sjinfo;
 SemiAntiJoinFactors *semifactors;
 Relids param_source_rels;
 Relids extra_lateral_rels;
   } SetJoinPathListArgs;

 I agree the idea. It also helps CSP driver implementation where it calls
 next driver that was already chained on its installation.

   if (set_join_pathlist_next)
   set_join_pathlist_next(args);

 is more stable manner than

   if (set_join_pathlist_next)
   set_join_pathlist_next(root,
joinrel,
outerrel,
innerrel,
restrictlist,
jointype,
sjinfo,
semifactors,
param_source_rels,
extra_lateral_rels);

The attached patch newly defines ExtraJoinPathArgs struct to pack
all the necessary information to be delivered on GetForeignJoinPaths
and set_join_pathlist_hook.

Its definition is below:
  typedef struct
  {
 PlannerInfo*root;
 RelOptInfo *joinrel;
 RelOptInfo *outerrel;
 RelOptInfo *innerrel;
 List   *restrictlist;
 JoinTypejointype;
 SpecialJoinInfo *sjinfo;
 SemiAntiJoinFactors *semifactors;
 Relids  param_source_rels;
 Relids  extra_lateral_rels;
  } ExtraJoinPathArgs;

then, hook invocation gets much simplified, like:

/*
 * 6. Finally, give extensions a chance to manipulate the path list.
 */
if (set_join_pathlist_hook)
set_join_pathlist_hook(jargs);

Thanks,
-- 
KaiGai Kohei kai...@kaigai.gr.jp


custom-join-argument-by-struct.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-05-08 Thread Tom Lane

Kouhei Kaigai kai...@ak.jp.nec.com writes:
 I've been trying to code-review this patch, because the documentation
 seemed several bricks shy of a load, and I find myself entirely confused
 by the fdw_ps_tlist and custom_ps_tlist fields.

 Main-point of your concern is lack of documentation/comments to introduce
 how does the pseudo-scan targetlist works here, isn't it??

Well, there's a bunch of omissions and outright errors in the docs and
comments, but this is the main issue that I was uncertain how to fix
from looking at the patch.

 Also,
 if that is what they're for (ie, to allow the FDW to redefine the scan
 tuple contents) it would likely be better to decouple that feature from
 whether the plan node is for a simple scan or a join.

 In this version, we don't intend FDW/CSP to redefine the contents of
 scan tuples, even though I want off-loads heavy targetlist calculation
 workloads to external computing resources in *the future version*.

I do not think it's a good idea to introduce such a field now and then
redefine how it works and what it's for in a future version.  We should
not be moving the FDW APIs around more than we absolutely have to,
especially not in ways that wouldn't throw an obvious compile error
for un-updated code.  Also, the longer we wait to make a change that
we know we want, the more pain we inflict on FDW authors (simply because
there will be more of them a year from now than there are today).

 The business about
 resjunk columns in that list also seems a bit half baked, or at least
 underdocumented.

 I'll add source code comments to introduce how does it works any when
 does it have resjunk=true. It will be a bit too deep to be introduced
 in the SGML file.

I don't actually see a reason for resjunk marking in that list at all,
if what it's for is to define the contents of the scan tuple.  I think we
should just s/ExecCleanTypeFromTL/ExecTypeFromTL/ in nodeForeignscan and
nodeCustom, and get rid of the sanity check in create_foreignscan_plan
(which is pretty pointless anyway, considering the number of other ways
you could screw up that tlist without it being detected).

I'm also inclined to rename the fields to
fdw_scan_tlist/custom_scan_tlist, which would better reflect what they do,
and to change the API of make_foreignscan() to add a parameter
corresponding to the scan tlist.  It's utterly bizarre and error-prone
that this patch has added a field that the FDW is supposed to set and
not changed make_foreignscan to match.

 I do not think that this should have gotten committed without an attendant
 proof-of-concept patch to postgres_fdw, so that the logic could be tested.

 Hanada-san is now working according to the comments from Robert.

That's nice, but 9.5 feature freeze is only a week away.  I don't have a
lot of confidence that this stuff is actually in a state where we won't
regret shipping it in 9.5.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-05-08 Thread Robert Haas

On Fri, May 8, 2015 at 1:46 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 That's nice, but 9.5 feature freeze is only a week away.  I don't have a
 lot of confidence that this stuff is actually in a state where we won't
 regret shipping it in 9.5.

Yeah.  The POC you were asking for upthread certainly exists and has
for a while, or I would not have committed this.  But I do not think
it likely that the  postgres_fdw support will be ready for 9.5.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-05-08 Thread Tom Lane

Robert Haas robertmh...@gmail.com writes:
 On Fri, May 8, 2015 at 1:46 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 That's nice, but 9.5 feature freeze is only a week away.  I don't have a
 lot of confidence that this stuff is actually in a state where we won't
 regret shipping it in 9.5.

 Yeah.  The POC you were asking for upthread certainly exists and has
 for a while, or I would not have committed this.  But I do not think
 it likely that the  postgres_fdw support will be ready for 9.5.

Well, we have two alternatives.  I can keep hacking on this and get it
to a state where it seems credible to me, but we won't have any proof
that it actually works (though perhaps we could treat any problems
as bugs that should hopefully get found before 9.5 ships, if a
postgres_fdw patch shows up in the next few months).  Or we could
revert the whole thing and bounce it to the 9.6 cycle.  I don't really
like doing the latter, but I'm pretty uncomfortable with committing to
published FDW APIs that are (a) as messy as this and (b) practically
untested.  The odds that something slipped through the cracks are high.

Aside from the other gripes I raised, I'm exceedingly unhappy with the
ad-hoc APIs proposed for GetForeignJoinPaths and set_join_pathlist_hook.
It's okay for internal calls in joinpath.c to look like that, but
exporting that set of parameters seems like pure folly.  We've changed
those parameter lists repeatedly (for instance in 9.2 and again in 9.3);
the odds that they'll need to change again in future approach 100%.

One way we could reduce the risk of code breakage there is to stuff all
or most of those parameters into a struct.  This might result in a small
slowdown for the internal calls, or then again maybe not --- there
probably aren't many architectures that can pass 10 parameters in
registers anyway.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-05-08 Thread Tom Lane

... btw, I just noticed something that had escaped me because it seems so
obviously wrong that I had not even stopped to consider the possibility
that the code was doing what it's doing.  To wit, that the planner
supposes that two foreign tables are potentially remote-joinable if they
share the same underlying FDW handler function.  Not the same server, and
not even the same pg_foreign_data_wrapper entry, but the pg_proc entry for
the handler function.  I think this is fundamentally bogus.  Under what
circumstances are we not just laying off the need to check same server
origin onto the FDW?  How is it that the urgent need for the FDW to check
for that isn't even mentioned in the documentation?

I think that we'd really be better off insisting on same server (as in
same pg_foreign_server OID), hence automatically same FDW, and what's
even more important, same user mapping for any possible query execution
context.  The possibility that there are some corner cases where some FDWs
could optimize other scenarios seems to me to be poor return for the bugs
and security holes that will arise any time typical FDWs forget to check
this.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-05-08 Thread Robert Haas

On Fri, May 8, 2015 at 2:51 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Well, we have two alternatives.  I can keep hacking on this and get it
 to a state where it seems credible to me, but we won't have any proof
 that it actually works (though perhaps we could treat any problems
 as bugs that should hopefully get found before 9.5 ships, if a
 postgres_fdw patch shows up in the next few months).  Or we could
 revert the whole thing and bounce it to the 9.6 cycle.  I don't really
 like doing the latter, but I'm pretty uncomfortable with committing to
 published FDW APIs that are (a) as messy as this and (b) practically
 untested.  The odds that something slipped through the cracks are high.

A lot of work went into this patch.  I think it would be a shame to
revert it.  I'd even rather ship something imperfect or somewhat
unstable and change it later than give up and roll it all back.

 Aside from the other gripes I raised, I'm exceedingly unhappy with the
 ad-hoc APIs proposed for GetForeignJoinPaths and set_join_pathlist_hook.
 It's okay for internal calls in joinpath.c to look like that, but
 exporting that set of parameters seems like pure folly.  We've changed
 those parameter lists repeatedly (for instance in 9.2 and again in 9.3);
 the odds that they'll need to change again in future approach 100%.

 One way we could reduce the risk of code breakage there is to stuff all
 or most of those parameters into a struct.  This might result in a small
 slowdown for the internal calls, or then again maybe not --- there
 probably aren't many architectures that can pass 10 parameters in
 registers anyway.

Putting it into a structure certainly seems fine.  I think it's pretty
silly to assume that the FDW APIs are frozen or we're never going to
change them.  There was much discussion of the merits of exposing that
information or not, and I was (and am) convinced that the FDWs need
access to most if not all of that stuff, and that removing access to
it will cripple the facility and result in mountains of duplicated and
inefficient code.  If in the future we compute more or different stuff
there, I expect there's a good chance that FDWs will need to be
updated to look at that stuff too.  Of course, I don't object to
maximizing our chances of not needing an API break, but I will be
neither surprised nor disappointed if such efforts fail.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-05-08 Thread Robert Haas

On Fri, May 8, 2015 at 5:48 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 ... btw, I just noticed something that had escaped me because it seems so
 obviously wrong that I had not even stopped to consider the possibility
 that the code was doing what it's doing.  To wit, that the planner
 supposes that two foreign tables are potentially remote-joinable if they
 share the same underlying FDW handler function.  Not the same server, and
 not even the same pg_foreign_data_wrapper entry, but the pg_proc entry for
 the handler function.  I think this is fundamentally bogus.  Under what
 circumstances are we not just laying off the need to check same server
 origin onto the FDW?  How is it that the urgent need for the FDW to check
 for that isn't even mentioned in the documentation?

 I think that we'd really be better off insisting on same server (as in
 same pg_foreign_server OID), hence automatically same FDW, and what's
 even more important, same user mapping for any possible query execution
 context.  The possibility that there are some corner cases where some FDWs
 could optimize other scenarios seems to me to be poor return for the bugs
 and security holes that will arise any time typical FDWs forget to check
 this.

I originally wanted to go quite the other way with this and check for
join pushdown via handler X any time at least one of the two relations
involved used handler X, even if the other one used some other handler
or was a plain table.  In particular, it seems to me quite plausible
to want to teach an FDW that a certain local table is replicated on a
remote node, allowing a join between a foreign table and a plain table
to be pushed down.  This infrastructure can't be used that way anyhow,
so maybe there's no harm in tightening it up, but I'm wary of
circumscribing what FDW authors can do.  I think it's better to be
rather expansive in terms of when we call them and let them return
without doing anything some of them time than to define the situations
in which we call them too narrowly and end up ruling out interesting
use cases.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-05-08 Thread Kohei KaiGai

2015-05-09 2:46 GMT+09:00 Tom Lane t...@sss.pgh.pa.us:
Kouhei Kaigai kai...@ak.jp.nec.com writes:
I've been trying to code-review this patch, because the documentation
seemed several bricks shy of a load, and I find myself entirely confused
by the fdw_ps_tlist and custom_ps_tlist fields.

Main-point of your concern is lack of documentation/comments to introduce
how does the pseudo-scan targetlist works here, isn't it??

Well, there's a bunch of omissions and outright errors in the docs and
comments, but this is the main issue that I was uncertain how to fix
from looking at the patch.

The business about
resjunk columns in that list also seems a bit half baked, or at least
underdocumented.

I'll add source code comments to introduce how does it works any when
does it have resjunk=true. It will be a bit too deep to be introduced
in the SGML file.

http://www.postgresql.org/message-id/9a28c8860f777e439aa12e8aea7694f8010d7...@bpxm15gp.gisp.nec.co.jp

OK, I'll do the both of changes. The name of ps_tlist is a shorten of
pseudo-scan target-list. So, fdw_scan_tlist/custom_scan_tlist are
almost intentional.

Thanks,
--
KaiGai Kohei kai...@kaigai.gr.jp

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-05-08 Thread Kohei KaiGai

2015-05-09 3:51 GMT+09:00 Tom Lane t...@sss.pgh.pa.us:
 Robert Haas robertmh...@gmail.com writes:
 On Fri, May 8, 2015 at 1:46 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 That's nice, but 9.5 feature freeze is only a week away.  I don't have a
 lot of confidence that this stuff is actually in a state where we won't
 regret shipping it in 9.5.

 Yeah.  The POC you were asking for upthread certainly exists and has
 for a while, or I would not have committed this.  But I do not think
 it likely that the  postgres_fdw support will be ready for 9.5.

 Well, we have two alternatives.  I can keep hacking on this and get it
 to a state where it seems credible to me, but we won't have any proof
 that it actually works (though perhaps we could treat any problems
 as bugs that should hopefully get found before 9.5 ships, if a
 postgres_fdw patch shows up in the next few months).  Or we could
 revert the whole thing and bounce it to the 9.6 cycle.  I don't really
 like doing the latter, but I'm pretty uncomfortable with committing to
 published FDW APIs that are (a) as messy as this and (b) practically
 untested.  The odds that something slipped through the cracks are high.

 Aside from the other gripes I raised, I'm exceedingly unhappy with the
 ad-hoc APIs proposed for GetForeignJoinPaths and set_join_pathlist_hook.
 It's okay for internal calls in joinpath.c to look like that, but
 exporting that set of parameters seems like pure folly.  We've changed
 those parameter lists repeatedly (for instance in 9.2 and again in 9.3);
 the odds that they'll need to change again in future approach 100%.

 One way we could reduce the risk of code breakage there is to stuff all
 or most of those parameters into a struct.  This might result in a small
 slowdown for the internal calls, or then again maybe not --- there
 probably aren't many architectures that can pass 10 parameters in
 registers anyway.

Is it like a following structure definition?

  typedef struct
  {
PlannerInfo *root;
RelOptInfo *joinrel;
RelOptInfo *outerrel;
RelOptInfo *innerrel;
List *restrictlist;
JoinType jointype;
SpecialJoinInfo *sjinfo;
SemiAntiJoinFactors *semifactors;
Relids param_source_rels;
Relids extra_lateral_rels;
  } SetJoinPathListArgs;

I agree the idea. It also helps CSP driver implementation where it calls
next driver that was already chained on its installation.

  if (set_join_pathlist_next)
  set_join_pathlist_next(args);

is more stable manner than

  if (set_join_pathlist_next)
  set_join_pathlist_next(root,
   joinrel,
   outerrel,
   innerrel,
   restrictlist,
   jointype,
   sjinfo,
   semifactors,
   param_source_rels,
   extra_lateral_rels);

Thanks,
--
KaiGai Kohei kai...@kaigai.gr.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-05-08 Thread Kohei KaiGai

2015-05-09 6:48 GMT+09:00 Tom Lane t...@sss.pgh.pa.us:
 ... btw, I just noticed something that had escaped me because it seems so
 obviously wrong that I had not even stopped to consider the possibility
 that the code was doing what it's doing.  To wit, that the planner
 supposes that two foreign tables are potentially remote-joinable if they
 share the same underlying FDW handler function.  Not the same server, and
 not even the same pg_foreign_data_wrapper entry, but the pg_proc entry for
 the handler function.  I think this is fundamentally bogus.  Under what
 circumstances are we not just laying off the need to check same server
 origin onto the FDW?  How is it that the urgent need for the FDW to check
 for that isn't even mentioned in the documentation?

Indeed. Comparison of fdw_handler may cause unexpected behavior.
I agree it needs to be fixed up.

 I think that we'd really be better off insisting on same server (as in
 same pg_foreign_server OID), hence automatically same FDW, and what's
 even more important, same user mapping for any possible query execution
 context.  The possibility that there are some corner cases where some FDWs
 could optimize other scenarios seems to me to be poor return for the bugs
 and security holes that will arise any time typical FDWs forget to check
 this.

The former version of foreign/custom-join patch did check for joinable relations
using FDW server id, however, it was changed to the current form because it
may have additional optimization opportunity - in case when multiple foreign
servers have same remote host, access credential and others...
Also, I understand your concern about potential security holes by oversight.
It is an issue like a weighing scales, however, it seems to me the benefit
come from the potential optimization case does not negate the security-
hole risk enough.
So, I'll make a patch to change the logic to check joinable foreign-tables.

Thanks,
--
KaiGai Kohei kai...@kaigai.gr.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-05-07 Thread Tom Lane

Robert Haas robertmh...@gmail.com writes:
 On Thu, Apr 30, 2015 at 5:21 PM, Kouhei Kaigai kai...@ak.jp.nec.com wrote:
 I wanted to submit the v14 after the above items get clarified.
 The attached patch (v14) includes all what you suggested in the previous
 message.

 Committed, after heavily working over the documentation, and with some
 more revisions to the comments as well.

I've been trying to code-review this patch, because the documentation
seemed several bricks shy of a load, and I find myself entirely confused
by the fdw_ps_tlist and custom_ps_tlist fields.  The names, along with
some of the comments, imply that these are just targetlists for the join
nodes; but if that is the case then we don't need them, because surely
scan.targetlist would serve the purpose.  There is some other, utterly
uncommented, code in setrefs.c and ruleutils.c that suggests these fields
are supposed to serve a purpose more like IndexOnlyScan.indextlist; but
if that's what they are the comments are woefully inadequate/misleading,
and I'm really unsure that the associated code actually works.  Also,
if that is what they're for (ie, to allow the FDW to redefine the scan
tuple contents) it would likely be better to decouple that feature from
whether the plan node is for a simple scan or a join.  The business about
resjunk columns in that list also seems a bit half baked, or at least
underdocumented.

I do not think that this should have gotten committed without an attendant
proof-of-concept patch to postgres_fdw, so that the logic could be tested.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-05-07 Thread Kouhei Kaigai

 Robert Haas robertmh...@gmail.com writes:
  On Thu, Apr 30, 2015 at 5:21 PM, Kouhei Kaigai kai...@ak.jp.nec.com wrote:
  I wanted to submit the v14 after the above items get clarified.
  The attached patch (v14) includes all what you suggested in the previous
  message.
 
  Committed, after heavily working over the documentation, and with some
  more revisions to the comments as well.
 
 I've been trying to code-review this patch, because the documentation
 seemed several bricks shy of a load, and I find myself entirely confused
 by the fdw_ps_tlist and custom_ps_tlist fields.  The names, along with
 some of the comments, imply that these are just targetlists for the join
 nodes; but if that is the case then we don't need them, because surely
 scan.targetlist would serve the purpose.  There is some other, utterly
 uncommented, code in setrefs.c and ruleutils.c that suggests these fields
 are supposed to serve a purpose more like IndexOnlyScan.indextlist; but
 if that's what they are the comments are woefully inadequate/misleading,
 and I'm really unsure that the associated code actually works.

Main-point of your concern is lack of documentation/comments to introduce
how does the pseudo-scan targetlist works here, isn't it??

 Also,
 if that is what they're for (ie, to allow the FDW to redefine the scan
 tuple contents) it would likely be better to decouple that feature from
 whether the plan node is for a simple scan or a join.

In this version, we don't intend FDW/CSP to redefine the contents of
scan tuples, even though I want off-loads heavy targetlist calculation
workloads to external computing resources in *the future version*.

 The business about
 resjunk columns in that list also seems a bit half baked, or at least
 underdocumented.

I'll add source code comments to introduce how does it works any when
does it have resjunk=true. It will be a bit too deep to be introduced
in the SGML file.

 I do not think that this should have gotten committed without an attendant
 proof-of-concept patch to postgres_fdw, so that the logic could be tested.

Hanada-san is now working according to the comments from Robert.
Overall design was already discussed in the upthread and the latest
implementation follows the people's consensus.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei kai...@ak.jp.nec.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-05-01 Thread Robert Haas

On Thu, Apr 30, 2015 at 5:21 PM, Kouhei Kaigai kai...@ak.jp.nec.com wrote:
 I wanted to submit the v14 after the above items get clarified.
 The attached patch (v14) includes all what you suggested in the previous
 message.

Committed, after heavily working over the documentation, and with some
more revisions to the comments as well.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-04-30 Thread Kouhei Kaigai

 On Sun, Apr 26, 2015 at 10:00 PM, Kouhei Kaigai kai...@ak.jp.nec.com wrote:
  The attached patch v13 is revised one according to the suggestion
  by Robert.
 
 Thanks.
 
 The last hunk in foreign.c is a useless whitespace change.

Sorry, my oversight.

 +   /* actually, not shift members */
 
 Change to: shift of 0 is the same as copying
 
 But actually, do we really need all of this?  I think you could reduce
 the size of this function to three lines of code if you just did this:
 
 x = -1;
 while ((x = bms_next_member(inputset, x)) = 0)
 outputset = bms_add_member(inputset, x + shift);
 
 It might be very slightly slower, but I think it would be worth it to
 reduce the amount of code needed.

OK, I reverted the bms_shift_members().

It seems to me the code block for T_ForeignScan and T_CustomScan in
setrefs.c are a bit large. It may be better to have a separate
function like T_IndexOnlyScan.
How about your opinion?

 +* 5. Consider paths added by FDW, in case when both of outer and
 +* inner relations are managed by the same driver.
 
 Change to: If both inner and outer relations are managed by the same
 FDW, give it a chance to push down joins.

OK,

 +* 6. At the last, consider paths added by extension, in addition to 
 the
 +* built-in paths.
 
 Change to: Finally, give extensions a chance to manipulate the path list.

OK,

 +* Fetch relation-id, if this foreign-scan node actuall scans on
 +* a particular real relation. Elsewhere, InvalidOid shall be
 +* informed to the FDW driver.
 
 Change to: If we're scanning a base relation, look up the OID.  (We
 can skip this if scanning a join relation.)

OK,

 +* Sanity check. Pseudo scan tuple-descriptor shall be constructed
 +* based on the fdw_ps_tlist, excluding resjunk=true, so we need to
 +* ensure all valid TLEs have to locate prior to junk ones.
 
 Is the goal here to make attribute numbers match up?  If so, between
 where and where?  If not, please explain further.

No, its purpose is to reduce unnecessary projection.

The *_ps_tlist is not only used to construct tuple-descriptor of
Foreign/CustomScan with scanrelid==0, but also used to resolve var-
nodes with varno==INDEX_VAR in EXPLAIN command.

For example,
  SELECT t1.y, t2.b FROM t1, t2 WHERE t1.x = t2.a;

If t1.x = t2.a is executable on external computing resource (like
remote RDBMS or GPU device, etc), both of t1.x and t2.a don't need
to appear on the targetlist of joinrel.
In this case, the best *_ps_tlist consists of two var-nodes of t1.x
and t2.a because it fits tuple-descriptor of result tuple slot, thus
it can skip per-tuple projection.

On the other hands, we may want to print out expression clause that
shall be executed on the external resource; t1.x = t2.a in this
case. If FDW/CSP keeps this clause in expression form, its var-nodes
shall be rewritten to a pair of INDEX_VAR and resno on *_ps_tlist.
So, deparse_expression() needs to be capable to find out t1.x and
t2.a on the *_ps_tlist. However, it does not make sense to include
these variables on the scan tuple-descriptor.

ExecInitForeignScan() and ExecInitCustomScan() makes its scan tuple-
descriptor using ExecCleanTypeFromTL(), not ExecTypeFromTL(), to omit
these unreferenced variables on the *_ps_tlist. All the var-nodes with
INDEX_VAR shall be identified by offset from head of the list, we cannot
allow any target-entry with resjunk=false after ones with resjunk=true,
to keep the expected varattno.

This sanity checks ensures no target-entry with resjunk=false after
the resjunk=true. It helps to distinct attributes to be included in
the result tuple from the ones for just reference in EXPLAIN.

Did my explain above introduced the reason of this sanity check well?


 +   if (splan-scan.scanrelid == 0)
 +   {
 ...
 +   }
 splan-scan.scanrelid += rtoffset;
 
 Does this need an else?  It seems surprising that you would offset
 scanrelid even if it's starting out as zero.
 
 (Note that there are two instances of this pattern.)

'break' was put on the tail of if-block, however, it may lead potential
bugs in the future. I'll use if-else manner as usual.

 + * 'found' : indicates whether RelOptInfo is actually constructed.
 + * true, if it was already built and on the cache.
 
 Leftover hunk.  Revert this.

Fixed,

 +typedef void (*GetForeignJoinPaths_function ) (PlannerInfo *root,
 
 Whitespace is wrong, still.

Fixed,

 + * An optional fdw_ps_tlist is used to map a reference to an attribute of
 + * underlying relation(s) on a pair of INDEX_VAR and alternative varattno.
 
 on - onto

OK,

 + * It looks like a scan on pseudo relation that is usually result of
 + * relations join on remote data source, and FDW driver is responsible to
 + * set expected target list for this.
 
 Change to: When fdw_ps_tlist is used, this

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-04-30 Thread Kouhei Kaigai

 On Thu, Apr 30, 2015 at 9:16 AM, Kouhei Kaigai kai...@ak.jp.nec.com wrote:
  It seems to me the code block for T_ForeignScan and T_CustomScan in
  setrefs.c are a bit large. It may be better to have a separate
  function like T_IndexOnlyScan.
  How about your opinion?
 
 Either way is OK with me.  Please do as you think best.

OK, in setrefs.c, I moved the code block for T_ForeignScan and T_CustomScan
into set_foreignscan_references() and set_customscan_references() for each.
Its nest-level is a bit deep to keep all the stuff within 80-characters row.
It also uses bms_add_member(), instead of bms_shift_members() reverted.

  +* Sanity check. Pseudo scan tuple-descriptor shall be constructed
  +* based on the fdw_ps_tlist, excluding resjunk=true, so we need to
  +* ensure all valid TLEs have to locate prior to junk ones.
 
  Is the goal here to make attribute numbers match up?  If so, between
  where and where?  If not, please explain further.
 
  No, its purpose is to reduce unnecessary projection.
 
  The *_ps_tlist is not only used to construct tuple-descriptor of
  Foreign/CustomScan with scanrelid==0, but also used to resolve var-
  nodes with varno==INDEX_VAR in EXPLAIN command.
 
  For example,
SELECT t1.y, t2.b FROM t1, t2 WHERE t1.x = t2.a;
 
  If t1.x = t2.a is executable on external computing resource (like
  remote RDBMS or GPU device, etc), both of t1.x and t2.a don't need
  to appear on the targetlist of joinrel.
  In this case, the best *_ps_tlist consists of two var-nodes of t1.x
  and t2.a because it fits tuple-descriptor of result tuple slot, thus
  it can skip per-tuple projection.
 
  On the other hands, we may want to print out expression clause that
  shall be executed on the external resource; t1.x = t2.a in this
  case. If FDW/CSP keeps this clause in expression form, its var-nodes
  shall be rewritten to a pair of INDEX_VAR and resno on *_ps_tlist.
  So, deparse_expression() needs to be capable to find out t1.x and
  t2.a on the *_ps_tlist. However, it does not make sense to include
  these variables on the scan tuple-descriptor.
 
  ExecInitForeignScan() and ExecInitCustomScan() makes its scan tuple-
  descriptor using ExecCleanTypeFromTL(), not ExecTypeFromTL(), to omit
  these unreferenced variables on the *_ps_tlist. All the var-nodes with
  INDEX_VAR shall be identified by offset from head of the list, we cannot
  allow any target-entry with resjunk=false after ones with resjunk=true,
  to keep the expected varattno.
 
  This sanity checks ensures no target-entry with resjunk=false after
  the resjunk=true. It helps to distinct attributes to be included in
  the result tuple from the ones for just reference in EXPLAIN.
 
  Did my explain above introduced the reason of this sanity check well?
 
 Yeah, I think so.  So what we want to do in this comment is summarize
 all of that briefly.  Maybe something like this:
 
 Sanity check.  There may be resjunk entries in fdw_ps_tlist that are
 included only to help EXPLAIN deparse plans properly.  We require that
 these are at the end, so that when the executor builds the scan
 descriptor based on the non-junk entries, it gets the attribute
 numbers correct.

Thanks, I used this sentence as is.

  +   if (splan-scan.scanrelid == 0)
  +   {
  ...
  +   }
  splan-scan.scanrelid += rtoffset;
 
  Does this need an else?  It seems surprising that you would offset
  scanrelid even if it's starting out as zero.
 
  (Note that there are two instances of this pattern.)
 
  'break' was put on the tail of if-block, however, it may lead potential
  bugs in the future. I'll use if-else manner as usual.
 
 Ah, OK, I missed that.  Yeah, that's probably a good change.

set_foreignscan_references() and set_customscan_references() are
split by two portions using the manner above; a code block if scanrelid==0
and others.

 I assume you realize you did not attach an updated patch?

I wanted to submit the v14 after the above items get clarified.
The attached patch (v14) includes all what you suggested in the previous
message.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei kai...@ak.jp.nec.com



pgsql-v9.5-custom-join.v14.patch
Description: pgsql-v9.5-custom-join.v14.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-04-30 Thread Robert Haas

On Thu, Apr 30, 2015 at 9:16 AM, Kouhei Kaigai kai...@ak.jp.nec.com wrote:
 It seems to me the code block for T_ForeignScan and T_CustomScan in
 setrefs.c are a bit large. It may be better to have a separate
 function like T_IndexOnlyScan.
 How about your opinion?

Either way is OK with me.  Please do as you think best.

 +* Sanity check. Pseudo scan tuple-descriptor shall be constructed
 +* based on the fdw_ps_tlist, excluding resjunk=true, so we need to
 +* ensure all valid TLEs have to locate prior to junk ones.

 Is the goal here to make attribute numbers match up?  If so, between
 where and where?  If not, please explain further.

 No, its purpose is to reduce unnecessary projection.

 The *_ps_tlist is not only used to construct tuple-descriptor of
 Foreign/CustomScan with scanrelid==0, but also used to resolve var-
 nodes with varno==INDEX_VAR in EXPLAIN command.

 For example,
   SELECT t1.y, t2.b FROM t1, t2 WHERE t1.x = t2.a;

 If t1.x = t2.a is executable on external computing resource (like
 remote RDBMS or GPU device, etc), both of t1.x and t2.a don't need
 to appear on the targetlist of joinrel.
 In this case, the best *_ps_tlist consists of two var-nodes of t1.x
 and t2.a because it fits tuple-descriptor of result tuple slot, thus
 it can skip per-tuple projection.

 On the other hands, we may want to print out expression clause that
 shall be executed on the external resource; t1.x = t2.a in this
 case. If FDW/CSP keeps this clause in expression form, its var-nodes
 shall be rewritten to a pair of INDEX_VAR and resno on *_ps_tlist.
 So, deparse_expression() needs to be capable to find out t1.x and
 t2.a on the *_ps_tlist. However, it does not make sense to include
 these variables on the scan tuple-descriptor.

 ExecInitForeignScan() and ExecInitCustomScan() makes its scan tuple-
 descriptor using ExecCleanTypeFromTL(), not ExecTypeFromTL(), to omit
 these unreferenced variables on the *_ps_tlist. All the var-nodes with
 INDEX_VAR shall be identified by offset from head of the list, we cannot
 allow any target-entry with resjunk=false after ones with resjunk=true,
 to keep the expected varattno.

 This sanity checks ensures no target-entry with resjunk=false after
 the resjunk=true. It helps to distinct attributes to be included in
 the result tuple from the ones for just reference in EXPLAIN.

 Did my explain above introduced the reason of this sanity check well?

Yeah, I think so.  So what we want to do in this comment is summarize
all of that briefly.  Maybe something like this:

Sanity check.  There may be resjunk entries in fdw_ps_tlist that are
included only to help EXPLAIN deparse plans properly.  We require that
these are at the end, so that when the executor builds the scan
descriptor based on the non-junk entries, it gets the attribute
numbers correct.

 +   if (splan-scan.scanrelid == 0)
 +   {
 ...
 +   }
 splan-scan.scanrelid += rtoffset;

 Does this need an else?  It seems surprising that you would offset
 scanrelid even if it's starting out as zero.

 (Note that there are two instances of this pattern.)

 'break' was put on the tail of if-block, however, it may lead potential
 bugs in the future. I'll use if-else manner as usual.

Ah, OK, I missed that.  Yeah, that's probably a good change.

I assume you realize you did not attach an updated patch?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-04-29 Thread Ashutosh Bapat

On Fri, Apr 24, 2015 at 3:08 PM, Shigeru HANADA shigeru.han...@gmail.com
wrote:

 Hi Ashutosh,

 Thanks for the review.

 2015/04/22 19:28、Ashutosh Bapat ashutosh.ba...@enterprisedb.com のメール：
  Tests
  ---
  1.The postgres_fdw test is re/setting enable_mergejoin at various
 places. The goal of these tests seems to be to test the sanity of foreign
 plans generated. So, it might be better to reset enable_mergejoin (and may
 be all of enable_hashjoin, enable_nestloop_join etc.) to false at the
 beginning of the testcase and set them again at the end. That way, we will
 also make sure that foreign plans are chosen irrespective of future planner
 changes.

 I have different, rather opposite opinion about it.  I disabled other join
 types as least as the tests pass, because I worry oversights come from
 planner changes.  I hope to eliminate enable_foo from the test script, by
 improving costing model smarter.


Ok, if you can do that, that will be excellent.



  2. In the patch, I see that the inheritance testcases have been deleted
 from postgres_fdw.sql, is that intentional? I do not see those being
 replaced anywhere else.

 It’s accidental removal, I restored the tests about inheritance feature.


Thanks.



  3. We need one test for each join type (or at least for INNER and LEFT
 OUTER) where there are unsafe to push conditions in ON clause along-with
 safe-to-push conditions. For INNER join, the join should get pushed down
 with the safe conditions and for OUTER join it shouldn't be. Same goes for
 WHERE clause, in which case the join will be pushed down but the
 unsafe-to-push conditions will be applied locally.

 Currently INNER JOINs with unsafe join conditions are not pushed down, so
 such test is not in the suit.  As you say, in theory, INNER JOINs can be
 pushed down even they have push-down-unsafe join conditions, because such
 conditions can be evaluated no local side against rows retrieved without
 those conditions.

  4. All the tests have ORDER BY, LIMIT in them, so the setref code is
 being exercised. But, something like aggregates would test the setref code
 better. So, we should add at-least one test like select avg(ft1.c1 +
 ft2.c2) from ft1 join ft2 on (ft1.c1 = ft2.c1).

 Added an aggregate case, and also added an UNION case for Append.


Thanks.



  5. It will be good to add some test which contain join between few
 foreign and few local tables to see whether we are able to push down the
 largest possible foreign join tree to the foreign server.
 


Are you planning to do anything on this point?




  Code
  ---
  In classifyConditions(), the code is now appending RestrictInfo::clause
 rather than RestrictInfo itself. But the callers of classifyConditions()
 have not changed. Is this change intentional?

 Yes, the purpose of the change is to make appendConditions (former name is
 appendWhereClause) can handle JOIN ON clause, list of Expr.

  The functions which consume the lists produced by this function handle
 expressions as well RestrictInfo, so you may not have noticed it. Because
 of this change, we might be missing some optimizations e.g. in function
 postgresGetForeignPlan()
   793 if (list_member_ptr(fpinfo-remote_conds, rinfo))
   794 remote_conds = lappend(remote_conds, rinfo-clause);
   795 else if (list_member_ptr(fpinfo-local_conds, rinfo))
   796 local_exprs = lappend(local_exprs, rinfo-clause);
   797 else if (is_foreign_expr(root, baserel, rinfo-clause))
   798 remote_conds = lappend(remote_conds, rinfo-clause);
   799 else
   800 local_exprs = lappend(local_exprs, rinfo-clause);
  Finding a RestrictInfo in remote_conds avoids another call to
 is_foreign_expr(). So with this change, I think we are doing an extra call
 to is_foreign_expr().
 

 Hm, it seems better to revert my change and make appendConditions downcast
 given information into RestrictInfo or Expr according to the node tag.


Thanks.



  The function get_jointype_name() returns an empty string for unsupported
 join types. Instead of that it should throw an error, if some code path
 accidentally calls the function with unsupported join type e.g. SEMI_JOIN.

 Agreed, fixed.

Thanks.



  While deparsing the SQL with rowmarks, the placement of FOR UPDATE/SHARE
 clause in the original query is not being honored, which means that we will
 end up locking the rows which are not part of the join result even when the
 join is pushed to the foreign server. E.g take the following query (it uses
 the tables created in postgres_fdw.sql tests)
  contrib_regression=# explain verbose select * from ft1 join ft2 on
 (ft1.c1 = ft2.c1) for update of ft1;
 
 
   QUERY PLAN
 
 
 
 ---

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-04-29 Thread Robert Haas

On Mon, Apr 27, 2015 at 5:05 AM, Shigeru HANADA
shigeru.han...@gmail.com wrote:
 Currently INNER JOINs with unsafe join conditions are not pushed down, so 
 such test is not in the suit.  As you say, in theory, INNER JOINs can be 
 pushed down even they have push-down-unsafe join conditions, because such 
 conditions can be evaluated no local side against rows retrieved without 
 those conditions.

I suspect it's worth trying to do the pushdown if there is at least
one safe joinclause.  If there are none, fetching a Cartesian product
figures to be a loser.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-04-29 Thread Robert Haas

On Sun, Apr 26, 2015 at 10:00 PM, Kouhei Kaigai kai...@ak.jp.nec.com wrote:
 The attached patch v13 is revised one according to the suggestion
 by Robert.

Thanks.

The last hunk in foreign.c is a useless whitespace change.

+   /* actually, not shift members */

Change to: shift of 0 is the same as copying

But actually, do we really need all of this?  I think you could reduce
the size of this function to three lines of code if you just did this:

x = -1;
while ((x = bms_next_member(inputset, x)) = 0)
outputset = bms_add_member(inputset, x + shift);

It might be very slightly slower, but I think it would be worth it to
reduce the amount of code needed.

+* 5. Consider paths added by FDW, in case when both of outer and
+* inner relations are managed by the same driver.

Change to: If both inner and outer relations are managed by the same
FDW, give it a chance to push down joins.

+* 6. At the last, consider paths added by extension, in addition to the
+* built-in paths.

Change to: Finally, give extensions a chance to manipulate the path list.

+* Fetch relation-id, if this foreign-scan node actuall scans on
+* a particular real relation. Elsewhere, InvalidOid shall be
+* informed to the FDW driver.

Change to: If we're scanning a base relation, look up the OID.  (We
can skip this if scanning a join relation.)

+* Sanity check. Pseudo scan tuple-descriptor shall be constructed
+* based on the fdw_ps_tlist, excluding resjunk=true, so we need to
+* ensure all valid TLEs have to locate prior to junk ones.

Is the goal here to make attribute numbers match up?  If so, between
where and where?  If not, please explain further.

+   if (splan-scan.scanrelid == 0)
+   {
...
+   }
splan-scan.scanrelid += rtoffset;

Does this need an else?  It seems surprising that you would offset
scanrelid even if it's starting out as zero.

(Note that there are two instances of this pattern.)

+ * 'found' : indicates whether RelOptInfo is actually constructed.
+ * true, if it was already built and on the cache.

Leftover hunk.  Revert this.

+typedef void (*GetForeignJoinPaths_function ) (PlannerInfo *root,

Whitespace is wrong, still.

+ * An optional fdw_ps_tlist is used to map a reference to an attribute of
+ * underlying relation(s) on a pair of INDEX_VAR and alternative varattno.

on - onto

+ * It looks like a scan on pseudo relation that is usually result of
+ * relations join on remote data source, and FDW driver is responsible to
+ * set expected target list for this.

Change to: When fdw_ps_tlist is used, this represents a remote join,
and the FDW driver is responsible for setting this field to an
appropriate value.

If FDW returns records as foreign-
+ * table definition, just put NIL here.

I think this is just referring to the non-join case; if so, just drop
it.  Otherwise, I'm confused and need a further explanation.

+ *  Note that since Plan trees can be copied, custom scan providers *must*

Extra space before Note

+   Bitmapset  *custom_relids;  /* set of relid (index of range-tables)
+*
represented by this node */

Maybe RTIs this node generates?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-04-27 Thread Shigeru HANADA

Hi Ashutosh,

Thanks for the review.

2015/04/22 19:28、Ashutosh Bapat ashutosh.ba...@enterprisedb.com のメール：
 Tests
 ---
 1.The postgres_fdw test is re/setting enable_mergejoin at various places. The 
 goal of these tests seems to be to test the sanity of foreign plans 
 generated. So, it might be better to reset enable_mergejoin (and may be all 
 of enable_hashjoin, enable_nestloop_join etc.) to false at the beginning of 
 the testcase and set them again at the end. That way, we will also make sure 
 that foreign plans are chosen irrespective of future planner changes.

I have different, rather opposite opinion about it.  I disabled other join 
types as least as the tests pass, because I worry oversights come from planner 
changes.  I hope to eliminate enable_foo from the test script, by improving 
costing model smarter.

 2. In the patch, I see that the inheritance testcases have been deleted from 
 postgres_fdw.sql, is that intentional? I do not see those being replaced 
 anywhere else.

It’s accidental removal, I restored the tests about inheritance feature.

 3. We need one test for each join type (or at least for INNER and LEFT OUTER) 
 where there are unsafe to push conditions in ON clause along-with 
 safe-to-push conditions. For INNER join, the join should get pushed down with 
 the safe conditions and for OUTER join it shouldn't be. Same goes for WHERE 
 clause, in which case the join will be pushed down but the unsafe-to-push 
 conditions will be applied locally.

Currently INNER JOINs with unsafe join conditions are not pushed down, so such 
test is not in the suit.  As you say, in theory, INNER JOINs can be pushed down 
even they have push-down-unsafe join conditions, because such conditions can be 
evaluated no local side against rows retrieved without those conditions.

 4. All the tests have ORDER BY, LIMIT in them, so the setref code is being 
 exercised. But, something like aggregates would test the setref code better. 
 So, we should add at-least one test like select avg(ft1.c1 + ft2.c2) from ft1 
 join ft2 on (ft1.c1 = ft2.c1).

Added an aggregate case, and also added an UNION case for Append.

 5. It will be good to add some test which contain join between few foreign 
 and few local tables to see whether we are able to push down the largest 
 possible foreign join tree to the foreign server. 
 





 Code
 ---
 In classifyConditions(), the code is now appending RestrictInfo::clause 
 rather than RestrictInfo itself. But the callers of classifyConditions() have 
 not changed. Is this change intentional?

Yes, the purpose of the change is to make appendConditions (former name is 
appendWhereClause) can handle JOIN ON clause, list of Expr.

 The functions which consume the lists produced by this function handle 
 expressions as well RestrictInfo, so you may not have noticed it. Because of 
 this change, we might be missing some optimizations e.g. in function 
 postgresGetForeignPlan()
  793 if (list_member_ptr(fpinfo-remote_conds, rinfo))
  794 remote_conds = lappend(remote_conds, rinfo-clause);
  795 else if (list_member_ptr(fpinfo-local_conds, rinfo))
  796 local_exprs = lappend(local_exprs, rinfo-clause);
  797 else if (is_foreign_expr(root, baserel, rinfo-clause))
  798 remote_conds = lappend(remote_conds, rinfo-clause);
  799 else
  800 local_exprs = lappend(local_exprs, rinfo-clause);
 Finding a RestrictInfo in remote_conds avoids another call to 
 is_foreign_expr(). So with this change, I think we are doing an extra call to 
 is_foreign_expr().
 

Hm, it seems better to revert my change and make appendConditions downcast 
given information into RestrictInfo or Expr according to the node tag.

 The function get_jointype_name() returns an empty string for unsupported join 
 types. Instead of that it should throw an error, if some code path 
 accidentally calls the function with unsupported join type e.g. SEMI_JOIN.

Agreed, fixed.

 While deparsing the SQL with rowmarks, the placement of FOR UPDATE/SHARE 
 clause in the original query is not being honored, which means that we will 
 end up locking the rows which are not part of the join result even when the 
 join is pushed to the foreign server. E.g take the following query (it uses 
 the tables created in postgres_fdw.sql tests)
 contrib_regression=# explain verbose select * from ft1 join ft2 on (ft1.c1 = 
 ft2.c1) for update of ft1;
   
  
   
  
  QUERY PLAN

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-04-26 Thread Kouhei Kaigai

The attached patch v13 is revised one according to the suggestion
by Robert.

- eliminated useless change in allpaths.c
- eliminated an extra space in FdwRoutine definition
- prohibited to have scanrelid==0 by other than ForeignScan
  or CustomScan, using Assert()
- definition of currentRelation in ExecInitForeignScan() and
  ExecInitCustomScan() were moved inside of the if-block on
  scanrelid  0
- GetForeignJoinPaths() was redefined and moved to
  add_paths_to_joinrel(), like set_join_pathlist_hook.

As suggested, FDW driver can skip to add additional paths if
equivalent paths are already added to a certain joinrel by
checking fdw_private. So, we can achieve the purpose when we
once moved the entrypoint to make_join_rel() - no to populate
redundant paths for each potential join combinations, even
though remote RDBMS handles it correctly. It also makes sense
if remote RDBMS handles tables join according to the order of
relations appear.

Its definition is below:
  void GetForeignJoinPaths(PlannerInfo *root,
   RelOptInfo *joinrel,
   RelOptInfo *outerrel,
   RelOptInfo *innerrel,
   List *restrictlist,
   JoinType jointype,
   SpecialJoinInfo *sjinfo,
   SemiAntiJoinFactors *semifactors,
   Relids param_source_rels,
   Relids extra_lateral_rels);

In addition to the arguments in the previous version, we added
some parameters computed during add_paths_to_joinrel().
Right now, I'm not certain whether we should include mergeclause_list
here, because it depends on enable_mergejoin even though extra join
logic based on merge-join may not want to be controlled by this GUC.

Hanada-san, could you adjust your postgres_fdw patch according to
the above new (previous?) definition.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei kai...@ak.jp.nec.com


 -Original Message-
 From: Kaigai Kouhei(海外 浩平)
 Sent: Friday, April 24, 2015 11:23 PM
 To: 'Robert Haas'
 Cc: Tom Lane; Thom Brown; Shigeru Hanada; pgsql-hackers@postgreSQL.org
 Subject: Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
 
  On Wed, Apr 22, 2015 at 10:48 PM, Kouhei Kaigai kai...@ak.jp.nec.com 
  wrote:
   +   else if (scan-scanrelid == 0 
   +(IsA(scan, ForeignScan) || IsA(scan,
 CustomScan)))
   +   varno = INDEX_VAR;
  
   Suppose scan-scanrelid == 0 but the scan type is something else?  Is
   that legal?  Is varno == 0 the correct outcome in that case?
  
   Right now, no other scan type has capability to return a tuples
   with flexible type/attributes more than static definition.
   I think it is a valid restriction that only foreign/custom-scan
   can have scanrelid == 0.
 
  But the code as you've written it doesn't enforce any such
  restriction.  It just spends CPU cycles testing for a condition which,
  to the best of your knowledge, will never happen.
 
  If it's really a can't happen condition, how about checking it via an 
  Assert()?
 
  else if (scan-scanrelid == 0)
  {
  Assert(IsA(scan, ForeignScan) || IsA(scan, CustomScan));
  varno = INDEX_VAR;
  }
 
 Thanks for your suggestion. I'd like to use this idea on the next patch.
 
 --
 NEC Business Creation Division / PG-Strom Project
 KaiGai Kohei kai...@ak.jp.nec.com


pgsql-v9.5-custom-join.v13.patch
Description: pgsql-v9.5-custom-join.v13.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-04-24 Thread Shigeru HANADA

Hi Ashutosh,

Thanks for the review.

2015/04/22 19:28、Ashutosh Bapat ashutosh.ba...@enterprisedb.com のメール：
 Tests
 ---
 1.The postgres_fdw test is re/setting enable_mergejoin at various places. The 
 goal of these tests seems to be to test the sanity of foreign plans 
 generated. So, it might be better to reset enable_mergejoin (and may be all 
 of enable_hashjoin, enable_nestloop_join etc.) to false at the beginning of 
 the testcase and set them again at the end. That way, we will also make sure 
 that foreign plans are chosen irrespective of future planner changes.

I have different, rather opposite opinion about it.  I disabled other join 
types as least as the tests pass, because I worry oversights come from planner 
changes.  I hope to eliminate enable_foo from the test script, by improving 
costing model smarter.

 2. In the patch, I see that the inheritance testcases have been deleted from 
 postgres_fdw.sql, is that intentional? I do not see those being replaced 
 anywhere else.

It’s accidental removal, I restored the tests about inheritance feature.

 3. We need one test for each join type (or at least for INNER and LEFT OUTER) 
 where there are unsafe to push conditions in ON clause along-with 
 safe-to-push conditions. For INNER join, the join should get pushed down with 
 the safe conditions and for OUTER join it shouldn't be. Same goes for WHERE 
 clause, in which case the join will be pushed down but the unsafe-to-push 
 conditions will be applied locally.

Currently INNER JOINs with unsafe join conditions are not pushed down, so such 
test is not in the suit.  As you say, in theory, INNER JOINs can be pushed down 
even they have push-down-unsafe join conditions, because such conditions can be 
evaluated no local side against rows retrieved without those conditions.

 4. All the tests have ORDER BY, LIMIT in them, so the setref code is being 
 exercised. But, something like aggregates would test the setref code better. 
 So, we should add at-least one test like select avg(ft1.c1 + ft2.c2) from ft1 
 join ft2 on (ft1.c1 = ft2.c1).

Added an aggregate case, and also added an UNION case for Append.

 5. It will be good to add some test which contain join between few foreign 
 and few local tables to see whether we are able to push down the largest 
 possible foreign join tree to the foreign server. 
 





 Code
 ---
 In classifyConditions(), the code is now appending RestrictInfo::clause 
 rather than RestrictInfo itself. But the callers of classifyConditions() have 
 not changed. Is this change intentional?

Yes, the purpose of the change is to make appendConditions (former name is 
appendWhereClause) can handle JOIN ON clause, list of Expr.

 The functions which consume the lists produced by this function handle 
 expressions as well RestrictInfo, so you may not have noticed it. Because of 
 this change, we might be missing some optimizations e.g. in function 
 postgresGetForeignPlan()
  793 if (list_member_ptr(fpinfo-remote_conds, rinfo))
  794 remote_conds = lappend(remote_conds, rinfo-clause);
  795 else if (list_member_ptr(fpinfo-local_conds, rinfo))
  796 local_exprs = lappend(local_exprs, rinfo-clause);
  797 else if (is_foreign_expr(root, baserel, rinfo-clause))
  798 remote_conds = lappend(remote_conds, rinfo-clause);
  799 else
  800 local_exprs = lappend(local_exprs, rinfo-clause);
 Finding a RestrictInfo in remote_conds avoids another call to 
 is_foreign_expr(). So with this change, I think we are doing an extra call to 
 is_foreign_expr().
 

Hm, it seems better to revert my change and make appendConditions downcast 
given information into RestrictInfo or Expr according to the node tag.

 The function get_jointype_name() returns an empty string for unsupported join 
 types. Instead of that it should throw an error, if some code path 
 accidentally calls the function with unsupported join type e.g. SEMI_JOIN.

Agreed, fixed.

 While deparsing the SQL with rowmarks, the placement of FOR UPDATE/SHARE 
 clause in the original query is not being honored, which means that we will 
 end up locking the rows which are not part of the join result even when the 
 join is pushed to the foreign server. E.g take the following query (it uses 
 the tables created in postgres_fdw.sql tests)
 contrib_regression=# explain verbose select * from ft1 join ft2 on (ft1.c1 = 
 ft2.c1) for update of ft1;
   
  
   
  
  QUERY PLAN

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-04-24 Thread Robert Haas

On Wed, Apr 22, 2015 at 10:48 PM, Kouhei Kaigai kai...@ak.jp.nec.com wrote:
 +   else if (scan-scanrelid == 0 
 +(IsA(scan, ForeignScan) || IsA(scan, CustomScan)))
 +   varno = INDEX_VAR;

 Suppose scan-scanrelid == 0 but the scan type is something else?  Is
 that legal?  Is varno == 0 the correct outcome in that case?

 Right now, no other scan type has capability to return a tuples
 with flexible type/attributes more than static definition.
 I think it is a valid restriction that only foreign/custom-scan
 can have scanrelid == 0.

But the code as you've written it doesn't enforce any such
restriction.  It just spends CPU cycles testing for a condition which,
to the best of your knowledge, will never happen.

If it's really a can't happen condition, how about checking it via an Assert()?

else if (scan-scanrelid == 0)
{
Assert(IsA(scan, ForeignScan) || IsA(scan, CustomScan));
varno = INDEX_VAR;
}

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-04-24 Thread Kouhei Kaigai

 On Wed, Apr 22, 2015 at 10:48 PM, Kouhei Kaigai kai...@ak.jp.nec.com wrote:
  +   else if (scan-scanrelid == 0 
  +(IsA(scan, ForeignScan) || IsA(scan, CustomScan)))
  +   varno = INDEX_VAR;
 
  Suppose scan-scanrelid == 0 but the scan type is something else?  Is
  that legal?  Is varno == 0 the correct outcome in that case?
 
  Right now, no other scan type has capability to return a tuples
  with flexible type/attributes more than static definition.
  I think it is a valid restriction that only foreign/custom-scan
  can have scanrelid == 0.
 
 But the code as you've written it doesn't enforce any such
 restriction.  It just spends CPU cycles testing for a condition which,
 to the best of your knowledge, will never happen.
 
 If it's really a can't happen condition, how about checking it via an 
 Assert()?
 
 else if (scan-scanrelid == 0)
 {
 Assert(IsA(scan, ForeignScan) || IsA(scan, CustomScan));
 varno = INDEX_VAR;
 }

Thanks for your suggestion. I'd like to use this idea on the next patch.

--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei kai...@ak.jp.nec.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-04-22 Thread Robert Haas

On Tue, Apr 21, 2015 at 10:33 PM, Kouhei Kaigai kai...@ak.jp.nec.com wrote:
 [ new patch ]

A little more nitpicking:

ExecInitForeignScan() and ExecInitCustomScan() could declare
currentRelation inside the if (scanrelid  0) block instead of in the
outer scope.

I'm not too excited about the addition of GetFdwHandlerForRelation,
which is a one-line function used in one place.  It seems like we
don't really need that.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-04-22 Thread Robert Haas

On Wed, Mar 25, 2015 at 9:51 PM, Kouhei Kaigai kai...@ak.jp.nec.com wrote:
 The attached patch adds GetForeignJoinPaths call on make_join_rel() only when
 'joinrel' is actually built and both of child relations are managed by same
 FDW driver, prior to any other built-in join paths.
 I adjusted the hook definition a little bit, because jointype can be 
 reproduced
 using SpecialJoinInfo. Right?

 Probably, it will solve the original concern towards multiple calls of FDW
 handler in case when it tries to replace an entire join subtree with a 
 foreign-
 scan on the result of remote join query.

 How about your opinion?

A few random cosmetic problems:

- The hunk in allpaths.c is useless.
- The first hunk in fdwapi.h contains an extra space before the
closing parenthesis.

And then:

+   else if (scan-scanrelid == 0 
+(IsA(scan, ForeignScan) || IsA(scan, CustomScan)))
+   varno = INDEX_VAR;

Suppose scan-scanrelid == 0 but the scan type is something else?  Is
that legal?  Is varno == 0 the correct outcome in that case?

More later.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-04-22 Thread Robert Haas

On Fri, Mar 13, 2015 at 8:02 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 On Fri, Mar 13, 2015 at 2:31 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 I don't object to the concept, but I think that is a pretty bad place
 to put the hook call: add_paths_to_joinrel is typically called multiple
 (perhaps *many*) times per joinrel and thus this placement would force
 any user of the hook to do a lot of repetitive work.

 Interesting point.  I guess the question is whether a some or all
 callers are going to actually *want* a separate call for each
 invocation of add_paths_to_joinrel(), or whether they'll be happy to
 operate on the otherwise-complete path list.

 Hmm.  You're right, it's certainly possible that some users would like to
 operate on each possible pair of input relations, rather than considering
 the joinrel as a whole.  Maybe we need two hooks, one like your patch
 and one like I suggested.

Let me attempt to summarize subsequent discussion on this thread by
saying the hook location that you proposed (just before set_cheapest)
has not elicited any enthusiasm from anyone else.  In a nutshell, the
problem is that a single callback for a large join problem is just
fine if there are no special joins involved, but in any other
scenario, nobody knows how to use a hook at that location for anything
useful.  To push down a join to the remote server, you've got to
figure out how to emit an SQL query for it.  To execute it with a
custom join strategy, you've got to know which of those joins should
have inner join semantics vs. left join semantics.  A hook/callback in
make_join_rel() or in add_paths_to_joinrel() makes that relatively
straightforward. Otherwise, it's not clear what to do, short of
copy-and-pasting join_search_one_level().  If you have a suggestion,
I'd like to hear it.

If not, I'm going to press forward with the idea of putting the
relevant logic in either add_paths_to_joinrel(), as previously
proposed, or perhaps up oe level in make_one_rel().  Either way, if
you don't need to be called multiple times per joinrel, you can stash
a flag inside whatever you hang off of the joinrel's fdw_private and
return immediately on every call after the first.  I think that's
cheap enough that we shouldn't get too stressed about it: for FDWs, we
only call the hook at all if everything in the joinrel uses the same
FDW, so it won't get called at all except for joinrels where it's
likely to win big; for custom joins, multiple calls are quite likely
to be useful and necessary, and if the hook burns too much CPU time
for the query performance you get out of it, that's the custom-join
provider's fault, not ours.  The current patch takes this approach one
step further and attempts FDW pushdown only once per joinrel.  It does
that because, while postgres_fdw DOES need the jointype and a valid
innerrel/outerrel breakdown to figure out what query to generate, it
does NOT every possible breakdown; rather, the first one is as good as
any other. But this might not be true for a non-PostgreSQL remote
database.  So I think it's better to call the hook every time and let
the hook return without doing anything if it wants.

I'm still not totally sure whether make_one_rel() is better than
add_paths_to_joinrel().  The current patch attempts to split the
difference by doing FDW pushdown from make_one_rel() and custom joins
from add_paths_to_joinrel().  I dunno why; if possible, those two
things should happen in the same place.  Doing it in make_one_rel()
makes for fewer arguments and fewer repetitive calls, but that's not
much good if you would have had a use for the extra arguments that
aren't computed until we get down to add_paths_to_joinrel().  I'm not
sure whether that's the case or not.  The latest version of the
postgres_fdw patch doesn't seem to mind not having extra_lateral_rels,
but I'm wondering if that's busted.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-04-22 Thread Kouhei Kaigai

Hi Robert,

Thanks for your comments.

 A few random cosmetic problems:
 
 - The hunk in allpaths.c is useless.
 - The first hunk in fdwapi.h contains an extra space before the
 closing parenthesis.

OK, it's my oversight.

 And then:
 
 +   else if (scan-scanrelid == 0 
 +(IsA(scan, ForeignScan) || IsA(scan, CustomScan)))
 +   varno = INDEX_VAR;
 
 Suppose scan-scanrelid == 0 but the scan type is something else?  Is
 that legal?  Is varno == 0 the correct outcome in that case?

Right now, no other scan type has capability to return a tuples
with flexible type/attributes more than static definition.
I think it is a valid restriction that only foreign/custom-scan
can have scanrelid == 0.

I checked overall code again. One point doubtful was ExecScanFetch().
If estate-es_epqTuple is not NULL, it tries to save a tuple from
a particular scanrelid (larger than zero).
IIUC, es_epqTuple is used only when fetched tuple is updated then
visibility checks are applied on writer operation again.
So, it should work for CPS with underlying actual scan node on base
relations, however, I need code investigation if FDW/CSP replaced
an entire join subtree by an alternative relation scan (like a
materialized view).


  [ new patch ]
 
 A little more nitpicking:
 
 ExecInitForeignScan() and ExecInitCustomScan() could declare
 currentRelation inside the if (scanrelid  0) block instead of in the
 outer scope.

OK,

 I'm not too excited about the addition of GetFdwHandlerForRelation,
 which is a one-line function used in one place.  It seems like we
 don't really need that.

OK,

 On Fri, Mar 13, 2015 at 8:02 PM, Tom Lane t...@sss.pgh.pa.us wrote:
  Robert Haas robertmh...@gmail.com writes:
  On Fri, Mar 13, 2015 at 2:31 PM, Tom Lane t...@sss.pgh.pa.us wrote:
  I don't object to the concept, but I think that is a pretty bad place
  to put the hook call: add_paths_to_joinrel is typically called multiple
  (perhaps *many*) times per joinrel and thus this placement would force
  any user of the hook to do a lot of repetitive work.
 
  Interesting point.  I guess the question is whether a some or all
  callers are going to actually *want* a separate call for each
  invocation of add_paths_to_joinrel(), or whether they'll be happy to
  operate on the otherwise-complete path list.
 
  Hmm.  You're right, it's certainly possible that some users would like to
  operate on each possible pair of input relations, rather than considering
  the joinrel as a whole.  Maybe we need two hooks, one like your patch
  and one like I suggested.
 
 Let me attempt to summarize subsequent discussion on this thread by
 saying the hook location that you proposed (just before set_cheapest)
 has not elicited any enthusiasm from anyone else.  In a nutshell, the
 problem is that a single callback for a large join problem is just
 fine if there are no special joins involved, but in any other
 scenario, nobody knows how to use a hook at that location for anything
 useful.  To push down a join to the remote server, you've got to
 figure out how to emit an SQL query for it.  To execute it with a
 custom join strategy, you've got to know which of those joins should
 have inner join semantics vs. left join semantics.  A hook/callback in
 make_join_rel() or in add_paths_to_joinrel() makes that relatively
 straightforward. Otherwise, it's not clear what to do, short of
 copy-and-pasting join_search_one_level().  If you have a suggestion,
 I'd like to hear it.

Nothing I have. Once I tried to put a hook just after the set_cheapest(),
the largest problem was that we cannot extract a set of left and right
relations from a set of joined relations, like an extraction of apple
and orange from mix juice.

 If not, I'm going to press forward with the idea of putting the
 relevant logic in either add_paths_to_joinrel(), as previously
 proposed, or perhaps up oe level in make_one_rel().  Either way, if
 you don't need to be called multiple times per joinrel, you can stash
 a flag inside whatever you hang off of the joinrel's fdw_private and
 return immediately on every call after the first.  I think that's
 cheap enough that we shouldn't get too stressed about it: for FDWs, we
 only call the hook at all if everything in the joinrel uses the same
 FDW, so it won't get called at all except for joinrels where it's
 likely to win big; for custom joins, multiple calls are quite likely
 to be useful and necessary, and if the hook burns too much CPU time
 for the query performance you get out of it, that's the custom-join
 provider's fault, not ours.  The current patch takes this approach one
 step further and attempts FDW pushdown only once per joinrel.  It does
 that because, while postgres_fdw DOES need the jointype and a valid
 innerrel/outerrel breakdown to figure out what query to generate, it
 does NOT every possible breakdown; rather, the first one is as good as
 any other. But this might not be true for a non-PostgreSQL remote

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-04-20 Thread Shigeru HANADA

Kaigai-san,

I reviewed the Custom/Foreign join API patch again after writing a patch of 
join push-down support for postgres_fdw.

2015/03/26 10:51、Kouhei Kaigai kai...@ak.jp.nec.com のメール：

 Or bottom of make_join_rel().  IMO build_join_rel() is responsible for just
 building (or searching from a list) a RelOptInfo for given relids.  After 
 that
 make_join_rel() calls add_paths_to_joinrel() with appropriate arguments per 
 join
 type to generate actual Paths implements the join.  make_join_rel() is called
 only once for particular relid combination, and there SpecialJoinInfo and
 restrictlist (conditions specified in JOIN-ON and WHERE), so it seems 
 promising
 for FDW cases.
 
 I like that idea, but I think we will have complex hook signature, it won't
 remain as simple as hook (root, joinrel).
 
 Signature of the hook (or the FDW API handler) would be like this:
 
 typedef void (*GetForeignJoinPaths_function ) (PlannerInfo *root,
   RelOptInfo *joinrel,
   RelOptInfo *outerrel,
   RelOptInfo *innerrel,
   JoinType jointype,
   SpecialJoinInfo *sjinfo,
   List *restrictlist);
 
 This is very similar to add_paths_to_joinrel(), but lacks semifactors and
 extra_lateral_rels.  semifactors can be obtained with
 compute_semi_anti_join_factors(), and extra_lateral_rels can be constructed
 from root-placeholder_list as add_paths_to_joinrel() does.
 
 From the viewpoint of postgres_fdw, jointype and restrictlist is necessary to
 generate SELECT statement, so it would require most work done in 
 make_join_rel
 again if the signature was hook(root, joinrel).  sjinfo will be necessary for
 supporting SEMI/ANTI joins, but currently it is not in the scope of 
 postgres_fdw.
 
 I guess that other FDWs require at least jointype and restrictlist.
 
 The attached patch adds GetForeignJoinPaths call on make_join_rel() only when
 'joinrel' is actually built and both of child relations are managed by same
 FDW driver, prior to any other built-in join paths.
 I adjusted the hook definition a little bit, because jointype can be 
 reproduced
 using SpecialJoinInfo. Right?

Yes, it can be derived from the expression below:
jointype = sjinfo ? sjinfo-jointype : JOIN_INNER;

 
 Probably, it will solve the original concern towards multiple calls of FDW
 handler in case when it tries to replace an entire join subtree with a 
 foreign-
 scan on the result of remote join query.
 
 How about your opinion?

AFAIS it’s well-balanced about calling count and available information.

New FDW API GetForeignJoinPaths is called only once for a particular 
combination of join, such as (A join B join C).  Before considering all joins 
in a join level (number of relations contained in the join tree), possible join 
combinations of lower join level are considered recursively. As Tom pointed out 
before, say imagine a case like ((huge JOIN large) LEFT JOIN small), expensive 
path in lower join level might be 

Here, please let me summarize the changes in the patch as the result of my 
review.

* Add set_join_pathlist_hook_type in add_paths_to_joinrel
This hook is intended to provide a chance to add one or more CustomPaths for an 
actual join combination.  If the join is reversible, the hook is called for 
both A * B and B * A.  This is different from FDW API but it seems fine because 
FDWs should have chances to process the join in more abstract level than CSPs.

Parameters are same as hash_inner_and_outer, so they would be enough for 
hash-like or nestloop-like methods.  I’m not sure whether mergeclause_list is 
necessary as a parameter or not.  It’s information for merge join which is 
generated when enable_mergejoin is on and the join is not FULL OUTER.  Does 
some CSP need it for processing a join in its own way?  Then it must be in 
parameter list because select_mergejoin_clauses is static so it’s not 
accessible from external modules.

The timing of the hooking, after considering all built-in path types, seems 
fine because some of CSPs might want to use built-in paths as a template or a 
source.

One concern is in the document of the hook function.  Implementing Custom 
Paths” says:

 A custom scan provider will be also able to add paths by setting the 
 following hook, to replace built-in join paths by custom-scan that performs 
 as if a scan on preliminary joined relations, which us called after the core 
 code has generated what it believes to be the complete and correct set of 
 access paths for the join.

I think “replace” would mis-lead readers that CSP can remove or edit existing 
built-in paths listed in RelOptInfo#pathlist or linked from cheapest_foo.  IIUC 
CSP can just add paths for the join relation, and planner choose it if it’s the 
cheapest.

* Add new FDW API

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-04-17 Thread Kouhei Kaigai

Hanada-san,

Thanks for your works. I have nothing to comment on any more (at this moment).
I hope committer review / comment on the couple of features.

Best regards,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei kai...@ak.jp.nec.com


 -Original Message-
 From: Shigeru HANADA [mailto:shigeru.han...@gmail.com]
 Sent: Friday, April 17, 2015 1:44 PM
 To: Kaigai Kouhei(海外 浩平)
 Cc: Ashutosh Bapat; Robert Haas; Tom Lane; Thom Brown;
 pgsql-hackers@postgreSQL.org
 Subject: ##freemail## Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] 
 Custom
 Plan API)
 
 Kaigai-san,
 
 2015/04/17 10:13、Kouhei Kaigai kai...@ak.jp.nec.com のメール：
 
  Hanada-san,
 
  I merged explain patch into foreign_join patch.
 
  Now v12 is the latest patch.
 
  It contains many garbage lines... Please ensure the
  patch is correctly based on tOhe latest master +
  custom_join patch.
 
 Oops, sorry.  I’ve re-created the patch as v13, based on Custom/Foreign join 
 v11
 patch and latest master.
 
 It contains EXPLAIN enhancement that new subitem “Relations” shows relations
 and joins, including order and type, processed by the foreign scan.
 
 --
 Shigeru HANADA
 shigeru.han...@gmail.com
 
 
 
 


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-04-16 Thread Kouhei Kaigai

Hanada-san,

 I merged explain patch into foreign_join patch.
 
 Now v12 is the latest patch.

It contains many garbage lines... Please ensure the
patch is correctly based on the latest master +
custom_join patch.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei kai...@ak.jp.nec.com


 -Original Message-
 From: Shigeru HANADA [mailto:shigeru.han...@gmail.com]
 Sent: Thursday, April 16, 2015 5:06 PM
 To: Kaigai Kouhei(海外 浩平)
 Cc: Ashutosh Bapat; Robert Haas; Tom Lane; Thom Brown;
 pgsql-hackers@postgreSQL.org
 Subject: ##freemail## Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] 
 Custom
 Plan API)
 
 Kaigai-san,
 
 2015/04/15 22:33、Kouhei Kaigai kai...@ak.jp.nec.com のメール：
  Oops, that’s right.  Attached is the revised version.  I chose fully 
  qualified
  name, schema.relname [alias] for the output.  It would waste some cycles 
  during
  planning if that is not for EXPLAIN, but it seems difficult to get a list 
  of
 name
  of relations in ExplainForeignScan() phase, because planning information 
  has
 gone
  away at that time.
 
  I understand. Private data structure of the postgres_fdw is not designed
  to keep tree structure data (like relations join tree), so it seems to me
  a straightforward way to implement the feature.
 
  I have a small suggestion. This patch makes deparseSelectSql initialize
  the StringInfoData if supplied, however, it usually shall be a task of
  function caller, not callee.
  In this case, I like to initStringInfo(relations) next to the line of
  initStingInfo(sql) on the postgresGetForeignPlan. In my sense, it is
  a bit strange to pass uninitialized StringInfoData, to get a text form.
 
  @@ -803,7 +806,7 @@ postgresGetForeignPlan(PlannerInfo *root,
  */
 initStringInfo(sql);
 deparseSelectSql(sql, root, baserel, fpinfo-attrs_used, remote_conds,
  -params_list, fdw_ps_tlist, retrieved_attrs);
  +params_list, fdw_ps_tlist, retrieved_attrs,
 relations);
 
 /*
  * Build the fdw_private list that will be available in the executor.
 
 
 Agreed.  If caller passes a buffer, it should be initialized by caller.  In
 addition to your idea, I added a check that the RelOptInfo is a JOINREL, coz 
 BASEREL
 doesn’t need relations for its EXPLAIN output.
 
  Also, could you merge the EXPLAIN output feature on the main patch?
  I think here is no reason why to split this feature.
 
 I merged explain patch into foreign_join patch.
 
 Now v12 is the latest patch.
 
 --
 Shigeru HANADA
 shigeru.han...@gmail.com
 
 
 
 


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-04-15 Thread Kouhei Kaigai

  Attached explain_forein_join.patch adds capability to show join combination
 of
  a ForeignScan in EXPLAIN output as an additional item “Relations”.  I 
  thought
  that using array to list relations is a good way too, but I chose one 
  string
 value
  because users would like to know order and type of joins too.
 
  A bit different from my expectation... I expected to display name of the 
  local
  foreign tables (and its alias), not remote one, because all the local join 
  logic
  displays local foreign tables name.
  Is it easy to adjust isn't it? Probably, all you need to do is, putting a 
  local
  relation name on the text buffer (at deparseSelectSql) instead of the 
  deparsed
  remote relation.
 
 Oops, that’s right.  Attached is the revised version.  I chose fully qualified
 name, schema.relname [alias] for the output.  It would waste some cycles 
 during
 planning if that is not for EXPLAIN, but it seems difficult to get a list of 
 name
 of relations in ExplainForeignScan() phase, because planning information has 
 gone
 away at that time.

I understand. Private data structure of the postgres_fdw is not designed
to keep tree structure data (like relations join tree), so it seems to me
a straightforward way to implement the feature.

I have a small suggestion. This patch makes deparseSelectSql initialize
the StringInfoData if supplied, however, it usually shall be a task of
function caller, not callee.
In this case, I like to initStringInfo(relations) next to the line of
initStingInfo(sql) on the postgresGetForeignPlan. In my sense, it is
a bit strange to pass uninitialized StringInfoData, to get a text form.

@@ -803,7 +806,7 @@ postgresGetForeignPlan(PlannerInfo *root,
 */
initStringInfo(sql);
deparseSelectSql(sql, root, baserel, fpinfo-attrs_used, remote_conds,
-params_list, fdw_ps_tlist, retrieved_attrs);
+params_list, fdw_ps_tlist, retrieved_attrs, relations);

/*
 * Build the fdw_private list that will be available in the executor.

Also, could you merge the EXPLAIN output feature on the main patch?
I think here is no reason why to split this feature.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei kai...@ak.jp.nec.com


 -Original Message-
 From: pgsql-hackers-ow...@postgresql.org
 [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Shigeru HANADA
 Sent: Tuesday, April 14, 2015 7:49 PM
 To: Kaigai Kouhei(海外 浩平)
 Cc: Ashutosh Bapat; Robert Haas; Tom Lane; Thom Brown;
 pgsql-hackers@postgreSQL.org
 Subject: Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
 
 KaiGai-san,
 
 2015/04/14 14:04、Kouhei Kaigai kai...@ak.jp.nec.com のメール：
 
  * Fix typos
 
  Please review the v11 patch, and mark it as “ready for committer” if it’s
 ok.
 
  It's OK for me, and wants to be reviewed by other people to get it 
  committed.
 
 
 Thanks!
 
  In addition to essential features, I tried to implement relation listing in
 EXPLAIN
  output.
 
  Attached explain_forein_join.patch adds capability to show join combination
 of
  a ForeignScan in EXPLAIN output as an additional item “Relations”.  I 
  thought
  that using array to list relations is a good way too, but I chose one 
  string
 value
  because users would like to know order and type of joins too.
 
  A bit different from my expectation... I expected to display name of the 
  local
  foreign tables (and its alias), not remote one, because all the local join 
  logic
  displays local foreign tables name.
  Is it easy to adjust isn't it? Probably, all you need to do is, putting a 
  local
  relation name on the text buffer (at deparseSelectSql) instead of the 
  deparsed
  remote relation.
 
 Oops, that’s right.  Attached is the revised version.  I chose fully qualified
 name, schema.relname [alias] for the output.  It would waste some cycles 
 during
 planning if that is not for EXPLAIN, but it seems difficult to get a list of 
 name
 of relations in ExplainForeignScan() phase, because planning information has 
 gone
 away at that time.
 
 
 
 --
 Shigeru HANADA
 shigeru.han...@gmail.com
 
 
 --
 Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
 To make changes to your subscription:
 http://www.postgresql.org/mailpref/pgsql-hackers


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-04-14 Thread Shigeru HANADA

KaiGai-san,

2015/04/14 14:04、Kouhei Kaigai kai...@ak.jp.nec.com のメール：
 
 * Fix typos
 
 Please review the v11 patch, and mark it as “ready for committer” if it’s ok.
 
 It's OK for me, and wants to be reviewed by other people to get it committed.
 

Thanks!

 In addition to essential features, I tried to implement relation listing in 
 EXPLAIN
 output.
 
 Attached explain_forein_join.patch adds capability to show join combination 
 of
 a ForeignScan in EXPLAIN output as an additional item “Relations”.  I thought
 that using array to list relations is a good way too, but I chose one string 
 value
 because users would like to know order and type of joins too.
 
 A bit different from my expectation... I expected to display name of the local
 foreign tables (and its alias), not remote one, because all the local join 
 logic
 displays local foreign tables name.
 Is it easy to adjust isn't it? Probably, all you need to do is, putting a 
 local
 relation name on the text buffer (at deparseSelectSql) instead of the deparsed
 remote relation.

Oops, that’s right.  Attached is the revised version.  I chose fully qualified 
name, schema.relname [alias] for the output.  It would waste some cycles during 
planning if that is not for EXPLAIN, but it seems difficult to get a list of 
name of relations in ExplainForeignScan() phase, because planning information 
has gone away at that time.



--
Shigeru HANADA
shigeru.han...@gmail.com


explain_foreign_join_v2.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-04-13 Thread Kouhei Kaigai

Hanada-san,

 Thanks for further review, but I found two bugs in v10 patch.
 I’ve fixed them and wrapped up v11 patch here.
 
 * Fix bug about illegal column order
 
 Scan against a base relation returns columns in order of column definition, 
 but
 its target list might require different order.  This can be resolved by tuple
 projection in usual cases, but pushing down joins skips the step, so we need 
 to
 treat it in remote query.
 
 Before this fix, deparseProjectionSql() was called only for queries which have
 ctid or whole-row reference in its target list, but it was a too-much 
 optimization.
 We always need to call it, because usual column list might require ordering
 conversion.  Checking ordering is not impossible, but it seems useless effort.
 
 Another way to resolve this issue is to reorder SELECT clause of a query for 
 base
 relation if it was a source of a join, but it requires stepping back in 
 planning,
 so the fix above was chosen.
 
 three tables join test case is also changed to check this behavior.

Sorry for my oversight. Yep, var-node reference on join relation cannot
expect any column orders predefined like as base relations.
All reasonable way I know is, relying on the targetlist of the RelOptInfo
that contains all the referenced columns in the later stage.

 * Fix bug of duplicate fdw_ps_tlist contents.
 
 I coded that deparseSelectSql passes fdw_ps_tlist to deparseSelectSql for
 underlying RelOptInfo, but it causes redundant entries in fdw_ps_tlist in 
 cases
 of joining more than two foreign tables. I changed to pass NULL as 
 fdw_ps_tlist
 for recursive call  of deparseSelectSql.

It's reasonable, and also makes performance benefit because descriptor
constructed based on the ps_tlist will match expected result tuple, so
it allows to avoid unnecessary projection.

 * Fix typos
 
 Please review the v11 patch, and mark it as “ready for committer” if it’s ok.
 
It's OK for me, and wants to be reviewed by other people to get it committed.

 In addition to essential features, I tried to implement relation listing in 
 EXPLAIN
 output.
 
 Attached explain_forein_join.patch adds capability to show join combination of
 a ForeignScan in EXPLAIN output as an additional item “Relations”.  I thought
 that using array to list relations is a good way too, but I chose one string 
 value
 because users would like to know order and type of joins too.

A bit different from my expectation... I expected to display name of the local
foreign tables (and its alias), not remote one, because all the local join logic
displays local foreign tables name.
Is it easy to adjust isn't it? Probably, all you need to do is, putting a local
relation name on the text buffer (at deparseSelectSql) instead of the deparsed
remote relation.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei kai...@ak.jp.nec.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-04-09 Thread Kouhei Kaigai

 2015/04/09 10:48、Kouhei Kaigai kai...@ak.jp.nec.com のメール：
 * merge_fpinfo()
  It seems to me fpinfo-rows should be joinrel-rows, and
  fpinfo-width also should be joinrel-width.
  No need to have special intelligence here, isn't it?
 
 
  Oops. They are vestige of my struggle which disabled SELECT clause 
  optimization
  (omit unused columns).  Now width and rows are inherited from joinrel.
 Besides
  that, fdw_startup_cost and fdw_tuple_cost seem wrong, so I fixed them to 
  use
 simple
  summary, not average.
 
  Does fpinfo-fdw_startup_cost represent a cost to open connection to remote
  PostgreSQL, doesn't it?
 
  postgres_fdw.c:1757 says as follows:
 
 /*
  * Add some additional cost factors to account for connection overhead
  * (fdw_startup_cost), transferring data across the network
  * (fdw_tuple_cost per retrieved row), and local manipulation of the data
  * (cpu_tuple_cost per retrieved row).
  */
 
  If so, does a ForeignScan that involves 100 underlying relation takes 100
  times heavy network operations on startup? Probably, no.
  I think, average is better than sum, and max of them will reflect the cost
  more correctly.
 
 In my current opinion, no. Though I remember that I've written such comments
 before :P.
 
 Connection establishment occurs only once for the very first access to the 
 server,
 so in the use cases with long-lived session (via psql, connection pooling, 
 etc.),
 taking connection overhead into account *every time* seems too pessimistic.
 
 Instead, for practical cases, fdw_startup_cost should consider overheads of 
 query
 construction and getting first response of it (hopefully it minus retrieving
 actual data).  These overheads are visible in the order of milliseconds.  I’m
 not sure how much is appropriate for the default, but 100 seems not so bad.
 
 Anyway fdw_startup_cost is per-server setting as same as fdw_tuple_cost, and 
 it
 should not be modified according to the width of the result, so using
 fpinfo_o-fdw_startup_cost would be ok.

Indeed, I forgot the connection cache mechanism. As long as we define
fdw_startup_cost as you mentioned, it seems to me your logic is heuristically
reasonable.

  Also, fdw_tuple_cost introduce the cost of data transfer over the network.
  I thinks, weighted average is the best strategy, like:
   fpinfo-fdw_tuple_cost =
 (fpinfo_o-width / (fpinfo_o-width + fpinfo_i-width) *
 fpinfo_o-fdw_tuple_cost +
 (fpinfo_i-width / (fpinfo_o-width + fpinfo_i-width) *
 fpinfo_i-fdw_tuple_cost;
 
  That's just my suggestion. Please apply the best way you thought.
 
 I can’t agree that strategy, because 1) width 0 causes per-tuple cost 0, and 
 2)
 fdw_tuple_cost never vary in a foreign server.  Using fpinfo_o-fdw_tuple_cost
 (it must be identical to fpinfo_i-fdw_tuple_cost) seems reasonable.  
 Thoughts?

OK, you are right.

I think it is time to hand over the patch reviewing to committers.
So, let me mark it ready for committers.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei kai...@ak.jp.nec.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-04-08 Thread Kouhei Kaigai

Hanada-san,

 In addition to your comments, I removed useless code that retrieves 
 ForeignPath
 from outer/inner RelOptInfo and store them into ForeignPath#fdw_private.  Now
 postgres_fdw’s join pushd-down is free from existence of ForeignPath under the
 join relation.  This means that we can support the case Robert mentioned 
 before,
 that whole (huge JOIN large) JOIN small” can be pushed down even if “(huge 
 JOIN
 large)” is dominated by another join path.

Yes, it's definitely reasonable design, and fits intention of the interface.
I should point out it from the beginning. :-)

  l of the first SELECT represents a whole-row reference.
  However, underlying SELECT contains system columns in its target-
  list.
 
  Is it available to construct such a query?
   SELECT l.a1, l.a2 FROM (SELECT (id,a,atext), ctid) l (a1, a2) ...
   ^^
 
 Simple relation reference such as l is not sufficient for the purpose, yes.
 But putting columns in parentheses would not work when a user column is 
 referenced
 in original query.
 
 I implemented deparseProjectionSql to use ROW(...) expression for a whole-row
 reference in the target list, in addition to ordinary column references for
 actually used columns and ctid.
 
 Please see the test case for mixed use of ctid and whole-row reference to
 postgres_fdw’s regression tests.  Now a whole-row reference in the remote 
 query
 looks like this:
 
It seems to me that deparseProjectionSql() works properly.

 -- ctid with whole-row reference
 EXPLAIN (COSTS false, VERBOSE)
 SELECT t1.ctid, t1, t2 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY 
 t1.c3,
 t1.c1 OFFSET 100 LIMIT 10;
 
 
 
 -
  Limit
Output: t1.ctid, t1.*, t2.*, t1.c3, t1.c1
-  Sort
  Output: t1.ctid, t1.*, t2.*, t1.c3, t1.c1
  Sort Key: t1.c3, t1.c1
  -  Foreign Scan
Output: t1.ctid, t1.*, t2.*, t1.c3, t1.c1
Remote SQL: SELECT l.a1, l.a2, l.a3, l.a4, r.a1 FROM (SELECT 
 l.a7,
 ROW(l.a10, l.a11, l.a12, l.a13, l.a14, l.a15, l.a16, l.a17), l.a12, l.a10 FROM
 (SELECT C 1 a10, c2 a11, c3 a12, c4 a13, c5 a14, c6 a15, c7 a1
 (8 rows)
 
 In fact l.a12 and l.a10, for t1.c3 and t1.c1, are redundant in transferred 
 data,
 but IMO this would simplify the code for deparsing.

I agree. Even if we can reduce networking cost a little, tuple
construction takes CPU cycles. Your decision is fair enough for
me.

  * merge_fpinfo()
  It seems to me fpinfo-rows should be joinrel-rows, and
  fpinfo-width also should be joinrel-width.
  No need to have special intelligence here, isn't it?
 
 
 Oops. They are vestige of my struggle which disabled SELECT clause 
 optimization
 (omit unused columns).  Now width and rows are inherited from joinrel.  
 Besides
 that, fdw_startup_cost and fdw_tuple_cost seem wrong, so I fixed them to use 
 simple
 summary, not average.

Does fpinfo-fdw_startup_cost represent a cost to open connection to remote
PostgreSQL, doesn't it?

postgres_fdw.c:1757 says as follows:

/*
 * Add some additional cost factors to account for connection overhead
 * (fdw_startup_cost), transferring data across the network
 * (fdw_tuple_cost per retrieved row), and local manipulation of the data
 * (cpu_tuple_cost per retrieved row).
 */

If so, does a ForeignScan that involves 100 underlying relation takes 100
times heavy network operations on startup? Probably, no.
I think, average is better than sum, and max of them will reflect the cost
more correctly.
Also, fdw_tuple_cost introduce the cost of data transfer over the network.
I thinks, weighted average is the best strategy, like:
  fpinfo-fdw_tuple_cost =
(fpinfo_o-width / (fpinfo_o-width + fpinfo_i-width) * 
fpinfo_o-fdw_tuple_cost +
(fpinfo_i-width / (fpinfo_o-width + fpinfo_i-width) * 
fpinfo_i-fdw_tuple_cost;

That's just my suggestion. Please apply the best way you thought.

  * explain output
 
  EXPLAIN output may be a bit insufficient to know what does it
  actually try to do.
 
  postgres=# explain select * from ft1,ft2 where a = b;
QUERY PLAN
  
  Foreign Scan  (cost=200.00..212.80 rows=1280 width=80)
  (1 row)
 
  Even though it is not an immediate request, it seems to me
  better to show users joined relations and remote ON/WHERE
  clause here.
 
 
 Like this?
 
 Foreign Scan on ft1 INNER JOIN ft2 ON ft1.a = ft2.b  (cost=200.00..212.80
 rows=1280 width=80)
 …

No. This line is produced by ExplainScanTarget(), so it requires the
backend knowledge to individual FDW.
Rather than the backend, postgresExplainForeignScan() shall produce
the output.

 It might produce a very long line in a case of joining many tables because it

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-04-06 Thread Kouhei Kaigai

Hanada-san,

Thanks for your dedicated efforts for remote-join feature.
Below are the comments from my side.


* Bug - mixture of ctid system column and whole row-reference
In case when ctid system column is required, deparseSelectSql()
adds ctid reference on the base relation scan level.
On the other hands, whole-row reference is transformed to
a reference to the underlying relation. It will work fine if
system column is not specified. However, system column reference
breaks tuple layout from the expected one.
Eventually it leads an error.

postgres=# select ft1.ctid,ft1 from ft1,ft2 where a=b;
ERROR:  malformed record literal: (2,2,bbb,(0,2))
DETAIL:  Too many columns.
CONTEXT:  column  of foreign table foreign join
STATEMENT:  select ft1.ctid,ft1 from ft1,ft2 where a=b;

postgres=# explain verbose
   select ft1.ctid,ft1 from ft1,ft2 where a=b;
   QUERY PLAN

 Foreign Scan  (cost=200.00..208.35 rows=835 width=70)
   Output: ft1.ctid, ft1.*
   Remote SQL: SELECT l.a1, l.a2 FROM (SELECT l.a7, l, l.a10 FROM (SELECT id a9,
 a a10, atext a11, ctid a7 FROM public.t1) l) l (a1, a2, a3) INNER JOIN (SELECT
b a10 FROM public.t2) r (a1) ON ((l.a3 = r.a1))

l of the first SELECT represents a whole-row reference.
However, underlying SELECT contains system columns in its target-
list.

Is it available to construct such a query?
  SELECT l.a1, l.a2 FROM (SELECT (id,a,atext), ctid) l (a1, a2) ...
  ^^
Probably, it is a job of deparseProjectionSql().


* postgresGetForeignJoinPaths()
It walks on the root-simple_rel_array to check whether
all the relations involved are manged by same foreign
server with same credential.
We may have more graceful way for this. Pay attention on
the fdw_private field of innerrel/outerrel. If they have
a valid fdw_private, it means FDW driver (postgres_fdw)
considers all the underlying relations scan/join are
available to run the remote-server.
So, all we need to check is whether server-id and user-id
of both relations are identical or not.


* merge_fpinfo()
It seems to me fpinfo-rows should be joinrel-rows, and
fpinfo-width also should be joinrel-width.
No need to have special intelligence here, isn't it?


* explain output

EXPLAIN output may be a bit insufficient to know what does it
actually try to do.

postgres=# explain select * from ft1,ft2 where a = b;
   QUERY PLAN

 Foreign Scan  (cost=200.00..212.80 rows=1280 width=80)
(1 row)

Even though it is not an immediate request, it seems to me
better to show users joined relations and remote ON/WHERE
clause here.


Please don't hesitate to consult me, if you have any questions.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei kai...@ak.jp.nec.com


 -Original Message-
 From: pgsql-hackers-ow...@postgresql.org
 [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Shigeru Hanada
 Sent: Friday, April 03, 2015 7:32 PM
 To: Kaigai Kouhei(海外 浩平)
 Cc: Ashutosh Bapat; Robert Haas; Tom Lane; Thom Brown;
 pgsql-hackers@postgreSQL.org
 Subject: Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
 
 Attached is the patch which adds join push-down support to postgres_fdw (v7).
 It supports SELECT statements with JOIN, but has some more possible 
 enhancements
 (see below).  I'd like to share the WIP patch here to get comments about new 
 FDW
 API design provided by KaiGai-san's v11 patch.
 
 To make reviewing easier, I summarized changes against Custom/Foreign join v11
 patch.
 
 Changes for Join push-down support
 ==
 - Add FDW API GetForeignJoinPaths().  It generates a ForeignPath which 
 represents
 a scan against pseudo join relation represented by given RelOptInfo.
 - Expand deparsing module to handle multi-relation queries.  Steps of 
 deparsing
 a join query:
 
 1) Optimizer calls postgresGetForeignPaths() for each BASEREL.  Here
 postgres_fdw does the same things as before, except adding column aliases in 
 SELECT
 clause.
 2) Optimizer calls postgresGetForeignJoinPaths() for each JOINREL.  Optimizer
 calls once per RelOptInfo with reloptkind == RELOPT_JOINREL, so postgres_fdw
 should consider both A JOIN B and B JOIN A in one call.
 
 postgres_fdw checks whether the join can be pushed down.
 
 a) Both outer and inner relations can be pushed down (NULL in
 RelOptInfo#fdw_private indicates such situation)
 b) Outmost command is a SELECT (this can be relaxed in the future)
 c) Join type is inner or one of outer
 d) Server of all relations in the join are identical
 e) Effective user id for all relations in the join are identical (they might 
 be
 different some were accessed via views)
 f) No local filters (this can be relaxed if inner  non-volatile)
 g) Join conditions doesn't contain any unsafe expression
 h) Remote filter doesn't contain any

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)


2015/03/25 12:59、Kouhei Kaigai kai...@ak.jp.nec.com のメール：

 At this moment, I'm not 100% certain about its logic. Especially, I didn't
 test SEMI- and ANTI- join cases yet.
 However, time is money - I want people to check overall design first, rather
 than detailed debugging. Please tell me if I misunderstood the logic to 
 break
 down join relations.
 
 With applying your patch, regression tests of “updatable view” failed.
 regression.diff contains some errors like this:
 ! ERROR:  could not find RelOptInfo for given relids
 
 Could you check that?
 
 It is a bug around the logic to find out two RelOptInfo that can construct
 another RelOptInfo of joinrel.
 Even though I'm now working to correct the logic, it is not obvious to
 identify two relids that satisfy joinrel-relids.
 (Yep, law of entropy enhancement…)

IIUC, this problem is in only non-INNER JOINs because we can treat relations 
joined with only INNER JOIN in arbitrary order.  But supporting OUTER JOINs 
would be necessary even for the first cut.

 On the other hands, we may have a solution that does not need a complicated
 reconstruction process. The original concern was, FDW driver may add paths
 that will replace entire join subtree by foreign-scan on remote join multiple
 times, repeatedly, but these paths shall be identical.
 
 If we put a hook for FDW/CSP on bottom of build_join_rel(), we may be able
 to solve the problem more straight-forward and simply way.
 Because build_join_rel() finds a cache on root-join_rel_hash then returns
 immediately if required joinrelids already has its RelOptInfo, bottom of
 this function never called twice on a particular set of joinrelids.
 Once FDW/CSP constructs a path that replaces entire join subtree towards
 the joinrel just after construction, it shall be kept until cheaper built-in
 paths are added (if exists).
 
 This idea has one other positive side-effect. We expect remote-join is cheaper
 than local join with two remote scan in most cases. Once a much cheaper path
 is added prior to local join consideration, add_path_precheck() breaks path
 consideration earlier.
 
 Please comment on.

Or bottom of make_join_rel().  IMO build_join_rel() is responsible for just 
building (or searching from a list) a RelOptInfo for given relids.  After that 
make_join_rel() calls add_paths_to_joinrel() with appropriate arguments per 
join type to generate actual Paths implements the join.  make_join_rel() is 
called only once for particular relid combination, and there SpecialJoinInfo 
and restrictlist (conditions specified in JOIN-ON and WHERE), so it seems 
promising for FDW cases.

Though I’m not sure that it also fits custom join provider’s requirements.

—
Shigeru HANADA

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

2015-03-25 Thread Ashutosh Bapat

On Wed, Mar 25, 2015 at 3:14 PM, Shigeru HANADA shigeru.han...@gmail.com
wrote:


 Or bottom of make_join_rel().  IMO build_join_rel() is responsible for
 just building (or searching from a list) a RelOptInfo for given relids.
 After that make_join_rel() calls add_paths_to_joinrel() with appropriate
 arguments per join type to generate actual Paths implements the join.
 make_join_rel() is called only once for particular relid combination, and
 there SpecialJoinInfo and restrictlist (conditions specified in JOIN-ON and
 WHERE), so it seems promising for FDW cases.


I like that idea, but I think we will have complex hook signature, it won't
remain as simple as hook (root, joinrel).

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

 On Wed, Mar 25, 2015 at 3:14 PM, Shigeru HANADA shigeru.han...@gmail.com 
 wrote:
   Or bottom of make_join_rel().  IMO build_join_rel() is responsible for
 just building (or searching from a list) a RelOptInfo for given relids.  After
 that make_join_rel() calls add_paths_to_joinrel() with appropriate arguments 
 per
 join type to generate actual Paths implements the join.  make_join_rel() is
 called only once for particular relid combination, and there SpecialJoinInfo 
 and
 restrictlist (conditions specified in JOIN-ON and WHERE), so it seems 
 promising
 for FDW cases.
 
 
 
 I like that idea, but I think we will have complex hook signature, it won't 
 remain
 as simple as hook (root, joinrel).
 
In this case, GetForeignJoinPaths() will take root, joinrel, rel1, rel2,
sjinfo and restrictlist.
It is not too simple, but not complicated signature.

Even if we reconstruct rel1 and rel2 using sjinfo, we also need to compute
restrictlist using build_joinrel_restrictlist() again. It is a static function
in relnode.c. So, I don't think either of them has definitive advantage from
the standpoint of simplicity.

Thanks,
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei kai...@ak.jp.nec.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)


2015/03/25 19:09、Kouhei Kaigai kai...@ak.jp.nec.com のメール：

 On Wed, Mar 25, 2015 at 3:14 PM, Shigeru HANADA shigeru.han...@gmail.com 
 wrote:
  Or bottom of make_join_rel().  IMO build_join_rel() is responsible for
 just building (or searching from a list) a RelOptInfo for given relids.  
 After
 that make_join_rel() calls add_paths_to_joinrel() with appropriate arguments 
 per
 join type to generate actual Paths implements the join.  make_join_rel() is
 called only once for particular relid combination, and there SpecialJoinInfo 
 and
 restrictlist (conditions specified in JOIN-ON and WHERE), so it seems 
 promising
 for FDW cases.
 
 
 
 I like that idea, but I think we will have complex hook signature, it won't 
 remain
 as simple as hook (root, joinrel).
 
 In this case, GetForeignJoinPaths() will take root, joinrel, rel1, rel2,
 sjinfo and restrictlist.
 It is not too simple, but not complicated signature.
 
 Even if we reconstruct rel1 and rel2 using sjinfo, we also need to compute
 restrictlist using build_joinrel_restrictlist() again. It is a static function
 in relnode.c. So, I don't think either of them has definitive advantage from
 the standpoint of simplicity.

The bottom of make_join_rel() seems good from the viewpoint of information, but 
it is called multiple times for join combinations which are essentially 
identical, for INNER JOIN case like this:

fdw=# explain select * from pgbench_branches b join pgbench_tellers t on t.bid 
= b.bid join pgbench_accounts a on a.bid = b.bid and a.bid = t.bid;
INFO:  postgresGetForeignJoinPaths() 1x2
INFO:  postgresGetForeignJoinPaths() 1x4
INFO:  postgresGetForeignJoinPaths() 2x4
INFO:  standard_join_search() old hook point
INFO:  standard_join_search() old hook point
INFO:  standard_join_search() old hook point
INFO:  postgresGetForeignJoinPaths() 0x4
INFO:  postgresGetForeignJoinPaths() 0x2
INFO:  postgresGetForeignJoinPaths() 0x1
INFO:  standard_join_search() old hook point
   QUERY PLAN
-
 Foreign Scan  (cost=100.00..102.11 rows=211 width=1068)
(1 row)

Here I’ve put probe point in the beginnig of GetForeignJoinPaths handler and 
just before set_cheapest() call in standard_join_search() as “old hook point”.  
In this example 1, 2, and 4 are base relations, and in the join level 3 planner 
calls GetForeignJoinPaths() three times for the combinations:

1) (1x2)x4
2) (1x4)x2
3) (2x4)x1

Tom’s suggestion is aiming at providing a chance to consider join push-down in 
more abstract level, IIUC.  So it would be good to call handler only once for 
that case, for flattened combination (1x2x3).

Hum, how about skipping calling handler (or hook) if the joinrel was found by 
find_join_rel()?  At least it suppress redundant call for different join 
orders, and handler can determine whether the combination can be flattened by 
checking that all RelOptInfo with RELOPT_JOINREL under joinrel has JOIN_INNER 
as jointype.

—
Shigeru HANADA

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

 2015/03/25 12:59、Kouhei Kaigai kai...@ak.jp.nec.com のメール：
 
  At this moment, I'm not 100% certain about its logic. Especially, I didn't
  test SEMI- and ANTI- join cases yet.
  However, time is money - I want people to check overall design first, 
  rather
  than detailed debugging. Please tell me if I misunderstood the logic to 
  break
  down join relations.
 
  With applying your patch, regression tests of “updatable view” failed.
  regression.diff contains some errors like this:
  ! ERROR:  could not find RelOptInfo for given relids
 
  Could you check that?
 
  It is a bug around the logic to find out two RelOptInfo that can construct
  another RelOptInfo of joinrel.
  Even though I'm now working to correct the logic, it is not obvious to
  identify two relids that satisfy joinrel-relids.
  (Yep, law of entropy enhancement…)
 
 IIUC, this problem is in only non-INNER JOINs because we can treat relations 
 joined
 with only INNER JOIN in arbitrary order.  But supporting OUTER JOINs would be
 necessary even for the first cut.
 
Yep. In case when joinrel contains all inner-joined relations managed by same
FDW driver, job of get_joinrel_broken_down() is quite simple.
However, people want to support outer-join also, doesn't it?

  On the other hands, we may have a solution that does not need a complicated
  reconstruction process. The original concern was, FDW driver may add paths
  that will replace entire join subtree by foreign-scan on remote join 
  multiple
  times, repeatedly, but these paths shall be identical.
 
  If we put a hook for FDW/CSP on bottom of build_join_rel(), we may be able
  to solve the problem more straight-forward and simply way.
  Because build_join_rel() finds a cache on root-join_rel_hash then returns
  immediately if required joinrelids already has its RelOptInfo, bottom of
  this function never called twice on a particular set of joinrelids.
  Once FDW/CSP constructs a path that replaces entire join subtree towards
  the joinrel just after construction, it shall be kept until cheaper built-in
  paths are added (if exists).
 
  This idea has one other positive side-effect. We expect remote-join is 
  cheaper
  than local join with two remote scan in most cases. Once a much cheaper path
  is added prior to local join consideration, add_path_precheck() breaks path
  consideration earlier.
 
  Please comment on.
 
 Or bottom of make_join_rel().  IMO build_join_rel() is responsible for just
 building (or searching from a list) a RelOptInfo for given relids.  After that
 make_join_rel() calls add_paths_to_joinrel() with appropriate arguments per 
 join
 type to generate actual Paths implements the join.  make_join_rel() is called
 only once for particular relid combination, and there SpecialJoinInfo and
 restrictlist (conditions specified in JOIN-ON and WHERE), so it seems 
 promising
 for FDW cases.
 
As long as caller can know whether build_join_rel() actually construct a new
RelOptInfo object, or not, I think it makes more sense than putting a hook
within make_join_rel().

 Though I’m not sure that it also fits custom join provider’s requirements.

Join replaced by CSP has two scenarios. First one implements just an alternative
logic of built-in join, will takes underlying inner/outer node, so its hook
is located on add_paths_to_joinrel() as like built-in join logics.
Second one tries to replace entire join sub-tree by materialized view (for
example), like FDW remote join cases. So, it has to be hooked nearby the
location of GetForeignJoinPaths().
In case of the second scenario, CSP does not have private field in RelOptInfo,
so it may not obvious to check whether the given joinrel exactly matches with
a particular materialized-view or other caches.

At this moment, what I'm interested in is the first scenario, so priority of
the second case is not significant for me, at least.

Thanks.
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei kai...@ak.jp.nec.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

 2015/03/25 19:47、Kouhei Kaigai kai...@ak.jp.nec.com のメール：
  The reason why FDW handler was called multiple times on your example is,
  your modified make_join_rel() does not check whether build_join_rel()
  actually build a new RelOptInfo, or just a cache reference, doesn't it?
 
 
 Yep.  After that change calling count looks like this:
 
 fdw=# explain select * from pgbench_branches b join pgbench_tellers t on t.bid
 = b.bid join pgbench_accounts a on a.bid = b.bid and a.bid = t.bid;
 INFO:  postgresGetForeignJoinPaths() 1x2
 INFO:  postgresGetForeignJoinPaths() 1x4
 INFO:  postgresGetForeignJoinPaths() 2x4
 INFO:  standard_join_search() old hook point
 INFO:  standard_join_search() old hook point
 INFO:  standard_join_search() old hook point
 INFO:  postgresGetForeignJoinPaths() 0x4
 INFO:  standard_join_search() old hook point
QUERY PLAN
 -
  Foreign Scan  (cost=100.00..102.11 rows=211 width=1068)
 (1 row)
 
 fdw=#
 
  If so, I'm inclined to your proposition.
  A new bool *found argument of build_join_rel() makes reduce number of
  FDW handler call, with keeping reasonable information to build remote-
  join query.
 
 Another idea is to pass “found” as parameter to FDW handler, and let FDW to 
 decide
 to skip or not.  Some of FDWs (and some of CSP?) might want to be conscious of
 join combination.

I think it does not match the concept we stand on.
Unlike CSP, FDW intends to replace an entire join sub-tree that is
represented with a particular joinrel, regardless of the sequence
to construct a joinrel from multiple baserels.
So, it is sufficient to call GetForeignJoinPaths() once a joinrel
is constructed, isn't it?

Thanks,
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei kai...@ak.jp.nec.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

  Or bottom of make_join_rel().  IMO build_join_rel() is responsible for just
 building (or searching from a list) a RelOptInfo for given relids.  After that
 make_join_rel() calls add_paths_to_joinrel() with appropriate arguments per 
 join
 type to generate actual Paths implements the join.  make_join_rel() is called
 only once for particular relid combination, and there SpecialJoinInfo and
 restrictlist (conditions specified in JOIN-ON and WHERE), so it seems 
 promising
 for FDW cases.
 
  I like that idea, but I think we will have complex hook signature, it won't
 remain as simple as hook (root, joinrel).
 
 Signature of the hook (or the FDW API handler) would be like this:
 
 typedef void (*GetForeignJoinPaths_function ) (PlannerInfo *root,
RelOptInfo *joinrel,
RelOptInfo *outerrel,
RelOptInfo *innerrel,
JoinType jointype,
SpecialJoinInfo *sjinfo,
List *restrictlist);
 
 This is very similar to add_paths_to_joinrel(), but lacks semifactors and
 extra_lateral_rels.  semifactors can be obtained with
 compute_semi_anti_join_factors(), and extra_lateral_rels can be constructed
 from root-placeholder_list as add_paths_to_joinrel() does.
 
 From the viewpoint of postgres_fdw, jointype and restrictlist is necessary to
 generate SELECT statement, so it would require most work done in make_join_rel
 again if the signature was hook(root, joinrel).  sjinfo will be necessary for
 supporting SEMI/ANTI joins, but currently it is not in the scope of 
 postgres_fdw.
 
 I guess that other FDWs require at least jointype and restrictlist.

The attached patch adds GetForeignJoinPaths call on make_join_rel() only when
'joinrel' is actually built and both of child relations are managed by same
FDW driver, prior to any other built-in join paths.
I adjusted the hook definition a little bit, because jointype can be reproduced
using SpecialJoinInfo. Right?

Probably, it will solve the original concern towards multiple calls of FDW
handler in case when it tries to replace an entire join subtree with a foreign-
scan on the result of remote join query.

How about your opinion?

Thanks,
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei kai...@ak.jp.nec.com



pgsql-v9.5-custom-join.v11.patch
Description: pgsql-v9.5-custom-join.v11.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)


2015/03/25 18:53、Ashutosh Bapat ashutosh.ba...@enterprisedb.com のメール：

 
 
 On Wed, Mar 25, 2015 at 3:14 PM, Shigeru HANADA shigeru.han...@gmail.com 
 wrote:
 
 Or bottom of make_join_rel().  IMO build_join_rel() is responsible for just 
 building (or searching from a list) a RelOptInfo for given relids.  After 
 that make_join_rel() calls add_paths_to_joinrel() with appropriate arguments 
 per join type to generate actual Paths implements the join.  make_join_rel() 
 is called only once for particular relid combination, and there 
 SpecialJoinInfo and restrictlist (conditions specified in JOIN-ON and WHERE), 
 so it seems promising for FDW cases.
 
 I like that idea, but I think we will have complex hook signature, it won't 
 remain as simple as hook (root, joinrel). 

Signature of the hook (or the FDW API handler) would be like this:

typedef void (*GetForeignJoinPaths_function ) (PlannerInfo *root,
   RelOptInfo *joinrel,
   RelOptInfo *outerrel,
   RelOptInfo *innerrel,
   JoinType jointype,
   SpecialJoinInfo *sjinfo,
   List *restrictlist);

This is very similar to add_paths_to_joinrel(), but lacks semifactors and 
extra_lateral_rels.  semifactors can be obtained with 
compute_semi_anti_join_factors(), and extra_lateral_rels can be constructed 
from root-placeholder_list as add_paths_to_joinrel() does.

From the viewpoint of postgres_fdw, jointype and restrictlist is necessary to 
generate SELECT statement, so it would require most work done in make_join_rel 
again if the signature was hook(root, joinrel).  sjinfo will be necessary for 
supporting SEMI/ANTI joins, but currently it is not in the scope of 
postgres_fdw.

I guess that other FDWs require at least jointype and restrictlist.

—
Shigeru HANADA

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)


2015/03/25 19:47、Kouhei Kaigai kai...@ak.jp.nec.com のメール：
 The reason why FDW handler was called multiple times on your example is,
 your modified make_join_rel() does not check whether build_join_rel()
 actually build a new RelOptInfo, or just a cache reference, doesn't it?
 

Yep.  After that change calling count looks like this:

fdw=# explain select * from pgbench_branches b join pgbench_tellers t on t.bid 
= b.bid join pgbench_accounts a on a.bid = b.bid and a.bid = t.bid;
INFO:  postgresGetForeignJoinPaths() 1x2
INFO:  postgresGetForeignJoinPaths() 1x4
INFO:  postgresGetForeignJoinPaths() 2x4
INFO:  standard_join_search() old hook point
INFO:  standard_join_search() old hook point
INFO:  standard_join_search() old hook point
INFO:  postgresGetForeignJoinPaths() 0x4
INFO:  standard_join_search() old hook point
   QUERY PLAN
-
 Foreign Scan  (cost=100.00..102.11 rows=211 width=1068)
(1 row)

fdw=#

 If so, I'm inclined to your proposition.
 A new bool *found argument of build_join_rel() makes reduce number of
 FDW handler call, with keeping reasonable information to build remote-
 join query.

Another idea is to pass “found” as parameter to FDW handler, and let FDW to 
decide to skip or not.  Some of FDWs (and some of CSP?) might want to be 
conscious of join combination. 

—
Shigeru HANADA

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

 2015/03/25 19:09、Kouhei Kaigai kai...@ak.jp.nec.com のメール：
 
  On Wed, Mar 25, 2015 at 3:14 PM, Shigeru HANADA shigeru.han...@gmail.com
 wrote:
 Or bottom of make_join_rel().  IMO build_join_rel() is responsible for
  just building (or searching from a list) a RelOptInfo for given relids.  
  After
  that make_join_rel() calls add_paths_to_joinrel() with appropriate 
  arguments
 per
  join type to generate actual Paths implements the join.  make_join_rel() is
  called only once for particular relid combination, and there 
  SpecialJoinInfo
 and
  restrictlist (conditions specified in JOIN-ON and WHERE), so it seems 
  promising
  for FDW cases.
 
 
 
  I like that idea, but I think we will have complex hook signature, it won't
 remain
  as simple as hook (root, joinrel).
 
  In this case, GetForeignJoinPaths() will take root, joinrel, rel1, rel2,
  sjinfo and restrictlist.
  It is not too simple, but not complicated signature.
 
  Even if we reconstruct rel1 and rel2 using sjinfo, we also need to compute
  restrictlist using build_joinrel_restrictlist() again. It is a static 
  function
  in relnode.c. So, I don't think either of them has definitive advantage from
  the standpoint of simplicity.
 
 The bottom of make_join_rel() seems good from the viewpoint of information, 
 but
 it is called multiple times for join combinations which are essentially 
 identical,
 for INNER JOIN case like this:
 
 fdw=# explain select * from pgbench_branches b join pgbench_tellers t on t.bid
 = b.bid join pgbench_accounts a on a.bid = b.bid and a.bid = t.bid;
 INFO:  postgresGetForeignJoinPaths() 1x2
 INFO:  postgresGetForeignJoinPaths() 1x4
 INFO:  postgresGetForeignJoinPaths() 2x4
 INFO:  standard_join_search() old hook point
 INFO:  standard_join_search() old hook point
 INFO:  standard_join_search() old hook point
 INFO:  postgresGetForeignJoinPaths() 0x4
 INFO:  postgresGetForeignJoinPaths() 0x2
 INFO:  postgresGetForeignJoinPaths() 0x1
 INFO:  standard_join_search() old hook point
QUERY PLAN
 -
  Foreign Scan  (cost=100.00..102.11 rows=211 width=1068)
 (1 row)
 
 Here I’ve put probe point in the beginnig of GetForeignJoinPaths handler and 
 just
 before set_cheapest() call in standard_join_search() as “old hook point”.  In
 this example 1, 2, and 4 are base relations, and in the join level 3 planner 
 calls
 GetForeignJoinPaths() three times for the combinations:
 
 1) (1x2)x4
 2) (1x4)x2
 3) (2x4)x1
 
 Tom’s suggestion is aiming at providing a chance to consider join push-down in
 more abstract level, IIUC.  So it would be good to call handler only once for
 that case, for flattened combination (1x2x3).

 Hum, how about skipping calling handler (or hook) if the joinrel was found by
 find_join_rel()?  At least it suppress redundant call for different join 
 orders,
 and handler can determine whether the combination can be flattened by checking
 that all RelOptInfo with RELOPT_JOINREL under joinrel has JOIN_INNER as 
 jointype.
 
The reason why FDW handler was called multiple times on your example is,
your modified make_join_rel() does not check whether build_join_rel()
actually build a new RelOptInfo, or just a cache reference, doesn't it?

If so, I'm inclined to your proposition.
A new bool *found argument of build_join_rel() makes reduce number of
FDW handler call, with keeping reasonable information to build remote-
join query.

Thanks,
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei kai...@ak.jp.nec.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)