Re: ctidscan as an example of custom-scan (Re: [HACKERS] [v9.5] Custom Plan API)
On Wed, Jul 15, 2015 at 2:28 AM, Michael Paquier wrote: > On Wed, Jul 15, 2015 at 10:12 AM, Kouhei Kaigai wrote: >> We never guarantee the interface compatibility between major version up. >> If we add/modify interface on v9.6, it is duty for developer of extensions >> to follow the new version, even not specific to custom-scan provider. >> If v9.6 adds support fine-grained function cost estimation, I also have >> to follow the feature, but it is natural. > > Maintaining compatibility across major versions is a best-effort and > even if we sometimes break things across major versions, and sometimes > even silently (take the example of 9.3's background worker that do not > start with 9.4 as long as bgw_notify_pid is not set to 0), the > approach is usually taken to have APIs stable and convenient able to > cover a maximum set of cases for a given set of plugins, and this > serves well in the long term. Now it is true that we cannot assume > either that the version of a plugin API will be perfect forever and > will be able to cover all the imagined test cases at first shot, still > I'd like to think that things are broken only when necessary and with > good reasons. A set of APIs changed at each major release tends to be > proof that research lacked in the first version, and would surely > demotivate its adoption by extension developers. Maybe. If those changes are demanded by extension developers who are trying to do useful stuff with the APIs and finding problems, then changing things will probably make those extension developers happy. If the changes are necessitated by core improvements, then we might get some complaints if those core improvements are perceived to have small value compared to the API break. But I think complaints of that kind are very rare. The only thing I can think of that falls into approximately that category is a gripe about the replacement of relistemp by relpersistence. But I think that was more motivated by the way it broke SQL monitoring queries than by the way it broke C code. There may be other more recent examples; but that's the only one I can think of offhand. I just don't see it as a big issue. -- 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: ctidscan as an example of custom-scan (Re: [HACKERS] [v9.5] Custom Plan API)
On Tue, Jul 14, 2015 at 6:04 PM, Tom Lane wrote: > Robert Haas writes: >> Both you and Andres have articulated the concern that CustomScan isn't >> actually useful, but I still don't really understand why not. > >> I'm curious, for example, whether CustomScan would have been >> sufficient to build TABLESAMPLE, and if not, why not. Obviously the >> syntax has to be in core, > > ... so you just made the point ... If you're going to set the bar that high, there's no point in anyone ever trying to add extensibility anywhere, because there will always be some other place where there isn't enough extensibility someplace else to do everything that someone might want. The fact that the parser isn't extensible doesn't make custom scans useless any more than the fact that the non-extensibility of WAL-logging makes pg_am useless. >> but why couldn't the syntax just call an >> extension-provided callback that returns a custom scan, instead of >> having a node just for TABLESAMPLE? > > Because that only works for small values of "work". As far as TABLESAMPLE > goes, I intend to hold Simon's feet to the fire until there's a less > cheesy answer to the problem of scan reproducibility. Assuming we're > going to allow sample methods that can't meet the reproducibility > requirement, we need something to prevent them from producing visibly > broken query results. Ideally, the planner would avoid putting such a > scan on the inside of a nestloop. A CustomScan-based implementation could > not possibly arrange such a thing; we'd have to teach the core planner > about the concern. Well, I think it would be better to change the tablesample methods as you previously proposed, so that they are actually stable. But if that's not possible, then when you (or someone) makes the core planner able to consider such matters, you could add a CUSTOM_PATH_UNSTABLE flag that a custom path can use to notify the core system about the problem. Then tablesample methods that are unstable can set the flag and those that are stable are not penalized. Presumably we'll end up with such a flag in the tablesample-path anyway, and a separate flag in every kind of future scan that needs similar consideration. If we put it in some piece of infrastructure (like the custom-path stuff) that is reusable, we could avoid reinventing the wheel every time. > Or, taking the example of a GpuScan node, it's essentially impossible > to persuade the planner to delegate any expensive function calculations, > aggregates, etc to such a node; much less teach it that that way is cheaper > than doing such things the usual way. So yeah, KaiGai-san may have a > module that does a few things with a GPU, but it's far from doing all or > even very much of what one would want. It's true that there are things he can't do this way, but without the custom-scan stuff, he'd be able to do even less. > Now, as part of the upper-planner-rewrite business that I keep hoping to > get to when I'm not riding herd on bad patches, it's likely that we might > have enough new infrastructure soon that that particular problem could > be solved. But there would just be another problem after that; a likely > example is not having adequate statistics, or sufficiently fine-grained > function cost estimates, to be able to make valid choices once there's > more than one way to do such calculations. (I'm not really impressed by > "the GPU is *always* faster" approaches.) Significant improvements of > that sort are going to take core-code changes. Probably, but giving people the ability to experiment outside core, even if it's limited, often leads to good things. And when it doesn't, it doesn't really cost us anything. > Even worse, if there do get to be any popular custom-scan extensions, > we'll break them anytime we make any nontrivial planner changes, because > there is no arms-length API there. A trivial example is that even adding > or changing any fields in struct Path will necessarily break custom scan > providers, because they build Paths for themselves with no interposed API. I agree that will happen, but I don't care. This happens all the time, and every person other than yourself who has commented on this issue has said that they'd rather have an API exposed that will later get whacked around without warning than have no API exposed at all, not just with respect to custom-paths, but with a whole variety of other things as well, like sticking PGDLLIMPORT on all of the variables that back GUCs. It is really far more tedious to try to work around the lack of a PGDLLIMPORT marking (which causes a huge annoyance now) than to adjust the code if and when somebody changes something about the GUC (which causes a small annoyance later, and only if there actually are changes). > In large part this is the same as my core concern about the TABLESAMPLE > patch: exposing dubiously-designed APIs is soon going to force us to make > choices between breaking those APIs or not being ab
Re: ctidscan as an example of custom-scan (Re: [HACKERS] [v9.5] Custom Plan API)
> -Original Message- > From: Michael Paquier [mailto:michael.paqu...@gmail.com] > Sent: Wednesday, July 15, 2015 3:29 PM > To: Kaigai Kouhei(海外 浩平) > Cc: Tom Lane; Robert Haas; Alvaro Herrera; hlinnaka; Jim Nasby; Kohei KaiGai; > PgHacker; Simon Riggs > Subject: Re: ctidscan as an example of custom-scan (Re: [HACKERS] [v9.5] > Custom > Plan API) > > On Wed, Jul 15, 2015 at 10:12 AM, Kouhei Kaigai wrote: > > We never guarantee the interface compatibility between major version up. > > If we add/modify interface on v9.6, it is duty for developer of extensions > > to follow the new version, even not specific to custom-scan provider. > > If v9.6 adds support fine-grained function cost estimation, I also have > > to follow the feature, but it is natural. > > Maintaining compatibility across major versions is a best-effort and > even if we sometimes break things across major versions, and sometimes > even silently (take the example of 9.3's background worker that do not > start with 9.4 as long as bgw_notify_pid is not set to 0), the > approach is usually taken to have APIs stable and convenient able to > cover a maximum set of cases for a given set of plugins, and this > serves well in the long term. Now it is true that we cannot assume > either that the version of a plugin API will be perfect forever and > will be able to cover all the imagined test cases at first shot, still > I'd like to think that things are broken only when necessary and with > good reasons. A set of APIs changed at each major release tends to be > proof that research lacked in the first version, and would surely > demotivate its adoption by extension developers. > Yep, I also don't want to break the interface compatibility without something reasonable, and also think following-up new core features is a good reason to enhance the interface in the future version. Thanks, -- NEC Business Creation Division / PG-Strom Project KaiGai Kohei -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: ctidscan as an example of custom-scan (Re: [HACKERS] [v9.5] Custom Plan API)
On Wed, Jul 15, 2015 at 10:12 AM, Kouhei Kaigai wrote: > We never guarantee the interface compatibility between major version up. > If we add/modify interface on v9.6, it is duty for developer of extensions > to follow the new version, even not specific to custom-scan provider. > If v9.6 adds support fine-grained function cost estimation, I also have > to follow the feature, but it is natural. Maintaining compatibility across major versions is a best-effort and even if we sometimes break things across major versions, and sometimes even silently (take the example of 9.3's background worker that do not start with 9.4 as long as bgw_notify_pid is not set to 0), the approach is usually taken to have APIs stable and convenient able to cover a maximum set of cases for a given set of plugins, and this serves well in the long term. Now it is true that we cannot assume either that the version of a plugin API will be perfect forever and will be able to cover all the imagined test cases at first shot, still I'd like to think that things are broken only when necessary and with good reasons. A set of APIs changed at each major release tends to be proof that research lacked in the first version, and would surely demotivate its adoption by extension developers. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: ctidscan as an example of custom-scan (Re: [HACKERS] [v9.5] Custom Plan API)
> Or, taking the example of a GpuScan node, it's essentially impossible > to persuade the planner to delegate any expensive function calculations, > aggregates, etc to such a node; much less teach it that that way is cheaper > than doing such things the usual way. So yeah, KaiGai-san may have a > module that does a few things with a GPU, but it's far from doing all or > even very much of what one would want. > Why do we need to run all the functions on GPU device? PG-Strom simply gives up to inject CustomPath if required qualifier contains unsupported functions or data types, thus, these workloads are executed as usual. I don't intend 100% coverage by GPU. That's no matter. People who use massive numerical/mathematical workload will love GPU. > Now, as part of the upper-planner-rewrite business that I keep hoping to > get to when I'm not riding herd on bad patches, it's likely that we might > have enough new infrastructure soon that that particular problem could > be solved. But there would just be another problem after that; a likely > example is not having adequate statistics, or sufficiently fine-grained > function cost estimates, to be able to make valid choices once there's > more than one way to do such calculations. (I'm not really impressed by > "the GPU is *always* faster" approaches.) Significant improvements of > that sort are going to take core-code changes. > We never guarantee the interface compatibility between major version up. If we add/modify interface on v9.6, it is duty for developer of extensions to follow the new version, even not specific to custom-scan provider. If v9.6 adds support fine-grained function cost estimation, I also have to follow the feature, but it is natural. > Even worse, if there do get to be any popular custom-scan extensions, > we'll break them anytime we make any nontrivial planner changes, because > there is no arms-length API there. A trivial example is that even adding > or changing any fields in struct Path will necessarily break custom scan > providers, because they build Paths for themselves with no interposed API. > I cannot understand... If Path field gets changed, it is duty of extension to follow the core change. We usually add/modify API specifications on major version up. For example, I remember ProcessUtility_hook has been changed a few times in the last several years. Thanks, -- NEC Business Creation Division / PG-Strom Project KaiGai Kohei -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: ctidscan as an example of custom-scan (Re: [HACKERS] [v9.5] Custom Plan API)
> -Original Message- > From: Tom Lane [mailto:t...@sss.pgh.pa.us] > Sent: Wednesday, July 15, 2015 5:47 AM > To: Robert Haas > Cc: Alvaro Herrera; hlinnaka; Kaigai Kouhei(海外 浩平); Michael Paquier; Jim > Nasby; Kohei KaiGai; PgHacker; Simon Riggs > Subject: Re: ctidscan as an example of custom-scan (Re: [HACKERS] [v9.5] > Custom > Plan API) > > Robert Haas writes: > > On Tue, Jul 14, 2015 at 3:07 PM, Alvaro Herrera > > wrote: > >> As a general principle, I think it's a good idea to have a module that's > >> mostly just a skeleton that guides people into writing something real to > >> use whatever API is being tested. It needs to be simple enough that not > >> much need to be deleted when writing the real thing, and complex enough > >> to cover the parts that need covering. If whatever replaces ctidscan is > >> too complex, it will not serve that purpose. > >> > >> My guess is that something whose only purpose is to test the custom scan > >> interface for coverage purposes can be simpler than this module. > > > See, I actually think the opposite: I think we've been accumulating a > > reasonable amount of test code that actually serves no really useful > > purpose and is just cruft. Stuff like test_shm_mq and test_decoding > > seem like they actually catches bugs, so I like that, but I think > > stuff like worker_spi is actually TOO simple to be useful in building > > anything real, and it provides no useful test coverage, either. But > > this is all a matter of opinion, of course, and I'll defer to whatever > > the consensus is. > > I think this ties into my core unhappiness with the customscan stuff, > which is that I don't believe it's *possible* to do anything of very > great interest with it. I think anything really useful will require > core code modifications and/or hooks that don't exist now. So a finger > exercise like ctidscan, even though it might have some marginal use, > doesn't do much to alleviate that concern. It certainly doesn't seem > like it's a suitable placeholder proving that we aren't breaking any > actual use cases for the feature. > The ctidscan is originally designed to validate the behavior of custom-scan interface, so it is natural this module is valuable in limited cased. However, I don't think that anything valuable usually takes core code enhancement and/or new hooks, because we already have various extensions around core code that utilizes existing infrastructures (even though its specifications may be changed on major version up). At least, custom-scan enables to implement edge-features which are not easy to merge the core because of various reasons; like dependency to proprietary library, too experimental features, too large code to review as minimum valuable portion and so on. > (BTW, if we care about the use cases this has, such as data recovery from > partially-corrupt tables, it would make way more sense and take way less > net new code to just teach TidScan about it.) > What I discussed with Hanada-san before was, a custom-scan provider that replaces a particular relations join by simple scan of materialized-view transparently. It is probably one other use case. Its design is in my brain, but time for development is missing piece for me. Thanks, -- NEC Business Creation Division / PG-Strom Project KaiGai Kohei -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: ctidscan as an example of custom-scan (Re: [HACKERS] [v9.5] Custom Plan API)
Robert Haas writes: > Both you and Andres have articulated the concern that CustomScan isn't > actually useful, but I still don't really understand why not. > I'm curious, for example, whether CustomScan would have been > sufficient to build TABLESAMPLE, and if not, why not. Obviously the > syntax has to be in core, ... so you just made the point ... > but why couldn't the syntax just call an > extension-provided callback that returns a custom scan, instead of > having a node just for TABLESAMPLE? Because that only works for small values of "work". As far as TABLESAMPLE goes, I intend to hold Simon's feet to the fire until there's a less cheesy answer to the problem of scan reproducibility. Assuming we're going to allow sample methods that can't meet the reproducibility requirement, we need something to prevent them from producing visibly broken query results. Ideally, the planner would avoid putting such a scan on the inside of a nestloop. A CustomScan-based implementation could not possibly arrange such a thing; we'd have to teach the core planner about the concern. Or, taking the example of a GpuScan node, it's essentially impossible to persuade the planner to delegate any expensive function calculations, aggregates, etc to such a node; much less teach it that that way is cheaper than doing such things the usual way. So yeah, KaiGai-san may have a module that does a few things with a GPU, but it's far from doing all or even very much of what one would want. Now, as part of the upper-planner-rewrite business that I keep hoping to get to when I'm not riding herd on bad patches, it's likely that we might have enough new infrastructure soon that that particular problem could be solved. But there would just be another problem after that; a likely example is not having adequate statistics, or sufficiently fine-grained function cost estimates, to be able to make valid choices once there's more than one way to do such calculations. (I'm not really impressed by "the GPU is *always* faster" approaches.) Significant improvements of that sort are going to take core-code changes. Even worse, if there do get to be any popular custom-scan extensions, we'll break them anytime we make any nontrivial planner changes, because there is no arms-length API there. A trivial example is that even adding or changing any fields in struct Path will necessarily break custom scan providers, because they build Paths for themselves with no interposed API. In large part this is the same as my core concern about the TABLESAMPLE patch: exposing dubiously-designed APIs is soon going to force us to make choices between breaking those APIs or not being able to make changes we need to make. In the case of custom scans, I will not be particularly sad when (not if) we break custom scan providers; but in other cases such tradeoffs are going to be harder to make. 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: ctidscan as an example of custom-scan (Re: [HACKERS] [v9.5] Custom Plan API)
On Tue, Jul 14, 2015 at 4:47 PM, Tom Lane wrote: > I think this ties into my core unhappiness with the customscan stuff, > which is that I don't believe it's *possible* to do anything of very > great interest with it. I think anything really useful will require > core code modifications and/or hooks that don't exist now. So a finger > exercise like ctidscan, even though it might have some marginal use, > doesn't do much to alleviate that concern. It certainly doesn't seem > like it's a suitable placeholder proving that we aren't breaking any > actual use cases for the feature. Both you and Andres have articulated the concern that CustomScan isn't actually useful, but I still don't really understand why not. KaiGai has got working code (under a GPL license) that shows that it can be used for what he calls GpuScan and GpuJoin, and the speedups are actually pretty cool if you happen to have the right sort of query to take advantage of it. That code may be buggy and the license precludes us using it anyway, but FWICT it does seem to work. I'd be entirely amenable to improving the infrastructure such that it could be used for a broader array of purposes, and I'm also amenable to adding more hooks if we need more hooks to make it useful, but I'm not clear at all on what you think is missing. I'm curious, for example, whether CustomScan would have been sufficient to build TABLESAMPLE, and if not, why not. Obviously the syntax has to be in core, but why couldn't the syntax just call an extension-provided callback that returns a custom scan, instead of having a node just for TABLESAMPLE? -- 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: ctidscan as an example of custom-scan (Re: [HACKERS] [v9.5] Custom Plan API)
Robert Haas writes: > On Tue, Jul 14, 2015 at 3:07 PM, Alvaro Herrera > wrote: >> As a general principle, I think it's a good idea to have a module that's >> mostly just a skeleton that guides people into writing something real to >> use whatever API is being tested. It needs to be simple enough that not >> much need to be deleted when writing the real thing, and complex enough >> to cover the parts that need covering. If whatever replaces ctidscan is >> too complex, it will not serve that purpose. >> >> My guess is that something whose only purpose is to test the custom scan >> interface for coverage purposes can be simpler than this module. > See, I actually think the opposite: I think we've been accumulating a > reasonable amount of test code that actually serves no really useful > purpose and is just cruft. Stuff like test_shm_mq and test_decoding > seem like they actually catches bugs, so I like that, but I think > stuff like worker_spi is actually TOO simple to be useful in building > anything real, and it provides no useful test coverage, either. But > this is all a matter of opinion, of course, and I'll defer to whatever > the consensus is. I think this ties into my core unhappiness with the customscan stuff, which is that I don't believe it's *possible* to do anything of very great interest with it. I think anything really useful will require core code modifications and/or hooks that don't exist now. So a finger exercise like ctidscan, even though it might have some marginal use, doesn't do much to alleviate that concern. It certainly doesn't seem like it's a suitable placeholder proving that we aren't breaking any actual use cases for the feature. (BTW, if we care about the use cases this has, such as data recovery from partially-corrupt tables, it would make way more sense and take way less net new code to just teach TidScan about it.) 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: ctidscan as an example of custom-scan (Re: [HACKERS] [v9.5] Custom Plan API)
On Tue, Jul 14, 2015 at 3:07 PM, Alvaro Herrera wrote: >> We don't have anything that currently tests the Custom Scan interface >> in the tree. The question is how important that is, and whether it's >> worth having what's basically a toy implementation just to demonstrate >> that the feature can work. If so, I think ctidscan is as good a toy >> example as any; in the interest of full disclosure, I was the one who >> suggested it in the first place. But I am not entirely sure it's a >> good idea to saddle ourselves with that maintenance effort. It would >> be a lot more interesting if we had an example that figured to be >> generally useful. > > As a general principle, I think it's a good idea to have a module that's > mostly just a skeleton that guides people into writing something real to > use whatever API is being tested. It needs to be simple enough that not > much need to be deleted when writing the real thing, and complex enough > to cover the parts that need covering. If whatever replaces ctidscan is > too complex, it will not serve that purpose. > > My guess is that something whose only purpose is to test the custom scan > interface for coverage purposes can be simpler than this module. See, I actually think the opposite: I think we've been accumulating a reasonable amount of test code that actually serves no really useful purpose and is just cruft. Stuff like test_shm_mq and test_decoding seem like they actually catches bugs, so I like that, but I think stuff like worker_spi is actually TOO simple to be useful in building anything real, and it provides no useful test coverage, either. But this is all a matter of opinion, of course, and I'll defer to whatever the consensus is. -- 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: ctidscan as an example of custom-scan (Re: [HACKERS] [v9.5] Custom Plan API)
Robert Haas wrote: > We don't have anything that currently tests the Custom Scan interface > in the tree. The question is how important that is, and whether it's > worth having what's basically a toy implementation just to demonstrate > that the feature can work. If so, I think ctidscan is as good a toy > example as any; in the interest of full disclosure, I was the one who > suggested it in the first place. But I am not entirely sure it's a > good idea to saddle ourselves with that maintenance effort. It would > be a lot more interesting if we had an example that figured to be > generally useful. As a general principle, I think it's a good idea to have a module that's mostly just a skeleton that guides people into writing something real to use whatever API is being tested. It needs to be simple enough that not much need to be deleted when writing the real thing, and complex enough to cover the parts that need covering. If whatever replaces ctidscan is too complex, it will not serve that purpose. My guess is that something whose only purpose is to test the custom scan interface for coverage purposes can be simpler than this module. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: ctidscan as an example of custom-scan (Re: [HACKERS] [v9.5] Custom Plan API)
On Fri, Jul 10, 2015 at 8:55 AM, Heikki Linnakangas wrote: >> If somebody still needs it, I'll rebase and adjust the patch towards >> the latest custom-scan interface. However, I cannot be motivated for >> the feature nobody wants. > > Robert, can you weigh in on this? Do we currently have anything in the > tree that tests the Custom Scan interface? If not, would this be helpful > for that purpose? We don't have anything that currently tests the Custom Scan interface in the tree. The question is how important that is, and whether it's worth having what's basically a toy implementation just to demonstrate that the feature can work. If so, I think ctidscan is as good a toy example as any; in the interest of full disclosure, I was the one who suggested it in the first place. But I am not entirely sure it's a good idea to saddle ourselves with that maintenance effort. It would be a lot more interesting if we had an example that figured to be generally useful. -- 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: ctidscan as an example of custom-scan (Re: [HACKERS] [v9.5] Custom Plan API)
On 07/02/2015 08:21 AM, Kouhei Kaigai wrote: > Folks, > >> Moved this patch to next CF 2015-02 because of lack of review(ers). >> > Do we still need this patch as contrib module? > It was originally required it as example of custom-scan interface last > summer, however, here was no strong requirement after that, then, it > was bumped to v9.6 development cycle. > > If somebody still needs it, I'll rebase and adjust the patch towards > the latest custom-scan interface. However, I cannot be motivated for > the feature nobody wants. Robert, can you weigh in on this? Do we currently have anything in the tree that tests the Custom Scan interface? If not, would this be helpful for that purpose? - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: ctidscan as an example of custom-scan (Re: [HACKERS] [v9.5] Custom Plan API)
Folks, > Moved this patch to next CF 2015-02 because of lack of review(ers). > Do we still need this patch as contrib module? It was originally required it as example of custom-scan interface last summer, however, here was no strong requirement after that, then, it was bumped to v9.6 development cycle. If somebody still needs it, I'll rebase and adjust the patch towards the latest custom-scan interface. However, I cannot be motivated for the feature nobody wants. Thanks, -- NEC Business Creation Division / PG-Strom Project KaiGai Kohei > -Original Message- > From: pgsql-hackers-ow...@postgresql.org > [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier > Sent: Friday, February 13, 2015 4:41 PM > To: Kaigai Kouhei(海外 浩平) > Cc: Jim Nasby; Robert Haas; Simon Riggs; Kohei KaiGai; PgHacker > Subject: Re: ctidscan as an example of custom-scan (Re: [HACKERS] [v9.5] > Custom > Plan API) > > > > On Tue, Jan 6, 2015 at 10:51 PM, Kouhei Kaigai wrote: > > > Jim, Thanks for your reviewing the patch. > > The attached patch is revised one according to your suggestion, > and also includes bug fix I could found. > > * Definitions of TIDOperator was moved to pg_operator.h > as other operator doing. > * Support the case of "ctid (operator) Param" expression. > * Add checks if commutator of TID was not found. > * Qualifiers gets extracted on plan stage, not path stage. > * Adjust cost estimation logic to fit SeqScan manner. > * Add some new test cases for regression test. > > > On 12/24/14, 7:54 PM, Kouhei Kaigai wrote: > > >>> On Mon, Dec 15, 2014 at 4:55 PM, Kouhei Kaigai > > >>> > > >> wrote: > > >>>> I'm not certain whether we should have this functionality in > > >>>> contrib from the perspective of workload that can help, but its > > >>>> major worth is for an example of custom-scan interface. > > >>> worker_spi is now in src/test/modules. We may add it there as > well, > > no? > > >>> > > >> Hmm, it makes sense for me. Does it also make sense to add a > > >> test-case to the core regression test cases? > > >> > > > The attached patch adds ctidscan module at test/module instead of > contrib. > > > Basic portion is not changed from the previous post, but file > > > locations and test cases in regression test are changed. > > > > First, I'm glad for this. It will be VERY valuable for anyone trying > to > > clean up the end of a majorly bloated table. > > > > Here's a partial review... > > > > +++ b/src/test/modules/ctidscan/ctidscan.c > > > > +/* missing declaration in pg_proc.h */ > > +#ifndef TIDGreaterOperator > > +#define TIDGreaterOperator 2800 > > ... > > If we're calling that out here, should we have a corresponding comment > in > > pg_proc.h, in case these ever get renumbered? > > > It was a quick hack when I moved this module out of the tree. > Yep, I should revert when I submit this patch again. > > > +CTidQualFromExpr(Node *expr, int varno) > > ... > > + if (IsCTIDVar(arg1, varno)) > > + other = arg2; > > + else if (IsCTIDVar(arg2, varno)) > > + other = arg1; > > + else > > + return NULL; > > + if (exprType(other) != TIDOID) > > + return NULL;/* should not happen */ > > + /* The other argument must be a pseudoconstant */ > > + if (!is_pseudo_constant_clause(other)) > > + return NULL; > > > > I think this needs some additional blank lines... > > > OK. And, I also noticed the coding style around this function is > different from other built-in plans, so I redefined the role of > this function just to check whether the supplied RestrictInfo is > OpExpr that involves TID inequality operator, or not. > > Expression node shall be extracted when Plan node is created > from Path node. > > > + if (IsCTIDVar(arg1, varno)) > > + other = arg2; > > + else if (IsCTIDVar(arg2, v
Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
Let me remind the problem not to be forgotten towards v9.5. The commit 1a8a4e5cde2b7755e11bde2ea7897bd650622d3e disallowed extensions to call create_plan_recurse(), however, it is required for custom-scan node that implements own join logic and takes child nodes to construct Plan node from Path node. So, at this moment, we cannot utilize the new feature well unless extension copies & pastes createplan.c to its own source tree (ugly!). Tom suggested an alternative infrastructure that tells planner Path nodes to be passed to create_plan_recurse() in createplan.c. > > 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. > > > The attached patch implements what you suggested as is. > It allows custom-scan providers to have child Paths without exporting > create_plan_recurse(), and enables to represent N-way join naturally. > Please add any solution, even if we don't reach the consensus of how > create_plan_recurse (and other useful static functions) are visible to > extensions. > Then, I made a patch (which was attached on the last message) according to the suggestion. I think it is one possible option. Or, one other option is to revert create_plan_recurse() non-static function as the infrastructure originally designed. I expect people think it is not a graceful design to force extensions to copy and paste core file with small adjustment. So, either of options, or others if any, needs to be merged to solve the problem. Thanks, -- NEC Business Creation Division / PG-Strom Project KaiGai Kohei > -Original Message- > From: pgsql-hackers-ow...@postgresql.org > [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Kouhei Kaigai > Sent: Friday, May 15, 2015 8:44 AM > To: Tom Lane; Kohei KaiGai > Cc: Robert Haas; Thom Brown; Shigeru Hanada; pgsql-hackers@postgreSQL.org > Subject: Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan 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. > > > The attached patch implements what you suggested as is. > It allows custom-scan providers to have child Paths without exporting > create_plan_recurse(), and enables to represent N-way join naturally. > Please add any solution, even if we don't reach the consensus of how > create_plan_recurse (and other useful static functions) are visible to > extensions. > > > Patch detail: > It adds a List field (List *custom_children) to CustomPath structure > to inform planner its child Path nodes, to be transformed to Plan node > through the planner's job. > CustomScan also have a new List field to have its child Plan nodes > which shall be processed by setrefs.c and subselect.c. > PlanCustomPath callback was extended to have a list of Plan nodes > that were constructed on create_customscan_plan in core, it is > a job of custom-scan provider to attach these Plan nodes onto > lefttree, righttree or the custom_children list. > CustomScanState also have an array to have PlanState nodes of the > children. It is used for EXPLAIN command know the child nodes. > > Regarding of FDW, as Hanada-san mentioned, I'm uncertain whether > similar feature is also needed because its join-pushdown feature > scan on the result-set of remotely joined relations, thus no need > to have local child Path nodes. > So, I put this custom_children list on Custom structure only. > > It may need additional section in the documentation. > > Thanks, > -- > NEC Business Creation Division / PG-Strom Project > KaiGai Kohei -- 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-15 8:43 GMT+09:00 Kouhei Kaigai : > Regarding of FDW, as Hanada-san mentioned, I'm uncertain whether > similar feature is also needed because its join-pushdown feature > scan on the result-set of remotely joined relations, thus no need > to have local child Path nodes. > So, I put this custom_children list on Custom structure only. AFAIS most of FDWs won't need child paths to process their external data. The most possible idea is that a FDW uses output of ForeignScan plan node which is handled by the FDW, but such work should be done by another CSP (or at least via CSP I/F). -- 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)
> 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. > The attached patch implements what you suggested as is. It allows custom-scan providers to have child Paths without exporting create_plan_recurse(), and enables to represent N-way join naturally. Please add any solution, even if we don't reach the consensus of how create_plan_recurse (and other useful static functions) are visible to extensions. Patch detail: It adds a List field (List *custom_children) to CustomPath structure to inform planner its child Path nodes, to be transformed to Plan node through the planner's job. CustomScan also have a new List field to have its child Plan nodes which shall be processed by setrefs.c and subselect.c. PlanCustomPath callback was extended to have a list of Plan nodes that were constructed on create_customscan_plan in core, it is a job of custom-scan provider to attach these Plan nodes onto lefttree, righttree or the custom_children list. CustomScanState also have an array to have PlanState nodes of the children. It is used for EXPLAIN command know the child nodes. Regarding of FDW, as Hanada-san mentioned, I'm uncertain whether similar feature is also needed because its join-pushdown feature scan on the result-set of remotely joined relations, thus no need to have local child Path nodes. So, I put this custom_children list on Custom structure only. It may need additional section in the documentation. Thanks, -- NEC Business Creation Division / PG-Strom Project KaiGai Kohei custom-join-problem-option-2.v1.patch Description: custom-join-problem-option-2.v1.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-05-12 10:24 GMT+09:00 Kouhei Kaigai : > option-2) > Tom's suggestion. Add a new list field of Path nodes on CustomPath, > then create_customscan_plan() will call static create_plan_recurse() > function to construct child plan nodes. > Probably, the attached patch will be an image of this enhancement, > but not tested yet, of course. Once we adopt this approach, I'll > adjust my PG-Strom code towards the new interface within 2 weeks > and report problems if any. +1. This design achieves the functionality required for custom join by Kaigai-san's use case, instantiating sub-plans of CustomScan plan recursively, without exposing create_plan_recurse. CSP can use the index number to distinguish information, like FDWs do with fdw_private. IMO it isn't necessary to apply the change onto ForeignPath/ForeignScan. FDW would have a way to keep-and-use sub paths as private information. In fact, I wrote postgres_fdw to use create_plan_recurse to generate SQL statements of inner/outer relations to be joined, but as of now SQL construction for join push-down is accomplished by calling private function deparseSelectSql recursively at the top of a join tree. Some FDW might hope to use sub-plan generation, but we don't have any concrete use case as of now. -- 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)
Hello, I tried to make patches for the three approaches. Please don't think the option-3 serious proposition, however, it is the only solution at this moment unfortunately. In my understanding, we don't guarantee interface compatibility across major version up, including the definitions of non-static functions. It is role of extension's author to follow up the new major version (and raise a problem report during development cycle if feature update makes problems without alternatives). In fact, people usually submit patches and a part of them changes definition of non-static functions, however, nobody can guarantee no extension uses this function thus don't break compatibility. It is a collateral evidence we don't think non-static functions are not stable interface for extensions, and it shall not be a reason why to prohibit functions in spite of its necessity. On the other hands, I understand it is not only issues around createplan.c, but also a (philosophical) issue around criteria and human's decision which functions should be static or non-static. So, it usually takes time to get overall consensus. If we keep the create_plan_recurse() static, the option-2 is a solution to balance both of opinions. Anyway, I really dislike the option-3, want to have a solution. Thanks, -- NEC Business Creation Division / PG-Strom Project KaiGai Kohei > -Original Message- > From: Kaigai Kouhei(海外 浩平) > Sent: Tuesday, May 12, 2015 10:24 AM > To: 'Andres Freund'; 'Robert Haas' > Cc: 'Tom Lane'; 'Kohei KaiGai'; 'Thom Brown'; 'Shigeru Hanada'; > 'pgsql-hackers@postgreSQL.org' > Subject: RE: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API) > > Hi, > > Let me back to the original concern and show possible options > we can take here. At least, the latest master is not usable to > implement custom-join logic without either of these options. > > option-1) > Revert create_plan_recurse() to non-static function for extensions. > It is the simplest solution, however, it is still gray zone which > functions shall be public and whether we deal with the non-static > functions as a stable API or not. > IMO, we shouldn't treat non-static functions as stable APIs, even > if it can be called by extensions not only codes in another source > file. In fact, we usually changes definition of non-static functions > even though we know extensions uses. It is role of extension to > follow up the feature across major version. > > > option-2) > Tom's suggestion. Add a new list field of Path nodes on CustomPath, > then create_customscan_plan() will call static create_plan_recurse() > function to construct child plan nodes. > Probably, the attached patch will be an image of this enhancement, > but not tested yet, of course. Once we adopt this approach, I'll > adjust my PG-Strom code towards the new interface within 2 weeks > and report problems if any. > > > option-3) > Enforce authors of custom-scan provider to copy and paste createplan.c. > I really don't want this option and nobody will be happy. > > Thanks, > -- > NEC Business Creation Division / PG-Strom Project > KaiGai Kohei > > > > -Original Message- > > From: Kaigai Kouhei(海外 浩平) > > Sent: Monday, May 11, 2015 12:48 PM > > To: 'Andres Freund'; Robert Haas > > Cc: Tom Lane; Kohei KaiGai; Thom Brown; Shigeru Hanada; > > pgsql-hackers@postgreSQL.org > > Subject: RE: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API) > > > > > On 2015-05-10 21:26:26 -0400, Robert Haas wrote: > > > > On Sun, May 10, 2015 at 8:41 PM, Tom Lane 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 us
Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
Hi, Let me back to the original concern and show possible options we can take here. At least, the latest master is not usable to implement custom-join logic without either of these options. option-1) Revert create_plan_recurse() to non-static function for extensions. It is the simplest solution, however, it is still gray zone which functions shall be public and whether we deal with the non-static functions as a stable API or not. IMO, we shouldn't treat non-static functions as stable APIs, even if it can be called by extensions not only codes in another source file. In fact, we usually changes definition of non-static functions even though we know extensions uses. It is role of extension to follow up the feature across major version. option-2) Tom's suggestion. Add a new list field of Path nodes on CustomPath, then create_customscan_plan() will call static create_plan_recurse() function to construct child plan nodes. Probably, the attached patch will be an image of this enhancement, but not tested yet, of course. Once we adopt this approach, I'll adjust my PG-Strom code towards the new interface within 2 weeks and report problems if any. option-3) Enforce authors of custom-scan provider to copy and paste createplan.c. I really don't want this option and nobody will be happy. Thanks, -- NEC Business Creation Division / PG-Strom Project KaiGai Kohei > -Original Message- > From: Kaigai Kouhei(海外 浩平) > Sent: Monday, May 11, 2015 12:48 PM > To: 'Andres Freund'; Robert Haas > Cc: Tom Lane; Kohei KaiGai; Thom Brown; Shigeru Hanada; > pgsql-hackers@postgreSQL.org > Subject: RE: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API) > > > On 2015-05-10 21:26:26 -0400, Robert Haas wrote: > > > On Sun, May 10, 2015 at 8:41 PM, Tom Lane 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 custom-join-children.patch Description: custom-join-children.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)
On Sun, May 10, 2015 at 11:07 PM, Andres Freund wrote: > 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. I see your point, but I don't think it really detracts from mine. The fact that we have a few inconsistently-named list functions is not preventing any core development project that would otherwise get completed to instead not get completed. Nor is any of that other stuff, except maybe the libpq API, but that's a lot (not just a bit) more exposed. Also, I'd actually be in favor of looking for a way to identify the StringInfo and PQexpBuffer stuff - and of partially deprecating the V0 calling convention. -- 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 2015-05-10 21:26:26 -0400, Robert Haas wrote: > > On Sun, May 10, 2015 at 8:41 PM, Tom Lane 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 -- 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 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)
On Sun, May 10, 2015 at 10:37 PM, Andres Freund 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)
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 2015-05-10 21:26:26 -0400, Robert Haas wrote: > On Sun, May 10, 2015 at 8:41 PM, Tom Lane 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)
On Sun, May 10, 2015 at 9:34 PM, Tom Lane wrote: > Robert Haas 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)
Robert Haas 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)
On Sun, May 10, 2015 at 8:41 PM, Tom Lane wrote: > Kohei KaiGai 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)
> Kohei KaiGai 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 -- 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)
Kohei KaiGai 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)
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 -- 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)
Robert Haas writes: > On Sat, May 9, 2015 at 1:05 PM, Tom Lane 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)
On Sat, May 9, 2015 at 1:05 PM, Tom Lane 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.
Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
Robert Haas writes: > On Fri, May 8, 2015 at 5:48 PM, Tom Lane 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 8:18 GMT+09:00 Kohei KaiGai : > 2015-05-09 2:46 GMT+09:00 Tom Lane : >> Kouhei Kaigai 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 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 8:32 GMT+09:00 Kohei KaiGai :
> 2015-05-09 3:51 GMT+09:00 Tom Lane :
>> Robert Haas writes:
>>> On Fri, May 8, 2015 at 1:46 PM, Tom Lane 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
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-09 11:21 GMT+09:00 Robert Haas : > On Fri, May 8, 2015 at 5:48 PM, Tom Lane 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 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)
On Fri, May 8, 2015 at 5:48 PM, Tom Lane 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)
On Fri, May 8, 2015 at 2:51 PM, Tom Lane 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-09 3:51 GMT+09:00 Tom Lane :
> Robert Haas writes:
>> On Fri, May 8, 2015 at 1:46 PM, Tom Lane 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
--
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 2:46 GMT+09:00 Tom Lane : > Kouhei Kaigai 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. Thanks, -- KaiGai Kohei -- 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 6:48 GMT+09:00 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? > 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 -- 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)
... 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)
Robert Haas writes: > On Fri, May 8, 2015 at 1:46 PM, Tom Lane 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)
On Fri, May 8, 2015 at 1:46 PM, Tom Lane 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)
Kouhei Kaigai 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)
> Robert Haas writes: > > On Thu, Apr 30, 2015 at 5:21 PM, Kouhei Kaigai 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 -- 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 writes: > On Thu, Apr 30, 2015 at 5:21 PM, Kouhei Kaigai 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)
On Thu, Apr 30, 2015 at 5:21 PM, Kouhei Kaigai 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)
> On Thu, Apr 30, 2015 at 9:16 AM, Kouhei Kaigai 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
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)
On Thu, Apr 30, 2015 at 9:16 AM, Kouhei Kaigai 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)
> On Sun, Apr 26, 2015 at 10:00 PM, Kouhei Kaigai 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 expec
Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
On Sun, Apr 26, 2015 at 10:00 PM, Kouhei Kaigai 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)
On Mon, Apr 27, 2015 at 5:05 AM, Shigeru HANADA 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)
On Fri, Apr 24, 2015 at 3:08 PM, Shigeru HANADA wrote: > Hi Ashutosh, > > Thanks for the review. > > 2015/04/22 19:28、Ashutosh Bapat のメール： > > 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)
Hi Ashutosh, Thanks for the review. 2015/04/22 19:28、Ashutosh Bapat のメール： > 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)
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
> -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
> > 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
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)
> On Wed, Apr 22, 2015 at 10:48 PM, Kouhei Kaigai 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
--
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 Wed, Apr 22, 2015 at 10:48 PM, Kouhei Kaigai 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)
Hi Ashutosh, Thanks for the review. 2015/04/22 19:28、Ashutosh Bapat のメール： > 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)
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 wrote: > > Robert Haas writes: > >> On Fri, Mar 13, 2015 at 2:31 PM, Tom Lane 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
Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
On Fri, Mar 13, 2015 at 8:02 PM, Tom Lane wrote: > Robert Haas writes: >> On Fri, Mar 13, 2015 at 2:31 PM, Tom Lane 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)
On Tue, Apr 21, 2015 at 10:33 PM, Kouhei Kaigai 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)
On Wed, Mar 25, 2015 at 9:51 PM, Kouhei Kaigai 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)
Hanada-san, > I reviewed the Custom/Foreign join API patch again after writing a patch of > join > push-down support for postgres_fdw. > Thanks for your dedicated jobs, my comments are inline below. > 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. > I think, a preferable way is to reproduce the mergeclause_list by extension itself, rather than pass it as a hook argument, because it is uncertain whether CSP should follow "enable_mergejoin" parameter even if it implements a logic like merge-join. Of course, it needs to expose select_mergejoin_clauses. It seems to me a straight- forward way. > 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. > I adjusted the documentation stuff as follows: A custom scan provider will be also able to add paths by setting the following hook, to add CustomPath nodes that perform as if built-in join logic doing. It is typically expected to take two input relations then generate a joined output stream, or just scans preliminaty joined relations like materialized-view. This hook is called next to the consideration of core join logics, then planner will choose the best path to run the relations join in the built-in and custom ones. Probably, it can introduce what this hook works correctly. v12 patch updated only this portion. > * Add new FDW API GetForeignJoinPaths in make_join_rel > This FDW API is intended to provide a chance to add ForeignPaths for a join > relation. > This is called only once for a join relation, so FDW should consider reversed > combination if it’s meaningful in their own mechanisms. > > Note that this is called only when the join relation was *NOT* found in the > PlannerInfo, to avoid redundant calls. > Yep, it is designed according to the discussion upthreads. It can produce N-way remote join paths even if intermediate join relation is more expensive than local join + two foreign scan. > Parameters seems enough for postgres_fdw to process N-way join on remote side > with pushing down join conditions and remote filters. > You ensured it clearly. > * Treat scanrelid == 0 as pseudo scan > A foreign/custom join is represented by a scan against a pseudo relation, i.e. > result of a join. Usually Scan has valid scanrelid, oid of a relation being > scanned, and many functions assume that it’s always valid. The patch adds > another > code paths for scanrelid == 0 as custom/foreign join scans. > Right, > * Pseudo scan target list support > CustomScan and ForeignScan have csp_ps_tlist and fdw_ps_tlist respectively, > for > column reference tracking. A scan generated for custom/foreign join would > have > column from multiple relations in its target list, i.e. output columns. > Ordinary > scans have all valid columns of the relation as output, so references to them > can be resolved easily, but we need an additional mechanism to determine where > a reference in a target list of custom/foreign scan come from. This is very > similar to what IndexOnlyScan does, so we reuse INDEX_VAR as mark of an > indirect > reference to another relation’s var. > Right, FDW/CSP driver is responsible to set *_ps_tlist to inform the core planner which columns of relations are referenced, and which attr
Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
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 のメール： >>> 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 jo
Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
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 > -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 のメール： > > > 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)
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 > -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 のメール： > >> 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, > > -¶ms_list, &fdw_ps_tlist, &retrieved_attrs); > > +¶ms_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)
> >> 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, -¶ms_list, &fdw_ps_tlist, &retrieved_attrs); +¶ms_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 > -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 のメール： > > > >> * 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)
KaiGai-san, 2015/04/14 14:04、Kouhei Kaigai のメール： > >> * 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)
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 -- 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 10:48、Kouhei Kaigai のメール： > * 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 -- 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)
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 backe
Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
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 > -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 us
Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
2015/03/26 10:51、Kouhei Kaigai のメール： > 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? OK. > > 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? Seems fine. I’ve fixed my postgres_fdw code to fit the new version, and am working on handling a whole-join-tree. It would be difficult in the 9.5 cycle, but a hook point where we can handle whole joinrel might allow us to optimize a query which accesses multiple parent tables, each is inherited by foreign tables and partitioned with identical join key, by building a path tree which joins sharded tables first, and then union those results. -- 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)
> > 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 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 19:47、Kouhei Kaigai のメール： > > 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 -- 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 のメール： > 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 18:53、Ashutosh Bapat のメール： > > > On Wed, Mar 25, 2015 at 3:14 PM, Shigeru HANADA > 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:09、Kouhei Kaigai のメール： > > >> On Wed, Mar 25, 2015 at 3:14 PM, Shigeru HANADA > 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 -- 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 のメール： > > >>> 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 -- 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 のメール： >> On Wed, Mar 25, 2015 at 3:14 PM, Shigeru HANADA >> 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)
> On Wed, Mar 25, 2015 at 3:14 PM, Shigeru HANADA > 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 -- 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 Wed, Mar 25, 2015 at 3:14 PM, Shigeru HANADA 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)
2015/03/25 12:59、Kouhei Kaigai のメール： >>> 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)
> > 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...) 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. Thanks, -- NEC OSS Promotion Center / PG-Strom Project KaiGai Kohei > -Original Message- > From: Shigeru HANADA [mailto:shigeru.han...@gmail.com] > Sent: Tuesday, March 24, 2015 7:36 PM > To: Kaigai Kouhei(海外 浩平) > Cc: Robert Haas; Tom Lane; Ashutosh Bapat; Thom Brown; > pgsql-hackers@postgreSQL.org > Subject: Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API) > > 2015/03/23 9:12、Kouhei Kaigai のメール： > > > Sorry for my response late. It was not easy to code during business trip. > > > > The attached patch adds a hook for FDW/CSP to replace entire join-subtree > > by a foreign/custom-scan, according to the discussion upthread. > > > > GetForeignJoinPaths handler of FDW is simplified as follows: > > typedef void (*GetForeignJoinPaths_function) (PlannerInfo *root, > >RelOptInfo *joinrel); > > It’s not a critical issue but I’d like to propose to rename > add_joinrel_extra_paths() to add_extra_paths_to_joinrel(), because the latter > would make it more clear that it does extra work in addition to > add_paths_to_joinrel(). > > > It takes PlannerInfo and RelOptInfo of the join-relation to be replaced > > if available. RelOptInfo contains 'relids' bitmap, so FDW driver will be > > able to know the relations to be involved and construct a remote join query. > > However, it is not obvious with RelOptInfo to know how relations are joined. > > > > The function below will help implement FDW driver that support remote join. > > > > List * > > get_joinrel_broken_down(PlannerInfo *root, RelOptInfo *joinrel, > > SpecialJoinInfo **p_sjinfo) > > > > It returns a list of RelOptInfo to be involved in the relations join that > > is represented with 'joinrel', and also set a SpecialJoinInfo on the third > > argument if not inner join. > > In case of inner join, it returns multiple (more than or equal to 2) > > relations to be inner-joined. Elsewhere, it returns two relations and > > a valid SpecialJoinInfo. > > As far as I tested, it works fine for SEMI and ANTI. > # I want dump function of BitmapSet for debugging, as Node has > nodeToString()... > > > 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? > > — > 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/23 9:12、Kouhei Kaigai のメール： > Sorry for my response late. It was not easy to code during business trip. > > The attached patch adds a hook for FDW/CSP to replace entire join-subtree > by a foreign/custom-scan, according to the discussion upthread. > > GetForeignJoinPaths handler of FDW is simplified as follows: > typedef void (*GetForeignJoinPaths_function) (PlannerInfo *root, >RelOptInfo *joinrel); It’s not a critical issue but I’d like to propose to rename add_joinrel_extra_paths() to add_extra_paths_to_joinrel(), because the latter would make it more clear that it does extra work in addition to add_paths_to_joinrel(). > It takes PlannerInfo and RelOptInfo of the join-relation to be replaced > if available. RelOptInfo contains 'relids' bitmap, so FDW driver will be > able to know the relations to be involved and construct a remote join query. > However, it is not obvious with RelOptInfo to know how relations are joined. > > The function below will help implement FDW driver that support remote join. > > List * > get_joinrel_broken_down(PlannerInfo *root, RelOptInfo *joinrel, > SpecialJoinInfo **p_sjinfo) > > It returns a list of RelOptInfo to be involved in the relations join that > is represented with 'joinrel', and also set a SpecialJoinInfo on the third > argument if not inner join. > In case of inner join, it returns multiple (more than or equal to 2) > relations to be inner-joined. Elsewhere, it returns two relations and > a valid SpecialJoinInfo. As far as I tested, it works fine for SEMI and ANTI. # I want dump function of BitmapSet for debugging, as Node has nodeToString()... > 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? — 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)
> On Wed, Mar 18, 2015 at 9:33 AM, Kouhei Kaigai wrote: > >> On Wed, Mar 18, 2015 at 2:34 AM, Kouhei Kaigai > >> wrote: > >> > So, overall consensus for the FDW hook location is just before the > >> > set_chepest() > >> > at standard_join_search() and merge_clump(), isn't it? > >> > >> Yes, I think so. > >> > >> > Let me make a design of FDW hook to reduce code duplications for each > >> > FDW driver, > >> > especially, to identify baserel/joinrel to be involved in this join. > >> > >> Great, thanks! > >> > >> One issue, which I think Ashutosh alluded to upthread, is that we need > >> to make sure it's not unreasonably difficult for foreign data wrappers > >> to construct the FROM clause of an SQL query to be pushed down to the > >> remote side. It should be simple when there are only inner joins > >> involved, but when there are all outer joins it might be a bit > >> complex. It would be very good if someone could try to write that > >> code, based on the new hook locations, and see how it turns out, so > >> that we can figure out how to address any issues that may crop up > >> there. > >> > > Here is an idea that provides a common utility function that break down > > the supplied RelOptInfo of joinrel into a pair of join-type and a list of > > baserel/joinrel being involved in the relations join. It intends to be > > called by FDW driver to list up underlying relations. > > IIUC, root->join_info_list will provide information of how relations are > > combined to the upper joined relations, thus, I expect it is not > > unreasonably complicated way to solve. > > Once a RelOptInfo of the target joinrel is broken down into multiple sub- > > relations (N>=2 if all inner join, elsewhere N=2), FDW driver can > > reference the RestrictInfo to be used in relations join. > > > > Anyway, I'll try to investigate the existing code for more detail today, > > to clarify whether the above approach is feasible. > > Sounds good. Keep in mind that, while the parse tree will obviously > reflect the way that the user actually specified the join > syntactically, it's not the job of the join_info_list to make it > simple to reconstruct that information. To the contrary, > join_info_list is supposed to be structured in a way that makes it > easy to determine whether *a particular join order is one of the legal > join orders* not *whether it is the specific join order selected by > the user*. See join_is_legal(). > > For FDW pushdown, I think it's sufficient to be able to identify *any > one* legal join order, not necessarily the same order the user > originally entered. For exampe, if the user entered A LEFT JOIN B ON > A.x = B.x LEFT JOIN C ON A.y = C.y and the FDW generates a query that > instead does A LEFT JOIN C ON a.y = C.y LEFT JOIN B ON A.x = B.x, I > suspect that's just fine. Particular FDWs might wish to try to be > smart about what they emit based on knowledge of what the remote > side's optimizer is likely to do, and that's fine. If the remote side > is PostgreSQL, it shouldn't matter much. > Sorry for my response late. It was not easy to code during business trip. The attached patch adds a hook for FDW/CSP to replace entire join-subtree by a foreign/custom-scan, according to the discussion upthread. GetForeignJoinPaths handler of FDW is simplified as follows: typedef void (*GetForeignJoinPaths_function) (PlannerInfo *root, RelOptInfo *joinrel); It takes PlannerInfo and RelOptInfo of the join-relation to be replaced if available. RelOptInfo contains 'relids' bitmap, so FDW driver will be able to know the relations to be involved and construct a remote join query. However, it is not obvious with RelOptInfo to know how relations are joined. The function below will help implement FDW driver that support remote join. List * get_joinrel_broken_down(PlannerInfo *root, RelOptInfo *joinrel, SpecialJoinInfo **p_sjinfo) It returns a list of RelOptInfo to be involved in the relations join that is represented with 'joinrel', and also set a SpecialJoinInfo on the third argument if not inner join. In case of inner join, it returns multiple (more than or equal to 2) relations to be inner-joined. Elsewhere, it returns two relations and a valid SpecialJoinInfo. The #if 0 ... #endif block is just for confirmation purpose to show how hook is invoked and the joinrel is broken down with above service routine. postgres=# select * from t0 left join t1 on t1.aid=bid left join t2 on t1.aid=cid left join t3 on t1.aid=did left join t4 on t1.aid=eid; INFO: LEFT JOIN: t0, t1 INFO: LEFT JOIN: (t0, t1), t2 INFO: LEFT JOIN: (t0, t1), t3 INFO: LEFT JOIN: (t0, t1), t4 INFO: LEFT JOIN: (t0, t1, t3), t2 INFO: LEFT JOIN: (t0, t1, t4), t2 INFO: LEFT JOIN: (t0, t1, t4), t3 INFO: LEFT JOIN: (t0, t1, t3, t4), t2 In this case, joinrel is broken down into (t0, t1, t3
Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
On Wed, Mar 18, 2015 at 9:33 AM, Kouhei Kaigai wrote: >> On Wed, Mar 18, 2015 at 2:34 AM, Kouhei Kaigai wrote: >> > So, overall consensus for the FDW hook location is just before the >> > set_chepest() >> > at standard_join_search() and merge_clump(), isn't it? >> >> Yes, I think so. >> >> > Let me make a design of FDW hook to reduce code duplications for each FDW >> > driver, >> > especially, to identify baserel/joinrel to be involved in this join. >> >> Great, thanks! >> >> One issue, which I think Ashutosh alluded to upthread, is that we need >> to make sure it's not unreasonably difficult for foreign data wrappers >> to construct the FROM clause of an SQL query to be pushed down to the >> remote side. It should be simple when there are only inner joins >> involved, but when there are all outer joins it might be a bit >> complex. It would be very good if someone could try to write that >> code, based on the new hook locations, and see how it turns out, so >> that we can figure out how to address any issues that may crop up >> there. >> > Here is an idea that provides a common utility function that break down > the supplied RelOptInfo of joinrel into a pair of join-type and a list of > baserel/joinrel being involved in the relations join. It intends to be > called by FDW driver to list up underlying relations. > IIUC, root->join_info_list will provide information of how relations are > combined to the upper joined relations, thus, I expect it is not > unreasonably complicated way to solve. > Once a RelOptInfo of the target joinrel is broken down into multiple sub- > relations (N>=2 if all inner join, elsewhere N=2), FDW driver can > reference the RestrictInfo to be used in relations join. > > Anyway, I'll try to investigate the existing code for more detail today, > to clarify whether the above approach is feasible. Sounds good. Keep in mind that, while the parse tree will obviously reflect the way that the user actually specified the join syntactically, it's not the job of the join_info_list to make it simple to reconstruct that information. To the contrary, join_info_list is supposed to be structured in a way that makes it easy to determine whether *a particular join order is one of the legal join orders* not *whether it is the specific join order selected by the user*. See join_is_legal(). For FDW pushdown, I think it's sufficient to be able to identify *any one* legal join order, not necessarily the same order the user originally entered. For exampe, if the user entered A LEFT JOIN B ON A.x = B.x LEFT JOIN C ON A.y = C.y and the FDW generates a query that instead does A LEFT JOIN C ON a.y = C.y LEFT JOIN B ON A.x = B.x, I suspect that's just fine. Particular FDWs might wish to try to be smart about what they emit based on knowledge of what the remote side's optimizer is likely to do, and that's fine. If the remote side is PostgreSQL, it shouldn't matter much. -- 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 Wed, Mar 18, 2015 at 2:34 AM, Kouhei Kaigai wrote: > > So, overall consensus for the FDW hook location is just before the > > set_chepest() > > at standard_join_search() and merge_clump(), isn't it? > > Yes, I think so. > > > Let me make a design of FDW hook to reduce code duplications for each FDW > > driver, > > especially, to identify baserel/joinrel to be involved in this join. > > Great, thanks! > > One issue, which I think Ashutosh alluded to upthread, is that we need > to make sure it's not unreasonably difficult for foreign data wrappers > to construct the FROM clause of an SQL query to be pushed down to the > remote side. It should be simple when there are only inner joins > involved, but when there are all outer joins it might be a bit > complex. It would be very good if someone could try to write that > code, based on the new hook locations, and see how it turns out, so > that we can figure out how to address any issues that may crop up > there. > Here is an idea that provides a common utility function that break down the supplied RelOptInfo of joinrel into a pair of join-type and a list of baserel/joinrel being involved in the relations join. It intends to be called by FDW driver to list up underlying relations. IIUC, root->join_info_list will provide information of how relations are combined to the upper joined relations, thus, I expect it is not unreasonably complicated way to solve. Once a RelOptInfo of the target joinrel is broken down into multiple sub- relations (N>=2 if all inner join, elsewhere N=2), FDW driver can reference the RestrictInfo to be used in relations join. Anyway, I'll try to investigate the existing code for more detail today, to clarify whether the above approach is feasible. Thanks, -- NEC OSS Promotion Center / PG-Strom Project KaiGai Kohei -- 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 Wed, Mar 18, 2015 at 2:34 AM, Kouhei Kaigai wrote: > So, overall consensus for the FDW hook location is just before the > set_chepest() > at standard_join_search() and merge_clump(), isn't it? Yes, I think so. > Let me make a design of FDW hook to reduce code duplications for each FDW > driver, > especially, to identify baserel/joinrel to be involved in this join. Great, thanks! One issue, which I think Ashutosh alluded to upthread, is that we need to make sure it's not unreasonably difficult for foreign data wrappers to construct the FROM clause of an SQL query to be pushed down to the remote side. It should be simple when there are only inner joins involved, but when there are all outer joins it might be a bit complex. It would be very good if someone could try to write that code, based on the new hook locations, and see how it turns out, so that we can figure out how to address any issues that may crop up there. -- 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)
> Robert Haas writes: > > On Tue, Mar 17, 2015 at 10:11 AM, Kouhei Kaigai > > wrote: > >> It might be an idea if foreign-scan path is not wiped out regardless of the > >> estimated cost, we will be able to construct an entirely remote-join path > >> even if intermediation path is expensive than local join. > >> A problem is, how to distinct these special paths from usual paths that are > >> eliminated on the previous stage once its path is more expensive. > > > Any solution that is based on not eliminating paths that would > > otherwise be discarded based on cost seems to me to be unlikely to be > > feasible. We can't complicate the core path-cost-comparison stuff for > > the convenience of FDW or custom-scan pushdown. > > I concur. I'm not even so worried about the cost of add_path as such; > the real problem with not discarding paths as aggressively as possible > is that it will result in a combinatorial explosion in the number of > path combinations that have to be examined at higher join levels. > I'm inclined to agree. It is also conclusion of the discussion with Hanada-san yesterday, due to the number of paths to be considered and combination problems, as you mentioned above. So, overall consensus for the FDW hook location is just before the set_chepest() at standard_join_search() and merge_clump(), isn't it? Let me make a design of FDW hook to reduce code duplications for each FDW driver, especially, to identify baserel/joinrel to be involved in this join. Thanks, -- NEC OSS Promotion Center / PG-Strom Project KaiGai Kohei -- 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 writes: > On Tue, Mar 17, 2015 at 10:11 AM, Kouhei Kaigai wrote: >> It might be an idea if foreign-scan path is not wiped out regardless of the >> estimated cost, we will be able to construct an entirely remote-join path >> even if intermediation path is expensive than local join. >> A problem is, how to distinct these special paths from usual paths that are >> eliminated on the previous stage once its path is more expensive. > Any solution that is based on not eliminating paths that would > otherwise be discarded based on cost seems to me to be unlikely to be > feasible. We can't complicate the core path-cost-comparison stuff for > the convenience of FDW or custom-scan pushdown. I concur. I'm not even so worried about the cost of add_path as such; the real problem with not discarding paths as aggressively as possible is that it will result in a combinatorial explosion in the number of path combinations that have to be examined at higher join levels. 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)
On Tue, Mar 17, 2015 at 8:34 PM, Robert Haas wrote: > On Tue, Mar 17, 2015 at 10:11 AM, Kouhei Kaigai > wrote: > >> A way to work around this is to leave the ForeignPaths (there can > possibly be > >> only one foreign path per join relation) in the joinrel without > removing them. > >> FDW should work on joining two relations if they have foreign paths in > the list > >> of paths, irrespective of whether the cheapest path is foreign join > path or not. > >> For the topmost joinrel, if the foreign path happens to be the cheapest > one, the > >> whole join tree will be pushed down. > >> > >> On the other thread implementing foreign join for postgres_fdw, > >> postgresGetForeignJoinPaths(), is just looking at the cheapest path, > which would > >> cause the problem you have described above. > >> > > It might be an idea if foreign-scan path is not wiped out regardless of > the > > estimated cost, we will be able to construct an entirely remote-join path > > even if intermediation path is expensive than local join. > > A problem is, how to distinct these special paths from usual paths that > are > > eliminated on the previous stage once its path is more expensive. > > Any solution that is based on not eliminating paths that would > otherwise be discarded based on cost seems to me to be unlikely to be > feasible. We can't complicate the core path-cost-comparison stuff for > the convenience of FDW or custom-scan pushdown. > > We already have a precedence here. We cache different cheapest paths e.g 439 struct Path *cheapest_startup_path; 440 struct Path *cheapest_total_path; 441 struct Path *cheapest_unique_path; 442 List *cheapest_parameterized_paths; All we have to do is add yet another there "cheapest_foreign_path" which can be NULL like cheapest_unique_path. > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
On Tue, Mar 17, 2015 at 10:11 AM, Kouhei Kaigai wrote: >> A way to work around this is to leave the ForeignPaths (there can possibly be >> only one foreign path per join relation) in the joinrel without removing >> them. >> FDW should work on joining two relations if they have foreign paths in the >> list >> of paths, irrespective of whether the cheapest path is foreign join path or >> not. >> For the topmost joinrel, if the foreign path happens to be the cheapest one, >> the >> whole join tree will be pushed down. >> >> On the other thread implementing foreign join for postgres_fdw, >> postgresGetForeignJoinPaths(), is just looking at the cheapest path, which >> would >> cause the problem you have described above. >> > It might be an idea if foreign-scan path is not wiped out regardless of the > estimated cost, we will be able to construct an entirely remote-join path > even if intermediation path is expensive than local join. > A problem is, how to distinct these special paths from usual paths that are > eliminated on the previous stage once its path is more expensive. Any solution that is based on not eliminating paths that would otherwise be discarded based on cost seems to me to be unlikely to be feasible. We can't complicate the core path-cost-comparison stuff for the convenience of FDW or custom-scan pushdown. -- 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 Sat, Mar 14, 2015 at 3:48 AM, Robert Haas wrote: > > > On Fri, Mar 13, 2015 at 2:31 PM, Tom Lane wrote: > > Robert Haas writes: > >> Another bit of this that I think we could commit without fretting > >> about it too much is the code adding set_join_pathlist_hook. This > is > >> - I think - analogous to set_rel_pathlist_hook, and like that hook, > >> could be used for other purposes than custom plan generation - e.g. > to > >> delete paths we do not want to use. I've extracted this portion of > >> the patch and adjusted the comments; if there are no objections, I > >> will commit this bit also. > > > > 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. It's true that if your > goal is to delete paths, it's probably best to be called just once > after the path list is complete, and there might be a use case for > that, but I guess it's less useful than for baserels. For a baserel, > as long as you don't nuke the sequential-scan path, there is always > going to be a way to complete the plan; so this would be a fine way to > implement a disable-an-index extension. But for joinrels, it's not so > easy to rule out, say, a hash-join here. Neither hook placement is > much good for that; the path you want to get rid of may have already > dominated paths you want to keep. > > Suppose you want to add paths - e.g. you have an extension that goes > and looks for a materialized view that matches this subtree of the > query, and if it finds one, it substitutes a scan of the materialized > view for a scan of the baserel. Or, as in KaiGai's case, you have an > extension that can perform the whole join in GPU-land and produce the > same results we would have gotten via normal execution. Either way, > you want - and this is the central point of the whole patch here - to > inject a scan path into a joinrel. It is not altogether obvious to me > what the best placement for this is. In the materialized view case, > you probably need a perfect match between the baserels in the view and > the baserels in the joinrel to do anything. There's no point in > re-checking that for every innerrels/outerrels combination. I don't > know enough about the GPU case to reason about it intelligently; maybe > KaiGai can comment. > > I think the foreign data wrapper join pushdown case, which also aims > to substitute a scan for a join, is interesting to think about, even > though it's likely to be handled by a new FDW method instead of via > the hook. Where should the FDW method get called from? Currently, > the FDW method in KaiGai's patch is GetForeignJoinPaths, and that gets > called from add_paths_to_joinrel(). The patch at > http://www.postgresql.org/message-id/CAEZqfEfy7p=uRpwN-Q-NNgzb8kwHbf > qf82ysb9ztfzg7zn6...@mail.gmail.com > uses that to implement join pushdown in postgres_fdw; if you have A > JOIN B JOIN C all on server X, we'll notice that the join with A and B > can be turned into a foreign scan on A JOIN B, and similarly for A-C > and B-C. Then, if it turns out that the cheapest path for A-B is the > foreign join, and the cheapest path for C is a foreign scan, we'll > arrive at the idea of a foreign scan on A-B-C, and we'll realize the > same thing in each of the other combinations as well. So, eventually > the foreign join gets pushed down. > > But there's another possible approach: suppose that > join_search_one_level, after considering left-sided and right-sided > joins and after considering bushy joins, checks whether every relation > it's got is from the same foreign server, and if so, asks that foreign > server whether it would like to contribute any paths. Would that be > better or worse? A disadvantage is that if you've got something like > A LEFT JOIN B LEFT JOIN C LEFT JOIN D LEFT JOIN E LEFT JOIN F LEFT > JOIN G LEFT JOIN H LEFT JOIN I but none of the joins can be pushed > down (say, each join clause calls a non-pushdown-safe function) you'll > end up examining a pile of joinrels - at every level of the join tree > - and individually rejecting each one. With the > build-it-up-incrementally approach, you'll figure t
Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
On Tue, Mar 17, 2015 at 10:28 AM, Shigeru Hanada wrote: > 2015-03-14 7:18 GMT+09:00 Robert Haas : > > I think the foreign data wrapper join pushdown case, which also aims > > to substitute a scan for a join, is interesting to think about, even > > though it's likely to be handled by a new FDW method instead of via > > the hook. Where should the FDW method get called from? Currently, > > the FDW method in KaiGai's patch is GetForeignJoinPaths, and that gets > > called from add_paths_to_joinrel(). The patch at > > > http://www.postgresql.org/message-id/CAEZqfEfy7p=urpwn-q-nngzb8kwhbfqf82ysb9ztfzg7zn6...@mail.gmail.com > > uses that to implement join pushdown in postgres_fdw; if you have A > > JOIN B JOIN C all on server X, we'll notice that the join with A and B > > can be turned into a foreign scan on A JOIN B, and similarly for A-C > > and B-C. Then, if it turns out that the cheapest path for A-B is the > > foreign join, and the cheapest path for C is a foreign scan, we'll > > arrive at the idea of a foreign scan on A-B-C, and we'll realize the > > same thing in each of the other combinations as well. So, eventually > > the foreign join gets pushed down. > > From the viewpoint of postgres_fdw, incremental approach seemed > natural way, although postgres_fdw should consider paths in pathlist > in additon to cheapest one as you mentioned in another thread. This > approarch allows FDW to use SQL statement generated for underlying > scans as parts of FROM clause, as postgres_fdw does in the join > push-down patch. > > > But there's another possible approach: suppose that > > join_search_one_level, after considering left-sided and right-sided > > joins and after considering bushy joins, checks whether every relation > > it's got is from the same foreign server, and if so, asks that foreign > > server whether it would like to contribute any paths. Would that be > > better or worse? A disadvantage is that if you've got something like > > A LEFT JOIN B LEFT JOIN C LEFT JOIN D LEFT JOIN E LEFT JOIN F LEFT > > JOIN G LEFT JOIN H LEFT JOIN I but none of the joins can be pushed > > down (say, each join clause calls a non-pushdown-safe function) you'll > > end up examining a pile of joinrels - at every level of the join tree > > - and individually rejecting each one. With the > > build-it-up-incrementally approach, you'll figure that all out at > > level 2, and then after that there's nothing to do but give up > > quickly. On the other hand, I'm afraid the incremental approach might > > miss a trick: consider small LEFT JOIN (big INNER JOIN huge ON big.x = > > huge.x) ON small.y = big.y AND small.z = huge.z, where all three are > > foreign tables on the same server. If the output of the big/huge join > > is big, none of those paths are going to survive at level 2, but the > > overall join size might be very small, so we surely want a chance to > > recover at level 3. (We discussed test cases of this form quite a bit > > in the context of e2fa76d80ba571d4de8992de6386536867250474.) > > Interesting, I overlooked that pattern. As you pointed out, join > between big foregin tables might be dominated, perhaps by a MergeJoin > path. Leaving dominated ForeignPath in pathlist for more optimization > in the future (in higher join level) is an idea, but it would make > planning time longer (and use more cycle and memory). > > Tom's idea sounds good for saving the path b), but I worry that > whether FDW can get enough information at that timing, just before > set_cheapest. It would not be good I/F if each FDW needs to copy many > code form joinrel.c... > > Even I have the same concern. A simple joinrel doesn't contain much information about the individual two way joins involved in it, so FDW may not be able to construct a query (or execution plan) and hence judge whether a join is pushable or not, just by looking at the joinrel. There will be a lot of code duplication to reconstruct that information, within the FDW code. > -- > 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 > -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
On Sat, Mar 14, 2015 at 3:48 AM, Robert Haas wrote: > On Fri, Mar 13, 2015 at 2:31 PM, Tom Lane wrote: > > Robert Haas writes: > >> Another bit of this that I think we could commit without fretting > >> about it too much is the code adding set_join_pathlist_hook. This is > >> - I think - analogous to set_rel_pathlist_hook, and like that hook, > >> could be used for other purposes than custom plan generation - e.g. to > >> delete paths we do not want to use. I've extracted this portion of > >> the patch and adjusted the comments; if there are no objections, I > >> will commit this bit also. > > > > 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. It's true that if your > goal is to delete paths, it's probably best to be called just once > after the path list is complete, and there might be a use case for > that, but I guess it's less useful than for baserels. For a baserel, > as long as you don't nuke the sequential-scan path, there is always > going to be a way to complete the plan; so this would be a fine way to > implement a disable-an-index extension. But for joinrels, it's not so > easy to rule out, say, a hash-join here. Neither hook placement is > much good for that; the path you want to get rid of may have already > dominated paths you want to keep. > > Suppose you want to add paths - e.g. you have an extension that goes > and looks for a materialized view that matches this subtree of the > query, and if it finds one, it substitutes a scan of the materialized > view for a scan of the baserel. Or, as in KaiGai's case, you have an > extension that can perform the whole join in GPU-land and produce the > same results we would have gotten via normal execution. Either way, > you want - and this is the central point of the whole patch here - to > inject a scan path into a joinrel. It is not altogether obvious to me > what the best placement for this is. In the materialized view case, > you probably need a perfect match between the baserels in the view and > the baserels in the joinrel to do anything. There's no point in > re-checking that for every innerrels/outerrels combination. I don't > know enough about the GPU case to reason about it intelligently; maybe > KaiGai can comment. > > I think the foreign data wrapper join pushdown case, which also aims > to substitute a scan for a join, is interesting to think about, even > though it's likely to be handled by a new FDW method instead of via > the hook. Where should the FDW method get called from? Currently, > the FDW method in KaiGai's patch is GetForeignJoinPaths, and that gets > called from add_paths_to_joinrel(). The patch at > > http://www.postgresql.org/message-id/CAEZqfEfy7p=urpwn-q-nngzb8kwhbfqf82ysb9ztfzg7zn6...@mail.gmail.com > uses that to implement join pushdown in postgres_fdw; if you have A > JOIN B JOIN C all on server X, we'll notice that the join with A and B > can be turned into a foreign scan on A JOIN B, and similarly for A-C > and B-C. Then, if it turns out that the cheapest path for A-B is the > foreign join, and the cheapest path for C is a foreign scan, we'll > arrive at the idea of a foreign scan on A-B-C, and we'll realize the > same thing in each of the other combinations as well. So, eventually > the foreign join gets pushed down. > > But there's another possible approach: suppose that > join_search_one_level, after considering left-sided and right-sided > joins and after considering bushy joins, checks whether every relation > it's got is from the same foreign server, and if so, asks that foreign > server whether it would like to contribute any paths. Would that be > better or worse? A disadvantage is that if you've got something like > A LEFT JOIN B LEFT JOIN C LEFT JOIN D LEFT JOIN E LEFT JOIN F LEFT > JOIN G LEFT JOIN H LEFT JOIN I but none of the joins can be pushed > down (say, each join clause calls a non-pushdown-safe function) you'll > end up examining a pile of joinrels - at every level of the join tree > - and individually rejecting each one. With the > build-it-up-incrementally approach, you'll figure that all out at > level 2, and then after that there's nothing to do but give up > quickly. On the other hand, I'm afraid the incremental approach might > miss a trick: consider small LEFT JOIN (big INNER JOIN huge ON big.x = > huge.x) ON small.y = big.y AND small.z = huge.z, where all three are > foreign tables on the same server. If the output of the big/huge join > is big, none of those paths are going to survive at lev
