Thread: [HACKERS] parallelize queries containing subplans
Currently, queries that have references to SubPlans or AlternativeSubPlans are considered parallel-restricted. I think we can lift this restriction in many cases especially when SubPlans are parallel-safe. To make this work, we need to propagate the parallel-safety information from path node to plan node and the same could be easily done while creating a plan. Another option could be that instead of propagating parallel-safety information from path to plan, we can find out from the plan if it is parallel-safe (doesn't contain any parallel-aware node) by traversing whole plan tree, but I think it is a waste of cycles. Once we have parallel-safety information in the plan, we can use that for detection of parallel-safe expressions in max_parallel_hazard_walker(). Finally, we can pass all the subplans to workers during plan serialization in ExecSerializePlan(). This will enable workers to execute subplans that are referred in parallel part of the plan. Now, we might be able to optimize it such that we pass only subplans that are referred in parallel portion of plan, but I am not sure if it is worth the trouble because it is one-time cost and much lesser than other things we do (like creating dsm, launching workers). Attached patch implements the above idea. This will enable parallelism for queries containing un-correlated subplans, an example of which is as follows: set parallel_tuple_cost=0; set parallel_setup_cost=0; set min_parallel_relation_size=50; create table t1 (i int, j int, k int); create table t2 (i int, j int, k int); insert into t1 values (generate_series(1,10)*random(), generate_series(5,50)*random(), generate_series(8,80)*random()); insert into t2 values (generate_series(4,10)*random(), generate_series(5,90)*random(), generate_series(2,10)*random()); Plan without Patch ----------------------------- postgres=# explain select * from t1 where t1.i not in (select t2.i from t2 where t2.i in (1,2,3)); QUERY PLAN --------------------------------------------------------------- Seq Scan on t1 (cost=110.84..411.72 rows=8395 width=12) Filter: (NOT (hashed SubPlan 1)) SubPlan 1 -> Seq Scan on t2 (cost=0.00..104.50 rows=2537 width=4) Filter: (i = ANY ('{1,2,3}'::integer[])) (5 rows) Plan with Patch ------------------------ postgres=# explain select * from t1 where t1.i not in (select t2.i from t2 where t2.i in (1,2,3)); QUERY PLAN ------------------------------------------------------------------------- Gather (cost=110.84..325.30 rows=8395 width=12) Workers Planned: 1 -> Parallel Seq Scan on t1 (cost=110.84..325.30 rows=4938 width=12) Filter: (NOT (hashed SubPlan 1)) SubPlan 1 -> Seq Scan on t2 (cost=0.00..104.50 rows=2537 width=4) Filter: (i = ANY ('{1,2,3}'::integer[])) (7 rows) We have observed that Q-16 in TPC-H have been improved with the patch and the analysis of same will be shared by my colleague Rafia. Now, we can further extend this to parallelize queries containing correlated subplans like below: explain select * from t1 where t1.i in (select t2.i from t2 where t2.i=t1.i); QUERY PLAN ------------------------------------------------------------- Seq Scan on t1 (cost=0.00..831049.09 rows=8395 width=12) Filter: (SubPlan 1) SubPlan 1 -> Seq Scan on t2 (cost=0.00..97.73 rows=493 width=4) Filter: (i = t1.i) (5 rows) In the above query, Filter on t2 (i = t1.i) generates Param node which is a parallel-restricted node, so such queries won't be able to use parallelism even with the patch. I think we can mark Params which refer to same level as parallel-safe and I think we have this information (node-> varlevelsup/ phlevelsup/ agglevelsup) available when we replace correlation vars (SS_replace_correlation_vars). The reason why it is not advisable to mark Params that don't refer to same query level as parallel-safe is that can lead to plans like below: Foo -> Gather -> Bar SubPlan 1 (SubPlan refers to Foo) Now, this problem is tricky because we need to pass all such Params each time we invoke Gather. That also is doable with much more effort, but I am not sure if it is worth because all of the use cases I have seen in TPC-H (Q-2) or TPC-DS (Q-6) always uses SubPlans that refer to same query level. Yet, another useful enhancement in this area could be to consider both parallel and non-parallel paths for subplans. As of now, we consider the cheapest/best path and form subplan from it, but it is quite possible that instead of choosing parallel path (in case it is cheapest) at subplan level, the non-parallel path at subplan level could be beneficial when upper plan can use parallelism. I think this will be a separate project in itself if we want to do this and based on my study of TPC-H and TPC-DS queries, I am confident that this will be helpful in certain queries at higher scale factors. Thoughts? -- 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
On Wed, Dec 28, 2016 at 11:47 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: > > Currently, queries that have references to SubPlans or > AlternativeSubPlans are considered parallel-restricted. I think we > can lift this restriction in many cases especially when SubPlans are > parallel-safe. To make this work, we need to propagate the > parallel-safety information from path node to plan node and the same > could be easily done while creating a plan. Another option could be > that instead of propagating parallel-safety information from path to > plan, we can find out from the plan if it is parallel-safe (doesn't > contain any parallel-aware node) by traversing whole plan tree, but I > think it is a waste of cycles. Once we have parallel-safety > information in the plan, we can use that for detection of > parallel-safe expressions in max_parallel_hazard_walker(). Finally, > we can pass all the subplans to workers during plan serialization in > ExecSerializePlan(). This will enable workers to execute subplans > that are referred in parallel part of the plan. Now, we might be able > to optimize it such that we pass only subplans that are referred in > parallel portion of plan, but I am not sure if it is worth the trouble > because it is one-time cost and much lesser than other things we do > (like creating > dsm, launching workers). > > Attached patch implements the above idea. This will enable > parallelism for queries containing un-correlated subplans, an example > of which is as follows: > > set parallel_tuple_cost=0; > set parallel_setup_cost=0; > set min_parallel_relation_size=50; > > create table t1 (i int, j int, k int); > create table t2 (i int, j int, k int); > > insert into t1 values (generate_series(1,10)*random(), > generate_series(5,50)*random(), > generate_series(8,80)*random()); > insert into t2 values (generate_series(4,10)*random(), > generate_series(5,90)*random(), > generate_series(2,10)*random()); > > > Plan without Patch > ----------------------------- > postgres=# explain select * from t1 where t1.i not in (select t2.i > from t2 where t2.i in (1,2,3)); > QUERY PLAN > --------------------------------------------------------------- > Seq Scan on t1 (cost=110.84..411.72 rows=8395 width=12) > Filter: (NOT (hashed SubPlan 1)) > SubPlan 1 > -> Seq Scan on t2 (cost=0.00..104.50 rows=2537 width=4) > Filter: (i = ANY ('{1,2,3}'::integer[])) > (5 rows) > > Plan with Patch > ------------------------ > postgres=# explain select * from t1 where t1.i not in (select t2.i > from t2 where t2.i in (1,2,3)); > QUERY PLAN > ------------------------------------------------------------------------- > Gather (cost=110.84..325.30 rows=8395 width=12) > Workers Planned: 1 > -> Parallel Seq Scan on t1 (cost=110.84..325.30 rows=4938 width=12) > Filter: (NOT (hashed SubPlan 1)) > SubPlan 1 > -> Seq Scan on t2 (cost=0.00..104.50 rows=2537 width=4) > Filter: (i = ANY ('{1,2,3}'::integer[])) > (7 rows) > > We have observed that Q-16 in TPC-H have been improved with the patch > and the analysis of same will be shared by my colleague Rafia. > To study the effect of uncorrelated sub-plan pushdown on TPC-H and TPC-DS benchmark queries we performed some experiments and the execution time results for same are summarised as follows, Query | HEAD | Patches | scale-factor -----------+---------+-----------+----------------- DS-Q45 | 35 | 19 | scale-factor: 100 H-Q16 | 812 | 645 | scale-factor: 300 H-Q16 | 49 | 37 | scale-factor: 20 Execution time given in above table is in seconds. Detailed analysis of this experimentation is as follows, Additional patches applied in this analysis are, Parallel index scan [1] Parallel index-only scan [2] Parallel merge-join [3] The system setup used for this experiment is, Server parameter settings: work_mem = 500 MB, max_parallel_workers_per_gather = 4, random_page_cost = seq_page_cost = 0.1 = parallel_tuple_cost, shared_buffers = 1 GB Machine used: IBM Power, 4 socket machine, 512 GB RAM TPC-DS scale factor = 100 (approx size of database is 150 GB) Query 45 which takes around 35 seconds on head, completes in 19 seconds with these patches. The point to note here is that without this patch of pushing uncorrelated sublans, hash join which is using subplan in join filter could not be pushed to workers and hence query was unable to use the parallelism enough, with this patch parallelism is available till the topmost join node. The output of explain analyse statement of this query on both head and with patches is given in attached file ds_q45.txt. On further evaluating these patches on TPC-H queries on different scale factors we came across query 16, for TPC-H scale factor 20 and aforementioned parameter settings with the change of max_parallel_workers_per gather = 2, it took 37 seconds with the patches and 49 seconds on head. Though the improvement in overall query performance is not appearing to be significantly high, yet the point to note here is that the time taken by join was around 26 seconds on head which reduced to 14 seconds with the patches. Overall benefit in performance is not high because sort node is dominating the execution time. The plan information of this query is given in attached file h_q16_20_2.txt. On increasing the scale factor to 300, setting work_mem to 1GB, increasing max_parallel_workers_per_gather = 6, and disabling the parallel sequential scan for supplier table by 'alter table supplier set (parallel_workers = 0)', Q16 completes in 645 seconds with patches, which was taking 812 seconds on head. We need to disable parallel_workers for supplier table because on higher worker count it was taking parallel seq scan and hence the scan node that is using subplan could not be parallelised. For this query both pushdown of subplans and parallel merge-join, without any one of these the benefits of parallelism might not be leveraged fully. The output of explain analyse for this query is given in h_q16_300.txt Overall, with pushdown of uncorrelated sub-plans to workers enables the parallelism in joins which was restricted before and hence good improvement in query performance can be witnessed. > Now, we can further extend this to parallelize queries containing > correlated subplans like below: > > explain select * from t1 where t1.i in (select t2.i from t2 where t2.i=t1.i); > QUERY PLAN > ------------------------------------------------------------- > Seq Scan on t1 (cost=0.00..831049.09 rows=8395 width=12) > Filter: (SubPlan 1) > SubPlan 1 > -> Seq Scan on t2 (cost=0.00..97.73 rows=493 width=4) > Filter: (i = t1.i) > (5 rows) > As per my analysis this extension to correlated subplans is likely to improve the performance of following queries -- Q2 in TPC-H and Q6 in TPC-DS. > Yet, another useful enhancement in this area could be to consider both > parallel and non-parallel paths for subplans. As of now, we consider > the cheapest/best path and form subplan from it, but it is quite > possible that instead of choosing parallel path (in case it is > cheapest) at subplan level, the non-parallel path at subplan level > could be beneficial when upper plan can use parallelism. I think this > will be a separate project in itself if we want to do this and based > on my study of TPC-H and TPC-DS queries, I am confident that this will > be helpful in certain queries at higher scale factors. > I agree as then we do not need to disable parallelism for particular relations as we currently do for supplier relation in Q16 of TPC-H. [1] https://www.postgresql.org/message-id/CAA4eK1KthrAvNjmB2cWuUHz%2Bp3ZTTtbD7o2KUw49PC8HK4r1Wg%40mail.gmail.com [2] https://www.postgresql.org/message-id/CAOGQiiOv9NA1VrAo8PmENfGi-y%3DCj_DiTF4vyjp%2BfmuEzovwEA%40mail.gmail.com [3] https://www.postgresql.org/message-id/CAFiTN-tdYpcsk7Lpv0HapcmvSnMN_TgKjC7RkmvVLZAF%2BXfmPg%40mail.gmail.com -- Regards, Rafia Sabih 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
On Wed, Dec 28, 2016 at 11:47 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: > > Now, we can further extend this to parallelize queries containing > correlated subplans like below: > > explain select * from t1 where t1.i in (select t2.i from t2 where t2.i=t1.i); > QUERY PLAN > ------------------------------------------------------------- > Seq Scan on t1 (cost=0.00..831049.09 rows=8395 width=12) > Filter: (SubPlan 1) > SubPlan 1 > -> Seq Scan on t2 (cost=0.00..97.73 rows=493 width=4) > Filter: (i = t1.i) > (5 rows) > > In the above query, Filter on t2 (i = t1.i) generates Param node which > is a parallel-restricted node, so such queries won't be able to use > parallelism even with the patch. I think we can mark Params which > refer to same level as parallel-safe and I think we have this > information (node-> varlevelsup/ phlevelsup/ agglevelsup) available > when we replace correlation vars (SS_replace_correlation_vars). > I have implemented the above idea which will allow same or immediate outer level PARAMS as parallel_safe. The results of above query after patch: postgres=# explain select * from t1 where t1.i in (select t2.i from t2 where t2.i=t1.i); QUERY PLAN -------------------------------------------------------------------------- Gather (cost=0.00..488889.88 rows=8395 width=12) Workers Planned: 1 -> Parallel Seq Scan on t1 (cost=0.00..488889.88 rows=4938 width=12) Filter: (SubPlan 1) SubPlan 1 -> Seq Scan on t2 (cost=0.00..97.73 rows=493 width=4) Filter: (i = t1.i) (7 rows) Note - This patch can be applied on top of pq_pushdown_subplan_v1.patch posted upthread [1] [1] - https://www.postgresql.org/message-id/CAA4eK1%2Be8Z45D2n%2BrnDMDYsVEb5iW7jqaCH_tvPMYau%3D1Rru9w%40mail.gmail.com -- 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
On Wed, Dec 28, 2016 at 11:47 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: > Attached patch implements the above idea. This will enable > parallelism for queries containing un-correlated subplans, an example > of which is as follows: I have reviewed the patch (pq_pushdown_subplan_v1.patch), Mostly patch looks clean to me. I have also done some basic functional testing and it's working fine. I have one comment. + else if (IsA(node, AlternativeSubPlan)) + { + AlternativeSubPlan *asplan = (AlternativeSubPlan *) node; + ListCell *lc; + + Assert(context->root); + + foreach(lc, asplan->subplans) + { + SubPlan *splan = (SubPlan *) lfirst(lc); + Plan *plan; + + Assert(IsA(splan, SubPlan)); + + plan = planner_subplan_get_plan(context->root, splan); + if (!plan->parallel_safe) + return true; + } } I see no reason why we need to process the subplan list of AlternativeSubPlan here, Anyway expression_tree_walker will take care of processing each subplan of AlternativeSubPlan. Now in the case where all the subplans in AlternativeSubPlan are parallel_safe we will process this list twice. But, more than that it will be cleaner to not handle AlternativeSubPlan here unless there is some strong reason? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Wed, Dec 28, 2016 at 1:17 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: > Currently, queries that have references to SubPlans or > AlternativeSubPlans are considered parallel-restricted. I think we > can lift this restriction in many cases especially when SubPlans are > parallel-safe. To make this work, we need to propagate the > parallel-safety information from path node to plan node and the same > could be easily done while creating a plan. Another option could be > that instead of propagating parallel-safety information from path to > plan, we can find out from the plan if it is parallel-safe (doesn't > contain any parallel-aware node) by traversing whole plan tree, but I > think it is a waste of cycles. Once we have parallel-safety > information in the plan, we can use that for detection of > parallel-safe expressions in max_parallel_hazard_walker(). Finally, > we can pass all the subplans to workers during plan serialization in > ExecSerializePlan(). This will enable workers to execute subplans > that are referred in parallel part of the plan. Now, we might be able > to optimize it such that we pass only subplans that are referred in > parallel portion of plan, but I am not sure if it is worth the trouble > because it is one-time cost and much lesser than other things we do > (like creating > dsm, launching workers). It seems unfortunate to have to add a parallel_safe flag to the finished plan; the whole reason we have the Path-Plan distinction is so that we can throw away information that won't be needed at execution time. The parallel_safe flag is, in fact, not needed at execution time, but just for further planning. Isn't there some way that we can remember, at the time when a sublink is converted to a subplan, whether or not the subplan was created from a parallel-safe path? That seems like it would be cleaner than maintaining this flag for all plans. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Jan 10, 2017 at 10:55 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Dec 28, 2016 at 1:17 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> Currently, queries that have references to SubPlans or >> AlternativeSubPlans are considered parallel-restricted. I think we >> can lift this restriction in many cases especially when SubPlans are >> parallel-safe. To make this work, we need to propagate the >> parallel-safety information from path node to plan node and the same >> could be easily done while creating a plan. Another option could be >> that instead of propagating parallel-safety information from path to >> plan, we can find out from the plan if it is parallel-safe (doesn't >> contain any parallel-aware node) by traversing whole plan tree, but I >> think it is a waste of cycles. Once we have parallel-safety >> information in the plan, we can use that for detection of >> parallel-safe expressions in max_parallel_hazard_walker(). Finally, >> we can pass all the subplans to workers during plan serialization in >> ExecSerializePlan(). This will enable workers to execute subplans >> that are referred in parallel part of the plan. Now, we might be able >> to optimize it such that we pass only subplans that are referred in >> parallel portion of plan, but I am not sure if it is worth the trouble >> because it is one-time cost and much lesser than other things we do >> (like creating >> dsm, launching workers). > > It seems unfortunate to have to add a parallel_safe flag to the > finished plan; the whole reason we have the Path-Plan distinction is > so that we can throw away information that won't be needed at > execution time. The parallel_safe flag is, in fact, not needed at > execution time, but just for further planning. Isn't there some way > that we can remember, at the time when a sublink is converted to a > subplan, whether or not the subplan was created from a parallel-safe > path? > The other alternative is to remember this information in SubPlan. We can retrieve parallel_safe information from best_path and can use it while generating SubPlan. The main reason for storing it in the plan was to avoid explicitly passing parallel_safe information while generating SubPlan as plan was already available at that time. However, it seems there are only two places in code (refer build_subplan) where this information needs to be propagated. Let me know if you prefer to remember the parallel_safe information in SubPlan instead of in Plan or if you have something else in mind? -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Wed, Jan 11, 2017 at 9:58 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: > The other alternative is to remember this information in SubPlan. We > can retrieve parallel_safe information from best_path and can use it > while generating SubPlan. The main reason for storing it in the plan > was to avoid explicitly passing parallel_safe information while > generating SubPlan as plan was already available at that time. > However, it seems there are only two places in code (refer > build_subplan) where this information needs to be propagated. Let me > know if you prefer to remember the parallel_safe information in > SubPlan instead of in Plan or if you have something else in mind? I think we should try doing it in the SubPlan, at least, and see if that comes out more elegant than what you have at the moment. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Jan 10, 2017 at 11:55 AM, Dilip Kumar <dilipbalaut@gmail.com> wrote: > On Wed, Dec 28, 2016 at 11:47 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> Attached patch implements the above idea. This will enable >> parallelism for queries containing un-correlated subplans, an example >> of which is as follows: > > I have reviewed the patch (pq_pushdown_subplan_v1.patch), Mostly patch > looks clean to me. > I have also done some basic functional testing and it's working fine. > > I have one comment. > > + else if (IsA(node, AlternativeSubPlan)) > + { > + AlternativeSubPlan *asplan = (AlternativeSubPlan *) node; > + ListCell *lc; > + > + Assert(context->root); > + > + foreach(lc, asplan->subplans) > + { > + SubPlan *splan = (SubPlan *) lfirst(lc); > + Plan *plan; > + > + Assert(IsA(splan, SubPlan)); > + > + plan = planner_subplan_get_plan(context->root, splan); > + if (!plan->parallel_safe) > + return true; > + } > } > > I see no reason why we need to process the subplan list of > AlternativeSubPlan here, Anyway expression_tree_walker will take care > of processing each subplan of AlternativeSubPlan. Now in the case > where all the subplans in AlternativeSubPlan are parallel_safe we will > process this list twice. > Valid point, but I think we can avoid that by returning false after foreach(..) loop. I think one improvement could be that instead of manually checking the parallel safety of each subplan, we can recursively call max_parallel_hazard_walker for each subplan. > But, more than that it will be cleaner to not > handle AlternativeSubPlan here unless there is some strong reason? > Yeah, the reason is to avoid unnecessary recursion. Also, in all other places in the code, we do handle both of them separately, refer cost_qual_eval_walker for somewhat similar usage. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Thu, Jan 12, 2017 at 9:22 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: > Valid point, but I think we can avoid that by returning false after > foreach(..) loop. I think one improvement could be that instead of > manually checking the parallel safety of each subplan, we can > recursively call max_parallel_hazard_walker for each subplan. I agree that this way we can avoid the problem what I mentioned. > > >> But, more than that it will be cleaner to not >> handle AlternativeSubPlan here unless there is some strong reason? >> > > Yeah, the reason is to avoid unnecessary recursion. Also, in all > other places in the code, we do handle both of them separately, refer > cost_qual_eval_walker for somewhat similar usage. ok. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Thu, Jan 12, 2017 at 8:51 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Jan 11, 2017 at 9:58 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> The other alternative is to remember this information in SubPlan. We >> can retrieve parallel_safe information from best_path and can use it >> while generating SubPlan. The main reason for storing it in the plan >> was to avoid explicitly passing parallel_safe information while >> generating SubPlan as plan was already available at that time. >> However, it seems there are only two places in code (refer >> build_subplan) where this information needs to be propagated. Let me >> know if you prefer to remember the parallel_safe information in >> SubPlan instead of in Plan or if you have something else in mind? > > I think we should try doing it in the SubPlan, at least, and see if > that comes out more elegant than what you have at the moment. > Okay, done that way. I have fixed the review comments raised by Dilip as well and added the test case in attached patch. -- 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
On Thu, Jan 12, 2017 at 7:56 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: > Okay, done that way. I have fixed the review comments raised by Dilip > as well and added the test case in attached patch. I have tested with pq_pushdown_subplan_v2.patch + pq_pushdown_correl_subplan_v1.patch sqlsmith. is reporting below error, I have attached the query to reproduce the issue. This issue is only coming when both the patch is applied, only with pq_pushdown_subplan_v2.patch there is no problem. So I think the problem is because of pq_pushdown_correl_subplan_v1.patch. ERROR: did not find '}' at end of input node at character 762 -- Regards, Dilip Kumar 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
Dilip Kumar <dilipbalaut@gmail.com> writes: > ERROR: did not find '}' at end of input node at character 762 I've not looked at the patches, but just seeing this error message, this looks like somebody's fat-fingered the correspondence between outfuncs.c and readfuncs.c processing. regards, tom lane
On Fri, Jan 13, 2017 at 7:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Dilip Kumar <dilipbalaut@gmail.com> writes: >> ERROR: did not find '}' at end of input node at character 762 > I could reproduce this error with simple query like: SELECT * FROM information_schema.role_usage_grants WHERE object_type LIKE 'FOREIGN%' AND object_name IN ('s6', 'foo') ORDER BY 1, 2, 3, 4, 5; The reason is that the stored rules have the old structure of Param. See below: {RELABELTYPE :arg {PARAM :paramkind 2 :paramid 1 :paramtype 11975 :paramtypmod -1 :paramcollid 100 :location -1} The new variable parallel_safe is not present in above node. If you recreate the fresh database you won't see the above problem. I have observed a problem in equal function which I have fixed in the attached patch. I have also added a test, so that we can catch any problem similar to what you have reported. > I've not looked at the patches, but just seeing this error message, > this looks like somebody's fat-fingered the correspondence between > outfuncs.c and readfuncs.c processing. > I think what we need is catversion bump as Param is part of stored rules. -- 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
On Thu, Jan 12, 2017 at 7:56 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: > On Thu, Jan 12, 2017 at 8:51 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Wed, Jan 11, 2017 at 9:58 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: >>> The other alternative is to remember this information in SubPlan. We >>> can retrieve parallel_safe information from best_path and can use it >>> while generating SubPlan. The main reason for storing it in the plan >>> was to avoid explicitly passing parallel_safe information while >>> generating SubPlan as plan was already available at that time. >>> However, it seems there are only two places in code (refer >>> build_subplan) where this information needs to be propagated. Let me >>> know if you prefer to remember the parallel_safe information in >>> SubPlan instead of in Plan or if you have something else in mind? >> >> I think we should try doing it in the SubPlan, at least, and see if >> that comes out more elegant than what you have at the moment. >> > > Okay, done that way. I have fixed the review comments raised by Dilip > as well and added the test case in attached patch. > After commit-ab1f0c8, this patch needs a rebase. Attached find rebased version of the patch. Thanks to Kuntal for informing me offlist that this patch needs rebase. -- 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
On Mon, Jan 16, 2017 at 9:13 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: > > After commit-ab1f0c8, this patch needs a rebase. Attached find > rebased version of the patch. > > Thanks to Kuntal for informing me offlist that this patch needs rebase. In this patch, I have observed some changes while creating subplan for CTE. So I have reviewed this from that perspective and also tried to perform some test. Here is my finding/ comments. @@ -1213,6 +1216,7 @@ SS_process_ctes(PlannerInfo *root) &splan->firstColCollation); splan->useHashTable = false; splan->unknownEqFalse= false; + splan->parallel_safe = best_path->parallel_safe; I noticed that if path for CTE is parallel safe then we are marking CTE subplan as parallel safe, In particular, I don't have any problem with that, but can you add some test case which can cover this path, I mean to say where CTE subplan are pushed. ------------ I have tried to test the subplan with CTE below is my test. create table t1(a int , b varchar); create table t (n int, b varchar); Query: explain verbose select * from t where t.n not in (WITH RECURSIVE t(n) AS ( VALUES (1) UNION ALL SELECT a+1 FROM t1 WHEREa < 100 ) SELECT sum(n) FROM t); During debugging I found that subplan created for below part of the query is parallel_unsafe, Is it a problem or there is some explanation of why it's not parallel_safe, (WITH RECURSIVE t(n) AS ( VALUES (1) UNION ALL SELECT a+1 FROM t1 WHERE a < 100 ) SELECT sum(n) FROM t); ---------- -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Thu, Jan 19, 2017 at 3:05 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote: > @@ -1213,6 +1216,7 @@ SS_process_ctes(PlannerInfo *root) > &splan->firstColCollation); > splan->useHashTable = false; > splan->unknownEqFalse = false; > + splan->parallel_safe = best_path->parallel_safe; > > I noticed that if path for CTE is parallel safe then we are marking > CTE subplan as parallel safe, In particular, I don't have any problem > with that, but can you add some test case which can cover this path, I > mean to say where CTE subplan are pushed. > > ------------ > I have tried to test the subplan with CTE below is my test. > create table t1(a int , b varchar); > create table t (n int, b varchar); > > Query: > explain verbose select * from t where t.n not in (WITH RECURSIVE t(n) AS ( > VALUES (1) > UNION ALL > SELECT a+1 FROM t1 WHERE a < 100 > ) > SELECT sum(n) FROM t); > > During debugging I found that subplan created for below part of the > query is parallel_unsafe, Is it a problem or there is some explanation > of why it's not parallel_safe, > > (WITH RECURSIVE t(n) AS ( > VALUES (1) > UNION ALL > SELECT a+1 FROM t1 WHERE a < 100 > ) > SELECT sum(n) FROM t); > ---------- The corresponding plan for the query you have specified is: QUERY PLAN ----------------------------------------------------------------------------------Seq Scan on public.t (cost=40.73..20894.74rows=500480 width=35) Output: t.n, t.b Filter: (NOT (hashed SubPlan 2)) SubPlan 2 -> Aggregate (cost=40.72..40.73 rows=1 width=8) Output: sum(t_1.n) CTE t -> Append (cost=0.00..31.18rows=424 width=4) -> Result (cost=0.00..0.01 rows=1 width=4) Output:1 -> Seq Scan on public.t1 (cost=0.00..26.93 rows=423 width=4) Output: (t1.a + 1) Filter: (t1.a < 100) -> CTEScan on t t_1 (cost=0.00..8.48 rows=424 width=4) Output: t_1.n (15 rows) Now, the plan for CTE is parallel_safe. But, a CTE plan is converted to an InitPlan and returns a Param which is used in the CTE Scan. Since Param is not parallel_safe till now, the SubPlan is also not parallel_safe. This is why CTE subplans will not be pushed under Gather. -- Thanks & Regards, Kuntal Ghosh EnterpriseDB: http://www.enterprisedb.com
On Thu, Jan 19, 2017 at 3:19 PM, Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote: > Since Param is not parallel_safe till now, the SubPlan is also not > parallel_safe. This is why CTE subplans will not be pushed under > Gather. Specifically, Params which are generated using generate_new_param() are not parallel_safe. In the patch, it is marked as parallel-unsafe. -- Thanks & Regards, Kuntal Ghosh EnterpriseDB: http://www.enterprisedb.com
On Thu, Jan 19, 2017 at 3:05 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote: > During debugging I found that subplan created for below part of the > query is parallel_unsafe, Is it a problem or there is some explanation > of why it's not parallel_safe, Okay, so basically we don't have any mechanism to perform parallel scan on CTE. And, IMHO subplan built for CTE (using SS_process_ctes) must come along with CTE scan. So I think we can avoid setting below code because we will never be able to test its side effect, another argument can be that if we don't consider the final effect, and just see this subplan then by logic it should be marked parallel-safe or unsafe as per it's path and it will not have any side effect, as it will finally become parallel-unsafe. So it's your call to keep it either way. @@ -1213,6 +1216,7 @@ SS_process_ctes(PlannerInfo *root) &splan->firstColCollation); splan->useHashTable = false; splan->unknownEqFalse= false; + splan->parallel_safe = best_path->parallel_safe; -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Thu, Jan 19, 2017 at 4:51 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote: > On Thu, Jan 19, 2017 at 3:05 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote: >> During debugging I found that subplan created for below part of the >> query is parallel_unsafe, Is it a problem or there is some explanation >> of why it's not parallel_safe, > > Okay, so basically we don't have any mechanism to perform parallel > scan on CTE. And, IMHO subplan built for CTE (using SS_process_ctes) > must come along with CTE scan. So I think we can avoid setting below > code because we will never be able to test its side effect, another > argument can be that if we don't consider the final effect, and just > see this subplan then by logic it should be marked parallel-safe or > unsafe as per it's path and it will not have any side effect, as it > will finally become parallel-unsafe. So it's your call to keep it > either way. Oops, you're right. We don't consider parallelism for RTE_CTE type. -- Thanks & Regards, Kuntal Ghosh EnterpriseDB: http://www.enterprisedb.com
On Thu, Jan 19, 2017 at 4:51 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote: > On Thu, Jan 19, 2017 at 3:05 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote: >> During debugging I found that subplan created for below part of the >> query is parallel_unsafe, Is it a problem or there is some explanation >> of why it's not parallel_safe, > > Okay, so basically we don't have any mechanism to perform parallel > scan on CTE. And, IMHO subplan built for CTE (using SS_process_ctes) > must come along with CTE scan. So I think we can avoid setting below > code because we will never be able to test its side effect, another > argument can be that if we don't consider the final effect, and just > see this subplan then by logic it should be marked parallel-safe or > unsafe as per it's path and it will not have any side effect, as it > will finally become parallel-unsafe. So it's your call to keep it > either way. > Yeah, actually setting parallel_safety information for subplan from corresponding is okay. However, in this particular case as we know that it might not be of any use till we enable parallelism for CTE Scan (and doing that is certainly not essential for this project). So, I have set parallel_safe as false for CTE subplans. -- 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
On Tue, Jan 24, 2017 at 10:59 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: > On Thu, Jan 19, 2017 at 4:51 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote: >> On Thu, Jan 19, 2017 at 3:05 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote: >>> During debugging I found that subplan created for below part of the >>> query is parallel_unsafe, Is it a problem or there is some explanation >>> of why it's not parallel_safe, >> >> Okay, so basically we don't have any mechanism to perform parallel >> scan on CTE. And, IMHO subplan built for CTE (using SS_process_ctes) >> must come along with CTE scan. So I think we can avoid setting below >> code because we will never be able to test its side effect, another >> argument can be that if we don't consider the final effect, and just >> see this subplan then by logic it should be marked parallel-safe or >> unsafe as per it's path and it will not have any side effect, as it >> will finally become parallel-unsafe. So it's your call to keep it >> either way. >> > > Yeah, actually setting parallel_safety information for subplan from > corresponding is okay. > missed the word *path* in above sentence. /corresponding/corresponding path -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Tue, Jan 24, 2017 at 10:59 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: > On Thu, Jan 19, 2017 at 4:51 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote: >> On Thu, Jan 19, 2017 at 3:05 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote: >>> During debugging I found that subplan created for below part of the >>> query is parallel_unsafe, Is it a problem or there is some explanation >>> of why it's not parallel_safe, >> >> Okay, so basically we don't have any mechanism to perform parallel >> scan on CTE. And, IMHO subplan built for CTE (using SS_process_ctes) >> must come along with CTE scan. So I think we can avoid setting below >> code because we will never be able to test its side effect, another >> argument can be that if we don't consider the final effect, and just >> see this subplan then by logic it should be marked parallel-safe or >> unsafe as per it's path and it will not have any side effect, as it >> will finally become parallel-unsafe. So it's your call to keep it >> either way. >> > > Yeah, actually setting parallel_safety information for subplan from > corresponding is okay. However, in this particular case as we know > that it might not be of any use till we enable parallelism for CTE > Scan (and doing that is certainly not essential for this project). > So, I have set parallel_safe as false for CTE subplans. > Moved this patch to next CF. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Tue, Jan 31, 2017 at 6:01 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: > Moved this patch to next CF. So here is what seems to be the key hunk from this patch: /* - * Since we don't have the ability to push subplans down to workers at - * present, we treat subplan references as parallel-restricted. We need - * not worry about examining their contents; if they are unsafe, we would - * have found that out while examining the whole tree before reduction of - * sublinks to subplans. (Really we should not see SubLink during a - * max_interesting == restricted scan, but if we do, return true.) + * We can push the subplans only if they don't contain any parallel-aware + * node as we don't support multi-level parallelism (parallel workers + * invoking another set of parallel workers). */ - else if (IsA(node, SubLink) || - IsA(node, SubPlan) || - IsA(node, AlternativeSubPlan)) + else if (IsA(node, SubPlan)) + return !((SubPlan *) node)->parallel_safe; + else if (IsA(node, AlternativeSubPlan)) { - if (max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context)) - return true; + AlternativeSubPlan *asplan = (AlternativeSubPlan *) node; + ListCell *lc; + + foreach(lc, asplan->subplans) + { + SubPlan *splan = (SubPlan *) lfirst(lc); + + Assert(IsA(splan, SubPlan)); + + if (max_parallel_hazard_walker((Node *) splan, context)) + return true; + } + + return false; } I don't see the reason for having this new code that makes AlternativeSubPlan walk over the subplans; expression_tree_walker already does that. On the flip side I don't see the reason for removing the max_parallel_hazard_test() call for AlternativeSubPlan or for SubLink. I think that the core of this change is to change the handling for SubPlan nodes to just return the flag from the subplan, and that seems fine, but the rest of this I'm skeptical about. + * CTE scans are not consider for parallelism (cf considered + select count(*)from tenk1 where (two, four) not in whitespace -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Feb 1, 2017 at 10:04 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Jan 31, 2017 at 6:01 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> Moved this patch to next CF. > > So here is what seems to be the key hunk from this patch: > > /* > - * Since we don't have the ability to push subplans down to workers at > - * present, we treat subplan references as parallel-restricted. We need > - * not worry about examining their contents; if they are unsafe, we would > - * have found that out while examining the whole tree before reduction of > - * sublinks to subplans. (Really we should not see SubLink during a > - * max_interesting == restricted scan, but if we do, return true.) > + * We can push the subplans only if they don't contain any parallel-aware > + * node as we don't support multi-level parallelism (parallel workers > + * invoking another set of parallel workers). > */ > - else if (IsA(node, SubLink) || > - IsA(node, SubPlan) || > - IsA(node, AlternativeSubPlan)) > + else if (IsA(node, SubPlan)) > + return !((SubPlan *) node)->parallel_safe; > + else if (IsA(node, AlternativeSubPlan)) > { > - if (max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context)) > - return true; > + AlternativeSubPlan *asplan = (AlternativeSubPlan *) node; > + ListCell *lc; > + > + foreach(lc, asplan->subplans) > + { > + SubPlan *splan = (SubPlan *) lfirst(lc); > + > + Assert(IsA(splan, SubPlan)); > + > + if (max_parallel_hazard_walker((Node *) splan, context)) > + return true; > + } > + > + return false; > } > > I don't see the reason for having this new code that makes > AlternativeSubPlan walk over the subplans; expression_tree_walker > already does that. > It is done this way mainly to avoid recursion/nested calls, but I think you prefer to handle it via expression_tree_walker so that code looks clean. Another probable reason could be that there is always a chance that we miss handling of some expression of a particular node (in this case AlternativeSubPlan), but if that is the case then there are other places in the code which do operate on individual subplan/'s in AlternativeSubPlan list. > On the flip side I don't see the reason for > removing the max_parallel_hazard_test() call for AlternativeSubPlan or > for SubLink. If we don't remove the current test of max_parallel_hazard_test() for AlternativeSubPlan, then AlternativeSubPlan node will be considered parallel restricted, so why do we want that after this patch. For Sublinks, I think they would have converted to Subplans by the time we reach this function for the parallel restricted check. Can you elaborate what you have in mind for the handling of AlternativeSubPlan and SubLink? -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Thu, Feb 2, 2017 at 9:23 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: > On Wed, Feb 1, 2017 at 10:04 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Tue, Jan 31, 2017 at 6:01 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >>> Moved this patch to next CF. >> >> So here is what seems to be the key hunk from this patch: >> >> /* >> - * Since we don't have the ability to push subplans down to workers at >> - * present, we treat subplan references as parallel-restricted. We need >> - * not worry about examining their contents; if they are unsafe, we would >> - * have found that out while examining the whole tree before reduction of >> - * sublinks to subplans. (Really we should not see SubLink during a >> - * max_interesting == restricted scan, but if we do, return true.) >> + * We can push the subplans only if they don't contain any parallel-aware >> + * node as we don't support multi-level parallelism (parallel workers >> + * invoking another set of parallel workers). >> */ >> - else if (IsA(node, SubLink) || >> - IsA(node, SubPlan) || >> - IsA(node, AlternativeSubPlan)) >> + else if (IsA(node, SubPlan)) >> + return !((SubPlan *) node)->parallel_safe; >> + else if (IsA(node, AlternativeSubPlan)) >> { >> - if (max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context)) >> - return true; >> + AlternativeSubPlan *asplan = (AlternativeSubPlan *) node; >> + ListCell *lc; >> + >> + foreach(lc, asplan->subplans) >> + { >> + SubPlan *splan = (SubPlan *) lfirst(lc); >> + >> + Assert(IsA(splan, SubPlan)); >> + >> + if (max_parallel_hazard_walker((Node *) splan, context)) >> + return true; >> + } >> + >> + return false; >> } >> >> I don't see the reason for having this new code that makes >> AlternativeSubPlan walk over the subplans; expression_tree_walker >> already does that. >> I have removed the check of AlternativeSubPlan so that it can be handled by expression_tree_walker. > > It is done this way mainly to avoid recursion/nested calls, but I > think you prefer to handle it via expression_tree_walker so that code > looks clean. Another probable reason could be that there is always a > chance that we miss handling of some expression of a particular node > (in this case AlternativeSubPlan), but if that is the case then there > are other places in the code which do operate on individual subplan/'s > in AlternativeSubPlan list. > >> On the flip side I don't see the reason for >> removing the max_parallel_hazard_test() call for AlternativeSubPlan or >> for SubLink. > > If we don't remove the current test of max_parallel_hazard_test() for > AlternativeSubPlan, then AlternativeSubPlan node will be considered > parallel restricted, so why do we want that after this patch. For > Sublinks, I think they would have converted to Subplans by the time we > reach this function for the parallel restricted check. Can you > elaborate what you have in mind for the handling of AlternativeSubPlan > and SubLink? > I have removed the changes for SubLink node. >> + * CTE scans are not consider for parallelism (cf >> >> >> considered >> Fixed. >> + select count(*)from tenk1 where (two, four) not in >> >> whitespace Fixed. Attached patch fixes all the comments raised till now. -- 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
On Tue, Jan 3, 2017 at 4:19 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: > On Wed, Dec 28, 2016 at 11:47 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> >> Now, we can further extend this to parallelize queries containing >> correlated subplans like below: >> >> explain select * from t1 where t1.i in (select t2.i from t2 where t2.i=t1.i); >> QUERY PLAN >> ------------------------------------------------------------- >> Seq Scan on t1 (cost=0.00..831049.09 rows=8395 width=12) >> Filter: (SubPlan 1) >> SubPlan 1 >> -> Seq Scan on t2 (cost=0.00..97.73 rows=493 width=4) >> Filter: (i = t1.i) >> (5 rows) >> >> In the above query, Filter on t2 (i = t1.i) generates Param node which >> is a parallel-restricted node, so such queries won't be able to use >> parallelism even with the patch. I think we can mark Params which >> refer to same level as parallel-safe and I think we have this >> information (node-> varlevelsup/ phlevelsup/ agglevelsup) available >> when we replace correlation vars (SS_replace_correlation_vars). >> > > I have implemented the above idea which will allow same or immediate > outer level PARAMS as parallel_safe. The results of above query after > patch: > > postgres=# explain select * from t1 where t1.i in (select t2.i from t2 > where t2.i=t1.i); > QUERY PLAN > -------------------------------------------------------------------------- > Gather (cost=0.00..488889.88 rows=8395 width=12) > Workers Planned: 1 > -> Parallel Seq Scan on t1 (cost=0.00..488889.88 rows=4938 width=12) > Filter: (SubPlan 1) > SubPlan 1 > -> Seq Scan on t2 (cost=0.00..97.73 rows=493 width=4) > Filter: (i = t1.i) > (7 rows) > On further evaluation, it seems this patch has one big problem which is that it will allow forming parallel plans which can't be supported with current infrastructure. For ex. marking immediate level params as parallel safe can generate below type of plan: Seq Scan on t1 Filter: (SubPlan 1) SubPlan 1 -> Gather Workers Planned: 1 -> Result One-Time Filter: (t1.k = 0) -> Parallel Seq Scan on t2 In this plan, we can't evaluate one-time filter (that contains correlated param) unless we have the capability to pass all kind of PARAM_EXEC param to workers. I don't want to invest too much time in this patch unless somebody can see some way using current parallel infrastructure to implement correlated subplans. Note that still, the other patch [1] in this thread which implements parallelism for uncorrelated subplan holds good. [1] - https://www.postgresql.org/message-id/CAA4eK1J9mDZLcp-OskkdzAf_yT8W4dBSGL9E%3DkoEiJkdpVZsEA%40mail.gmail.com -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Tue, Feb 14, 2017 at 4:24 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: > On further evaluation, it seems this patch has one big problem which > is that it will allow forming parallel plans which can't be supported > with current infrastructure. For ex. marking immediate level params > as parallel safe can generate below type of plan: > > Seq Scan on t1 > Filter: (SubPlan 1) > SubPlan 1 > -> Gather > Workers Planned: 1 > -> Result > One-Time Filter: (t1.k = 0) > -> Parallel Seq Scan on t2 > > > In this plan, we can't evaluate one-time filter (that contains > correlated param) unless we have the capability to pass all kind of > PARAM_EXEC param to workers. I don't want to invest too much time in > this patch unless somebody can see some way using current parallel > infrastructure to implement correlated subplans. I don't think this approach has much chance of working; it just seems too simplistic. I'm not entirely sure what the right approach is. Unfortunately, the current query planner code seems to compute the sets of parameters that are set and used quite late, and really only on a per-subquery level. Here we need to know whether there is anything that's set below the Gather node and used above it, or the other way around, and we need to know it much earlier, while we're still doing path generation. There doesn't seem to be any simple way of getting that information, but I think you need it. What's more, I think you would still need it even if you had the ability to pass parameter values between processes. For example, consider: Gather -> Parallel Seq Scan Filter: (Correlated Subplan Reference Goes Here) Of course, the Param in the filter condition *can't* be a shared Param across all processes. It needs to be private to each process participating in the parallel sequential scan -- and the params passing data down from the Parallel Seq Scan to the correlated subplan also need to be private. On the other hand, in your example quoted above, you do need to share across processes: the value for t1.k needs to get passed down. So it seems to me that we somehow need to identify, for each parameter that gets used, whether it's provided by something beneath the Gather node (in which case it should be private to the worker) or whether it's provided from higher up (in which case it should be passed down to the worker, or if we can't do that, then don't use parallelism there). (There's also possible a couple of other cases, like an initPlan that needs to get executed only once, and also maybe a case where a parameter is set below the Gather and later used above the Gather. Not sure if that latter one happen, or how to deal with it.) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Feb 13, 2017 at 11:07 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: > I have removed the check of AlternativeSubPlan so that it can be > handled by expression_tree_walker. ... > Attached patch fixes all the comments raised till now. Committed after removing the reference to AlternativeSubPlan from the comment. I didn't see any need to mention that. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Feb 15, 2017 at 4:46 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Feb 13, 2017 at 11:07 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> I have removed the check of AlternativeSubPlan so that it can be >> handled by expression_tree_walker. > ... >> Attached patch fixes all the comments raised till now. > > Committed after removing the reference to AlternativeSubPlan from the > comment. I didn't see any need to mention that. > Okay, I was also of two minds while adding that line in the comment. I had kept it because there are few places in the code where both SubPlan and AlternativeSubPlan are handled together, so I thought the person looking at this code should not wonder why we have not handled AlternativeSubPlan. However, I think it is okay to remove it from comment as there exist other places where only Subplan is handled and we rely on expression tree walker for AlternativeSubPlans. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Wed, Feb 15, 2017 at 4:38 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Feb 14, 2017 at 4:24 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> On further evaluation, it seems this patch has one big problem which >> is that it will allow forming parallel plans which can't be supported >> with current infrastructure. For ex. marking immediate level params >> as parallel safe can generate below type of plan: >> >> Seq Scan on t1 >> Filter: (SubPlan 1) >> SubPlan 1 >> -> Gather >> Workers Planned: 1 >> -> Result >> One-Time Filter: (t1.k = 0) >> -> Parallel Seq Scan on t2 >> >> >> In this plan, we can't evaluate one-time filter (that contains >> correlated param) unless we have the capability to pass all kind of >> PARAM_EXEC param to workers. I don't want to invest too much time in >> this patch unless somebody can see some way using current parallel >> infrastructure to implement correlated subplans. > > I don't think this approach has much chance of working; it just seems > too simplistic. I'm not entirely sure what the right approach is. > Unfortunately, the current query planner code seems to compute the > sets of parameters that are set and used quite late, and really only > on a per-subquery level. > Now just for the sake of discussion consider we have list of allParams at the path level, then also I think it might not be easy to make it work. > Here we need to know whether there is > anything that's set below the Gather node and used above it, or the > other way around, and we need to know it much earlier, while we're > still doing path generation. > Yes, that is exactly the challenge. I am not sure if currently there is a way by which we can identify if a Param on a particular node refers to node below it or above it. > (There's also possible a couple of other cases, like an initPlan that > needs to get executed only once, and also maybe a case where a > parameter is set below the Gather and later used above the Gather. > Not sure if that latter one happen, or how to deal with it.) > I think the case for initPlan is slightly different because we can always evaluate it at Gather (considering it is an uncorrelated initplan) and then pass it to Workers. We generally have a list of all the params at each plan node, so we can identify which of these are initPlan params and then evaluate them. Now, it can be used irrespective of whether it is used above or below the Gather node. For the cases, where it can be used above Gather node, it will work as we always store the computed value of such params in estate/econtext and for the cases when it has to be used below Gather, we need to pass the computed value to workers. Now, there is some exceptions like for few cases not all the params are available at a particular node, but I feel those can be handled easily by either traversing the planstate tree or by actually storing them at Gather node. Actually, in short, this is what is done in the patch proposed for parallizing initplans [1]. [1] - https://commitfest.postgresql.org/13/997/ -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com