Thread: [HACKERS] Inadequate parallel-safety check for SubPlans
While poking at the question of parallel_safe marking for Plans, I noticed that max_parallel_hazard_walker() does this: /* We can push the subplans only if they are parallel-safe. */ else if (IsA(node, SubPlan)) return !((SubPlan*) node)->parallel_safe; This is 100% wrong. It's failing to recurse into the subexpressions of the SubPlan, which could very easily include parallel-unsafe function calls. Example: regression=# explain select * from tenk1 where int4(unique1 + random()) not in (select ten from tenk2); QUERY PLAN -----------------------------------------------------------------------------Gather (cost=470.00..884.25 rows=5000 width=244) Workers Planned: 4 -> Parallel Seq Scan on tenk1 (cost=470.00..884.25 rows=1250 width=244) Filter: (NOT(hashed SubPlan 1)) SubPlan 1 -> Seq Scan on tenk2 (cost=0.00..445.00 rows=10000 width=4) random() is parallel-restricted so it's not cool that the SubPlan was allowed to be passed down to workers. I tried to fix it like this: else if (IsA(node, SubPlan)) { if (!((SubPlan *) node)->parallel_safe && max_parallel_hazard_test(PROPARALLEL_RESTRICTED,context)) return true; } but got some failures in the regression tests due to things not getting parallelized when expected. Poking into that, I remembered that for some SubPlans, the testexpr contains Params representing the output columns of the sub-select. So the recursive call of max_parallel_hazard_walker visits those and deems the expression parallel-restricted. Basically, therefore, this logic is totally inadequate. I think what we need to do is improve matters so that while looking at the testexpr of a SubPlan, we pass down a whitelist saying that the Params having the numbers indicated by the SubLink's paramIds list are OK. I'm also suspicious that the existing dumb treatment of Params here may be blocking recognition that correlated subplans are parallel-safe. But that would only be a failure to apply a possible optimization, not a failure to generate a valid plan. regards, tom lane
On Wed, Apr 12, 2017 at 2:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > While poking at the question of parallel_safe marking for Plans, > I noticed that max_parallel_hazard_walker() does this: > > /* We can push the subplans only if they are parallel-safe. */ > else if (IsA(node, SubPlan)) > return !((SubPlan *) node)->parallel_safe; > > This is 100% wrong. It's failing to recurse into the subexpressions of > the SubPlan, which could very easily include parallel-unsafe function > calls. My understanding (apparently flawed?) is that the parallel_safe flag on the SubPlan is supposed to encapsulate whether than SubPlan is entirely parallel safe, so that no recursion is needed. If the parallel_safe flag on the SubPlan is being set wrong, doesn't that imply that the Path from which the subplan was created had the wrong value of this flag, too? ... Oh, I see. The testexpr is separate from the Path. Oops. > Basically, therefore, this logic is totally inadequate. I think what > we need to do is improve matters so that while looking at the testexpr > of a SubPlan, we pass down a whitelist saying that the Params having > the numbers indicated by the SubLink's paramIds list are OK. Sounds reasonable. Do you want to have a go at that? > I'm also suspicious that the existing dumb treatment of Params here > may be blocking recognition that correlated subplans are parallel-safe. > But that would only be a failure to apply a possible optimization, > not a failure to generate a valid plan. Smarter treatment of Params would be very nice, but possibly hard. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > On Wed, Apr 12, 2017 at 2:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> This is 100% wrong. It's failing to recurse into the subexpressions of >> the SubPlan, which could very easily include parallel-unsafe function >> calls. > My understanding (apparently flawed?) is that the parallel_safe flag > on the SubPlan is supposed to encapsulate whether than SubPlan is > entirely parallel safe, so that no recursion is needed. If the > parallel_safe flag on the SubPlan is being set wrong, doesn't that > imply that the Path from which the subplan was created had the wrong > value of this flag, too? > ... > Oh, I see. The testexpr is separate from the Path. Oops. Right. Although you're nudging up against an alternate idea that I just had: we could define the parallel_safe flag on the SubPlan as encapsulating not only whether the child plan is safe, but whether the contained testexpr (and args) are safe. If we were to make an is_parallel_safe() check on the testexpr before injecting PARAM_EXEC Params into it, and record the results in the SubPlan, maybe that would lead to a simpler answer. OTOH, that might not work out nicely; and I have a feeling that we'd end up needing a whitelist anyway later, to handle the case of correlated subplans. > Sounds reasonable. Do you want to have a go at that? Yeah. I'm knocking off for the day a bit early today, but I'll have a look at it tomorrow, unless anyone in the Far East beats me to it. regards, tom lane
On Thu, Apr 13, 2017 at 1:05 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Wed, Apr 12, 2017 at 2:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> This is 100% wrong. It's failing to recurse into the subexpressions of >>> the SubPlan, which could very easily include parallel-unsafe function >>> calls. > >> My understanding (apparently flawed?) is that the parallel_safe flag >> on the SubPlan is supposed to encapsulate whether than SubPlan is >> entirely parallel safe, so that no recursion is needed. If the >> parallel_safe flag on the SubPlan is being set wrong, doesn't that >> imply that the Path from which the subplan was created had the wrong >> value of this flag, too? > >> ... > >> Oh, I see. The testexpr is separate from the Path. Oops. > > Right. Although you're nudging up against an alternate idea that > I just had: we could define the parallel_safe flag on the SubPlan as > encapsulating not only whether the child plan is safe, but whether > the contained testexpr (and args) are safe. If we were to make > an is_parallel_safe() check on the testexpr before injecting > PARAM_EXEC Params into it, and record the results in the SubPlan, > maybe that would lead to a simpler answer. > > OTOH, that might not work out nicely; and I have a feeling that > we'd end up needing a whitelist anyway later, to handle the > case of correlated subplans. > The problem with supporting correlated subplans is that it is not easy to decide about params that belong to whitelist because we need to ensure that such params are available below the gather node. It is quite clear how whitelist of params can be useful for the case in hand but it is not immediately clear to me how we will use it for correlated sublans. However, if you have ideas on how to make it work, I can try to draft a patch as well on those lines as it can certainly help some TPC-H queries. >> Sounds reasonable. Do you want to have a go at that? > > Yeah. I'm knocking off for the day a bit early today, but I'll have > a look at it tomorrow, unless anyone in the Far East beats me to it. > Thanks for looking into it. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Wed, Apr 12, 2017 at 02:41:09PM -0400, Tom Lane wrote: > While poking at the question of parallel_safe marking for Plans, > I noticed that max_parallel_hazard_walker() does this: > > /* We can push the subplans only if they are parallel-safe. */ > else if (IsA(node, SubPlan)) > return !((SubPlan *) node)->parallel_safe; > > This is 100% wrong. It's failing to recurse into the subexpressions of > the SubPlan, which could very easily include parallel-unsafe function > calls. Example: > > regression=# explain select * from tenk1 where int4(unique1 + random()) not in (select ten from tenk2); > QUERY PLAN > ----------------------------------------------------------------------------- > Gather (cost=470.00..884.25 rows=5000 width=244) > Workers Planned: 4 > -> Parallel Seq Scan on tenk1 (cost=470.00..884.25 rows=1250 width=244) > Filter: (NOT (hashed SubPlan 1)) > SubPlan 1 > -> Seq Scan on tenk2 (cost=0.00..445.00 rows=10000 width=4) > > random() is parallel-restricted so it's not cool that the SubPlan was > allowed to be passed down to workers. > > I tried to fix it like this: > > else if (IsA(node, SubPlan)) > { > if (!((SubPlan *) node)->parallel_safe && > max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context)) > return true; > } > > but got some failures in the regression tests due to things not getting > parallelized when expected. Poking into that, I remembered that for some > SubPlans, the testexpr contains Params representing the output columns > of the sub-select. So the recursive call of max_parallel_hazard_walker > visits those and deems the expression parallel-restricted. > > Basically, therefore, this logic is totally inadequate. I think what > we need to do is improve matters so that while looking at the testexpr > of a SubPlan, we pass down a whitelist saying that the Params having > the numbers indicated by the SubLink's paramIds list are OK. The above-described topic is currently a PostgreSQL 10 open item. Robert, since you committed the patch believed to have created it, you own this open item. If some other commit is more relevant or if this does not belong as a v10 open item, please let us know. Otherwise, I will look for an update according to your current blanket status update[1]: I will begin investigating this no later than April 24th, my first day back from vacation, and will provide a further updateby that same day. Thanks. [1] https://www.postgresql.org/message-id/CA+Tgmoa1-529KFwdB-+A1eG2MFc3j9eqJenBUFU=MsT-H9Q8BQ@mail.gmail.com
Noah Misch <noah@leadboat.com> writes: > On Wed, Apr 12, 2017 at 02:41:09PM -0400, Tom Lane wrote: >> This is 100% wrong. It's failing to recurse into the subexpressions of >> the SubPlan, which could very easily include parallel-unsafe function >> calls. Example: > The above-described topic is currently a PostgreSQL 10 open item. Robert, > since you committed the patch believed to have created it, you own this open > item. If some other commit is more relevant or if this does not belong as a > v10 open item, please let us know. FYI, I have this on my to-look-at list, and expect to fix it before Robert returns from vacation. regards, tom lane
On Sun, Apr 16, 2017 at 9:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Noah Misch <noah@leadboat.com> writes: >> On Wed, Apr 12, 2017 at 02:41:09PM -0400, Tom Lane wrote: >>> This is 100% wrong. It's failing to recurse into the subexpressions of >>> the SubPlan, which could very easily include parallel-unsafe function >>> calls. Example: > >> The above-described topic is currently a PostgreSQL 10 open item. Robert, >> since you committed the patch believed to have created it, you own this open >> item. If some other commit is more relevant or if this does not belong as a >> v10 open item, please let us know. > > FYI, I have this on my to-look-at list, and expect to fix it before Robert > returns from vacation. > Let me know if an initial patch by someone else can be helpful? I was not completely sure whether we need to pass any sort of whitelist of params for parallel safety, but if you think that is the better way to go, I can give it a try. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Amit Kapila <amit.kapila16@gmail.com> writes: > On Sun, Apr 16, 2017 at 9:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> FYI, I have this on my to-look-at list, and expect to fix it before Robert >> returns from vacation. > Let me know if an initial patch by someone else can be helpful? Sure, have a go at it. I won't get to this for a day or so ... > I was not completely sure whether we need to pass any sort of whitelist > of params for parallel safety, but if you think that is the better way > to go, I can give it a try. There's certainly more than one way to do it. regards, tom lane
On Mon, Apr 17, 2017 at 1:24 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Amit Kapila <amit.kapila16@gmail.com> writes: >> On Sun, Apr 16, 2017 at 9:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> FYI, I have this on my to-look-at list, and expect to fix it before Robert >>> returns from vacation. > >> Let me know if an initial patch by someone else can be helpful? > > Sure, have a go at it. I won't get to this for a day or so ... Thanks to both of you. I appreciate the help. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Apr 17, 2017 at 10:54 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Amit Kapila <amit.kapila16@gmail.com> writes: >> On Sun, Apr 16, 2017 at 9:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> FYI, I have this on my to-look-at list, and expect to fix it before Robert >>> returns from vacation. > >> Let me know if an initial patch by someone else can be helpful? > > Sure, have a go at it. I won't get to this for a day or so ... > I have ended up doing something along the lines suggested by you (or at least what I have understood from your e-mail). Basically, pass the safe-param-ids list to parallel safety function and decide based on that if Param node reference in input expression is safe. Note that I have added a new parameter paramids list to build_subplan to make AlternativeSubPlan case consistent with SubPlan case even though we should be able to do without that as it seems to be exercised only for the correlated EXISTS case. Also, I think we don't need to check parallel-safety for splan->args as that also is related to correlated queries for which parallelism is not supported as of now. I have also explored it to do it by checking parallel-safety of testexpr before PARAM_EXEC params are replaced in testexpr, however that won't work because we add PARAM_SUBLINK params at the time of parse-analysis in transformSubLink(). I am not sure if it is a good idea to test parallel-safety of testexpr at the time of parse-analysis phase, so didn't pursue that path. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Amit Kapila <amit.kapila16@gmail.com> writes: > I have ended up doing something along the lines suggested by you (or > at least what I have understood from your e-mail). Basically, pass > the safe-param-ids list to parallel safety function and decide based > on that if Param node reference in input expression is safe. I did not like changing the signature of is_parallel_safe() for this: that's getting a lot of call sites involved in something they don't care about, and I'm not even sure that it's formally correct. The key thing here is that a Param could only be considered parallel-safe in context. That is, if you looked at the testexpr alone and asked if it could be pushed down to a worker, the answer would have to be "no". Only when you look at the whole SubPlan tree and ask if it can be pushed down should the answer be "yes". Therefore, I feel pretty strongly that what we want is for the safe_param_ids whitelist to be mutated internally in is_parallel_safe() as it descends the tree. That way it can give the right answer when considering a fragment of a plan. I've committed a patch that does it like that. > ... Also, I think we don't need to check > parallel-safety for splan->args as that also is related to correlated > queries for which parallelism is not supported as of now. I think leaving that sort of thing out is just creating a latent bug that is certain to bite you later. It's true that as long as the args list contains only Vars, it would never be parallel-unsafe --- but primnodes.h is pretty clear that one shouldn't assume that it will stay that way. So it's better to recurse over the whole tree rather than ignoring parts of it, especially if you're not going to document the assumption you're making anywhere. > I have also explored it to do it by checking parallel-safety of > testexpr before PARAM_EXEC params are replaced in testexpr, however > that won't work because we add PARAM_SUBLINK params at the time of > parse-analysis in transformSubLink(). AFAICS we don't do parallel-safety checks early enough to need to bother with that, so the existing handling of SubLink and PARAM_SUBLINK params should be fine. regards, tom lane
On Wed, Apr 19, 2017 at 1:27 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Amit Kapila <amit.kapila16@gmail.com> writes: >> I have ended up doing something along the lines suggested by you (or >> at least what I have understood from your e-mail). Basically, pass >> the safe-param-ids list to parallel safety function and decide based >> on that if Param node reference in input expression is safe. > > I did not like changing the signature of is_parallel_safe() for this: > that's getting a lot of call sites involved in something they don't care > about, and I'm not even sure that it's formally correct. The key thing > here is that a Param could only be considered parallel-safe in context. > That is, if you looked at the testexpr alone and asked if it could be > pushed down to a worker, the answer would have to be "no". Only when > you look at the whole SubPlan tree and ask if it can be pushed down > should the answer be "yes". Therefore, I feel pretty strongly that > what we want is for the safe_param_ids whitelist to be mutated > internally in is_parallel_safe() as it descends the tree. That way > it can give the right answer when considering a fragment of a plan. > I've committed a patch that does it like that. > Thanks. >> ... Also, I think we don't need to check >> parallel-safety for splan->args as that also is related to correlated >> queries for which parallelism is not supported as of now. > > I think leaving that sort of thing out is just creating a latent bug > that is certain to bite you later. It's true that as long as the args > list contains only Vars, it would never be parallel-unsafe --- but > primnodes.h is pretty clear that one shouldn't assume that it will > stay that way. Sure, but the point I was trying to make was whenever subplan has args, I think it won't be parallel-safe as those args are used to pass params. > So it's better to recurse over the whole tree rather > than ignoring parts of it, especially if you're not going to document > the assumption you're making anywhere. > No problem. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Amit Kapila <amit.kapila16@gmail.com> writes: > On Wed, Apr 19, 2017 at 1:27 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I think leaving that sort of thing out is just creating a latent bug >> that is certain to bite you later. It's true that as long as the args >> list contains only Vars, it would never be parallel-unsafe --- but >> primnodes.h is pretty clear that one shouldn't assume that it will >> stay that way. > Sure, but the point I was trying to make was whenever subplan has > args, I think it won't be parallel-safe as those args are used to pass > params. Right now, yes, but surely we're going to be trying to relax that sometime soon. And at that point it would be a latent bug for this function to not recurse into the args list. Whatever the restrictions are on the tree as a whole, they'll apply to that subtree too. regards, tom lane