On Tue, Feb 23, 2021 at 10:53 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Tue, Feb 23, 2021 at 4:47 PM Greg Nancarrow <gregn4...@gmail.com> wrote: > > > > On Tue, Feb 23, 2021 at 2:30 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > > > On Tue, Feb 23, 2021 at 6:37 AM Greg Nancarrow <gregn4...@gmail.com> > > > wrote: > > > > > > > > On Tue, Feb 23, 2021 at 12:33 AM Amit Kapila <amit.kapil...@gmail.com> > > > > wrote: > > > > > > > > > > On Mon, Feb 22, 2021 at 8:41 AM Greg Nancarrow <gregn4...@gmail.com> > > > > > wrote: > > > > > > > > > > > > On Fri, Feb 19, 2021 at 9:38 PM Amit Kapila > > > > > > <amit.kapil...@gmail.com> wrote: > > > > > > > > > > > > > > On Thu, Feb 18, 2021 at 11:05 AM Amit Langote > > > > > > > <amitlangot...@gmail.com> wrote: > > > > > > > > > > > > > > > > > > It also occurred to me that we can avoid pointless adding of > > > > > > > > > > partitions if the final plan won't use parallelism. For > > > > > > > > > > that, the > > > > > > > > > > patch adds checking glob->parallelModeNeeded, which seems > > > > > > > > > > to do the > > > > > > > > > > trick though I don't know if that's the correct way of > > > > > > > > > > doing that. > > > > > > > > > > > > > > > > > > > > > > > > > > > > I'm not sure if's pointless adding partitions even in the > > > > > > > > > case of NOT > > > > > > > > > using parallelism, because we may be relying on the result of > > > > > > > > > parallel-safety checks on partitions in both cases. > > > > > > > > > For example, insert_parallel.sql currently includes a test > > > > > > > > > (that you > > > > > > > > > originally provided in a previous post) that checks a > > > > > > > > > non-parallel > > > > > > > > > plan is generated after a parallel-unsafe trigger is created > > > > > > > > > on a > > > > > > > > > partition involved in the INSERT. > > > > > > > > > If I further add to that test by then dropping that trigger > > > > > > > > > and then > > > > > > > > > again using EXPLAIN to see what plan is generated, then I'd > > > > > > > > > expect a > > > > > > > > > parallel-plan to be generated, but with the setrefs-v3.patch > > > > > > > > > it still > > > > > > > > > generates a non-parallel plan. So I think the "&& > > > > > > > > > glob->parallelModeNeeded" part of test needs to be removed. > > > > > > > > > > > > > > > > Ah, okay, I didn't retest my case after making that change. > > > > > > > > > > > > > > > > > > > > > > Greg has point here but I feel something on previous lines > > > > > > > (having a > > > > > > > test of glob->parallelModeNeeded) is better. We only want to > > > > > > > invalidate the plan if the prepared plan is unsafe to execute next > > > > > > > time. It is quite possible that there are unsafe triggers on > > > > > > > different > > > > > > > partitions and only one of them is dropped, so next time planning > > > > > > > will > > > > > > > again yield to the same non-parallel plan. If we agree with that I > > > > > > > think it is better to add this dependency in set_plan_refs (along > > > > > > > with > > > > > > > Gather node handling). > > > > > > > > > > > > > > > > > > > I think we should try to be consistent with current plan-cache > > > > > > functionality, rather than possibly inventing new rules for > > > > > > partitions. > > > > > > I'm finding that with current Postgres functionality (master > > > > > > branch), > > > > > > if there are two parallel-unsafe triggers defined on a normal table > > > > > > and one is dropped, it results in replanning and it yields the same > > > > > > (non-parallel) plan. > > > > > > > > > > > > > > > > Does such a plan have partitions access in it? Can you share the plan? > > > > > > > > > > > > > Er, no (it's just a regular table), but that was exactly my point: > > > > aren't you suggesting functionality for partitions that doesn't seem > > > > to work the same way for non-partitions? > > > > > > > > > > I don't think so. The non-parallel plan for Insert doesn't directly > > > depend on partitions so we don't need to invalidate those. > > > > > > > But the non-parallel plan was chosen (instead of a parallel plan) > > because of parallel-safety checks on the partitions, which found > > attributes of the partitions which weren't parallel-safe. > > So it's not so clear to me that the dependency doesn't exist - the > > non-parallel plan does in fact depend on the state of the partitions. > > > > Hmm, I think that is not what we can consider as a dependency. >
Then if it's not a dependency, then we shouldn't have to check the attributes of the partitions for parallel-safety, to determine whether we must use a non-parallel plan (or can use a parallel plan). Except, of course, we do have to ... > > I know you're suggesting to reduce plan-cache invalidation by only > > recording a dependency in the parallel-plan case, but I've yet to see > > that in the existing code, and in fact it seems to be inconsistent > > with the behaviour I've tested so far (one example given prior, but > > will look for more). > > > > I don't see your example matches what you are saying as in it the > regular table exists in the plan whereas for the case we are > discussing partitions doesn't exist in the plan. Amit L. seems to have > given a correct explanation [1] of why we don't need to invalidate for > non-parallel plans which match my understanding. > > [1] - > https://www.postgresql.org/message-id/CA%2BHiwqFKJfzgBbkg0i0Fz_FGsCiXW-Fw0tBjdsaUbNbpyv0JhA%40mail.gmail.com > Amit L's explanation was: I may be wrong but it doesn't seem to me that the possibility of constructing a better plan due to a given change is enough reason for plancache.c to invalidate plans that depend on that change. AIUI, plancache.c only considers a change interesting if it would *break* a Query or a plan. So in this case, a non-parallel plan may be slower, but it isn't exactly rendered *wrong* by changes that make a parallel plan possible. This explanation doesn't seem to match existing planner behavior AFAICS, and we should try to be consistent with existing behavior (rather than doing something different, for partitions specifically in this case). Using a concrete example, to avoid any confusion, consider the following SQL (using unpatched Postgres, master branch): -- encourage use of parallel plans, where possible set parallel_setup_cost=0; set parallel_tuple_cost=0; set min_parallel_table_scan_size=0; set max_parallel_workers_per_gather=4; create or replace function myfunc() returns boolean as $$ begin return true; end; $$ language plpgsql parallel unsafe immutable; create table mytable(x int); insert into mytable(x) values(generate_series(1,10)); prepare q as select * from mytable, myfunc(); explain execute q; -- change myfunc to be parallel-safe, to see the effect -- on the planner for the same select create or replace function myfunc() returns boolean as $$ begin return true; end; $$ language plpgsql parallel safe immutable; explain execute q; Here a function referenced in the SELECT statement is changed from parallel-unsafe to parallel-safe, to see the effect on plancache. According to your referenced explanation, that shouldn't be considered an "interesting" change by plancache.c, as it wouldn't "break" the previously planned and cached (non-parallel) query - the existing non-parallel plan could and should be used, as it still would execute OK, even if slower. Right? BUT - for the above example, it DOES cause the query to be replanned, and it then uses a parallel plan, instead of keeping the original non-parallel plan. This does not match the explanation about how plancache works. Output is below: test=# \i test.sql -- encourage use of parallel plans, where possible set parallel_setup_cost=0; SET set parallel_tuple_cost=0; SET set min_parallel_table_scan_size=0; SET set max_parallel_workers_per_gather=4; SET create or replace function myfunc() returns boolean as $$ begin return true; end; $$ language plpgsql parallel unsafe immutable; CREATE FUNCTION create table mytable(x int); CREATE TABLE insert into mytable(x) values(generate_series(1,10)); INSERT 0 10 prepare q as select * from mytable, myfunc(); PREPARE explain execute q; QUERY PLAN ----------------------------------------------------------- Seq Scan on mytable (cost=0.00..35.50 rows=2550 width=5) (1 row) -- change myfunc to be parallel-safe, to see the effect -- on the planner for the same select create or replace function myfunc() returns boolean as $$ begin return true; end; $$ language plpgsql parallel safe immutable; CREATE FUNCTION explain execute q; QUERY PLAN ------------------------------------------------------------------------- Gather (cost=0.00..18.23 rows=2550 width=5) Workers Planned: 3 -> Parallel Seq Scan on mytable (cost=0.00..18.23 rows=823 width=5) (3 rows) So what I am saying is why should this behavior be different for our partition case? Doing something different for partitions would be inconsistent with this behavior, would it not? Going back to my original example, I pointed out that if there are two parallel-unsafe triggers defined on a normal table and one is dropped, it results in replanning and it yields the same (non-parallel) plan. If the 2nd trigger is dropped, it also results in replanning (and with patch applied, will yield a parallel plan). And with the functionality you are suggesting (only add partition OIDs as dependencies in the case of using a parallel plan), the behavior for partitions (as opposed to a normal table) will be different. Drop one or two of the parallel-unsafe triggers and it won't result in replanning and will still use the original non-parallel plan. So why should the behavior for partitions be different here? Regards, Greg Nancarrow Fujitsu Australia