On Fri, Sep 25, 2015 at 8:50 AM, Robert Haas <robertmh...@gmail.com> wrote: > > On Thu, Sep 24, 2015 at 2:31 PM, Amit Kapila <amit.kapil...@gmail.com> wrote: > > I have fixed most of the review comments raised in this mail > > as well as other e-mails and rebased the patch on commit- > > 020235a5. Even though I have fixed many of the things, but > > still quite a few comments are yet to be handled. This patch > > is mainly a rebased version to ease the review. We can continue > > to have discussion on the left over things and I will address > > those in consecutive patches. > > Thanks for the update. Here are some more review comments: > > 1. parallel_seqscan_degree is still not what we should have here. As > previously mentioned, I think we should have a GUC for the maximum > degree of parallelism in a query generally, not the maximum degree of > parallel sequential scan. >
Agreed and upthread I have asked about your opinion after proposing a suitable name. Some suitable names could be: degree_of_parallelism, max_degree_of_parallelism, degree_of_query_ parallelism, max_degree_of_query_parallelism > 2. fix_node_funcids() can go away because of commit > 9f1255ac859364a86264a67729dbd1a36dd63ff2. > Agreed. > 3. cost_patialseqscan is still misspelled. I pointed this out before, too. > In the latest patch (parallel_seqscan_partialseqscan_v18.patch) posted by me yesterday, this was fixed. Am I missing something or by any chance you are referring to wrong version of patch > 4. Much more seriously than any of the above, > create_parallelscan_paths() generates plans that are badly broken: > > rhaas=# explain select * from pgbench_accounts where filler < random()::text; > QUERY PLAN > ----------------------------------------------------------------------------------------- > Funnel on pgbench_accounts (cost=0.00..35357.73 rows=3333333 width=97) > Filter: ((filler)::text < (random())::text) > Number of Workers: 10 > -> Partial Seq Scan on pgbench_accounts (cost=0.00..35357.73 > rows=3333333 width=97) > Filter: ((filler)::text < (random())::text) > (5 rows) > > This is wrong both because random() is parallel-restricted and thus > can't be executed in a parallel worker, and also because enforcing a > volatile qual twice is no good. > Yes, the patch needs more work in terms of dealing with parallel-restricted expressions/functions. One idea which I have explored previously is push down only safe clauses to workers (via partialseqscan node) and execute restricted clauses in master (via Funnel node). My analysis is as follows: Usage of restricted functions in quals- During create_plan() phase, separate out the quals that needs to be executed at funnel node versus quals that needs to be executed on partial seq scan node (do something similar to what is done in create_indexscan_plan for index and non-index quals). Basically PartialSeqScan node can contain two different list of quals, one for non-restrictive quals and other for restrictive quals and then Funnel node can retrieve restrictive quals from partialseqscan node, assuming partialseqscan node is its left child. Now, I think the above can only be possible under the assumption that partialseqscan node is always a left child of funnel node, however that is not true because a gating node (Result node) can be added between the two and tomorrow there could be more cases when other nodes will be added between the two, if we consider the case of aggregation, the situation will be more complex as before partial aggregation, all the quals should be executed. Unless there is a good way to achieve the partial execution of quals, I think it is better to prohibit parallel plans, if there is any restrictive clause. Yet another way could be don't push qual if it contains restricted functions and execute it at Funnel node, but I think that will be quite costly due additional flow of tuples from worker backends. Usage of restricted functions in target list - One way could be if target list contains any restricted function, then parallel worker needs to always send the complete tuple and the target list will be evaluated by master backend during funnel node execution. I think some of restrictions that are applies to quals apply to this also especially if there are other nodes like aggregation in-between Funnel and partialseqscan. > rhaas=# explain select * from pgbench_accounts where aid % 10000 = 0; > QUERY PLAN > --------------------------------------------------------------------------------------- > Funnel on pgbench_accounts (cost=0.00..28539.55 rows=50000 width=97) > Filter: ((aid % 10000) = 0) > Number of Workers: 10 > -> Partial Seq Scan on pgbench_accounts (cost=0.00..28539.55 > rows=50000 width=97) > Filter: ((aid % 10000) = 0) > (5 rows) > > This will work, but it's a bad plan because we shouldn't need to > re-apply the filter condition in the parallel leader after we've > already checked it in the worker. > It's only shown in Explain plan, but it never executes quals at funnel node. This needs to be changed and the change will depend on previous case. > rhaas=# explain select * from pgbench_accounts a where a.bid not in > (select bid from pgbench_branches); > QUERY PLAN > ------------------------------------------------------------------------------------------- > Funnel on pgbench_accounts a (cost=2.25..26269.07 rows=5000000 width=97) > Filter: (NOT (hashed SubPlan 1)) > Number of Workers: 10 > -> Partial Seq Scan on pgbench_accounts a (cost=2.25..26269.07 > rows=5000000 width=97) > Filter: (NOT (hashed SubPlan 1)) > SubPlan 1 > -> Seq Scan on pgbench_branches (cost=0.00..2.00 rows=100 width=4) > SubPlan 1 > -> Seq Scan on pgbench_branches (cost=0.00..2.00 rows=100 width=4) > (9 rows) > > This will not work, because the subplan isn't available inside the > worker. Trying it without the EXPLAIN crashes the server. This is > more or less the same issue as one of the known issues you already > mentioned, but I mention it again here because I think this case is > closely related to the previous two. > > Basically, you need to have some kind of logic for deciding which > things need to go below the funnel and which on the funnel itself. > The stuff that's safe should get pushed down, and the stuff that's not > safe should get attached to the funnel. The unsafe stuff is whatever > contains references to initplans or subplans, or anything that > contains parallel-restricted functions ... and there might be some > other stuff, too, but at least those things. > Yes, another thing as we discussed previously that needs to be prohibited is nested-funnel nodes. > Instead of preventing initplans or subplans from getting pushed down > into the funnel, we could instead try to teach the system to push them > down. However, that's very complicated; e.g. a subplan that > references a CTE isn't safe to push down, and a subplan that > references another subplan must be pushed down if that other subplan > is pushed down, and an initplan that contains volatile functions can't > be pushed down because each worker would execute it separately and > they might not all get the same answer, and an initplan that > references a temporary table can't be pushed down because it can't be > referenced from a worker. All in all, it seems better not to go there > right now. > As mentioned in my previous mail, I think initplans should work as we are executing them in master backend, however handling of subplans needs more thoughts. > Now, when you fix this, you're quickly going to run into another > problem, which is that as you have this today, the funnel node does > not actually enforce its filter condition, so the EXPLAIN plan is a > dastardly lie. And when you try to fix that, you're going to run into > a third problem, which is that the stuff the funnel node needs in > order to evaluate its filter condition might not be in the partial seq > scan's target list. So you need to fix both of those problems, too. > Even if you cheat and just don't generate a parallel path at all > except when all quals can be pushed down, you're still going to have > to fix these problems: it's not OK to print out a filter condition on > the funnel as if you were going to enforce it, and then not actually > enforce it there. > Agreed and fix depends on what we decide to do for un-safe quals. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
