Thread
Commits
-
Prevent generation of bogus subquery scan paths.
- dc1057fcd878 11.0 landed
-
Let Parallel Append over simple UNION ALL have partial subpaths.
- 0927d2f46ddd 11.0 cited
-
[sqlsmith] Failed assertion in create_gather_path
Andreas Seltenreich <seltenreich@gmx.de> — 2018-04-07T18:08:57Z
Hi, sqlsmith found a query that triggers the following assertion in master as of 039eb6e92f: TRAP: FailedAssertion("!(subpath->parallel_safe)", File: "pathnode.c", Line: 1813) Backtrace and recipe against the regression database below. regards, Andreas #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51 #1 0x00007f25474cf42a in __GI_abort () at abort.c:89 #2 0x0000556c14b75bb3 in ExceptionalCondition (conditionName=conditionName@entry=0x556c14d16d68 "!(subpath->parallel_safe)", errorType=errorType@entry=0x556c14bc4dbd "FailedAssertion", fileName=fileName@entry=0x556c14d16d43 "pathnode.c", lineNumber=lineNumber@entry=1813) at assert.c:54 #3 0x0000556c149ca01d in create_gather_path (root=root@entry=0x556c16bfb7a0, rel=rel@entry=0x7f253e36f418, subpath=0x7f253e37d9d8, target=0x7f253e36f650, required_outer=required_outer@entry=0x0, rows=rows@entry=0x0) at pathnode.c:1813 #4 0x0000556c1498a3d7 in generate_gather_paths (root=root@entry=0x556c16bfb7a0, rel=rel@entry=0x7f253e36f418, override_rows=override_rows@entry=false) at allpaths.c:2564 #5 0x0000556c1498a7b0 in set_rel_pathlist (root=root@entry=0x556c16bfb7a0, rel=0x7f253e36f418, rti=rti@entry=2, rte=0x556c16bfb420) at allpaths.c:497 #6 0x0000556c1498b09d in set_base_rel_pathlists (root=<optimized out>) at allpaths.c:310 #7 make_one_rel (root=root@entry=0x556c16bfb7a0, joinlist=joinlist@entry=0x7f253e374450) at allpaths.c:180 #8 0x0000556c149abfac in query_planner (root=root@entry=0x556c16bfb7a0, tlist=tlist@entry=0x7f253e3eb4a0, qp_callback=qp_callback@entry=0x556c149acb90 <standard_qp_callback>, qp_extra=qp_extra@entry=0x7ffe8088a200) at planmain.c:259 #9 0x0000556c149b0be5 in grouping_planner (root=root@entry=0x556c16bfb7a0, inheritance_update=inheritance_update@entry=false, tuple_fraction=<optimized out>, tuple_fraction@entry=0) at planner.c:1914 #10 0x0000556c149b31a1 in subquery_planner (glob=glob@entry=0x556c16c234d0, parse=parse@entry=0x556c16bfaeb8, parent_root=parent_root@entry=0x0, hasRecursion=hasRecursion@entry=false, tuple_fraction=tuple_fraction@entry=0) at planner.c:984 #11 0x0000556c149b4356 in standard_planner (parse=0x556c16bfaeb8, cursorOptions=256, boundParams=<optimized out>) at planner.c:405 #12 0x0000556c14a680dd in pg_plan_query (querytree=0x556c16bfaeb8, cursorOptions=256, boundParams=0x0) at postgres.c:808 #13 0x0000556c14a681be in pg_plan_queries (querytrees=<optimized out>, cursorOptions=cursorOptions@entry=256, boundParams=boundParams@entry=0x0) at postgres.c:874 #14 0x0000556c14a686a9 in exec_simple_query ( query_string=0x556c16b2b438 "...") at postgres.c:1049 #15 0x0000556c14a6a341 in PostgresMain (argc=<optimized out>, argv=argv@entry=0x556c16b56ad8, dbname=<optimized out>, username=<optimized out>) at postgres.c:4149 #16 0x0000556c1474eac4 in BackendRun (port=0x556c16b4c030) at postmaster.c:4409 #17 BackendStartup (port=0x556c16b4c030) at postmaster.c:4081 #18 ServerLoop () at postmaster.c:1754 #19 0x0000556c149ec017 in PostmasterMain (argc=3, argv=0x556c16b257d0) at postmaster.c:1362 #20 0x0000556c1475006d in main (argc=3, argv=0x556c16b257d0) at main.c:228 set min_parallel_table_scan_size to 0; select 66 as c0, ref_1.cid as c1, pg_catalog.min( cast((select timetzcol from public.brintest limit 1 offset 3) as timetz)) over (partition by ref_1.name order by ref_1.name) as c2, ref_0.c as c3 from public.prt1_l as ref_0 right join public.my_property_normal as ref_1 on (ref_0.a <= ref_0.a) where EXISTS ( select ref_2.y as c0, ref_2.y as c1, sample_0.random as c2, ref_1.tel as c3, ref_0.a as c4, sample_0.random as c5, ref_2.y as c6, ref_2.x as c7, case when (true <> (select pg_catalog.bool_and(n) from testxmlschema.test2) ) and (sample_0.seqno = (select int_four from public.test_type_diff2_c3 limit 1 offset 1) ) then ref_2.y else ref_2.y end as c8, sample_0.seqno as c9, ref_1.name as c10, ref_0.a as c11, (select nslots from public.hub limit 1 offset 2) as c12, ref_1.name as c13 from public.hash_name_heap as sample_0 tablesample system (8.2) left join public.tt6 as ref_2 on ((((cast(null as tinterval) <= (select f1 from public.tinterval_tbl limit 1 offset 79) ) and (ref_2.y is not NULL)) or (((false) and ((cast(null as tsquery) > (select keyword from public.test_tsquery limit 1 offset 34) ) or ((((select pg_catalog.jsonb_agg(sl_name) from public.shoelace_obsolete) <@ cast(null as jsonb)) or (EXISTS ( select 100 as c0, ref_0.a as c1, sample_0.seqno as c2, ref_0.a as c3, sample_0.seqno as c4, ref_0.a as c5, (select a from public.prt3_n limit 1 offset 30) as c6, ref_2.y as c7, ref_1.cid as c8, ref_2.y as c9 from public.num_exp_mul as sample_1 tablesample system (7.1) where true limit 89))) and (cast(null as _aclitem) @> cast(null as aclitem))))) and ((select timecol from public.brintest limit 1 offset 96) > cast(null as "time")))) and (cast(null as timestamptz) < cast(null as "timestamp"))) where ((EXISTS ( select sample_2.int_four as c0, sample_0.seqno as c1, 43 as c2 from public.test_type_diff2_c1 as sample_2 tablesample bernoulli (2.3) where (sample_0.random ~~ ref_1.name) and (ref_2.y <> ref_2.y) limit 98)) and (sample_0.random is NULL)) and (cast(null as point) <@ (select b from public.quad_box_tbl limit 1 offset 5) ) limit 61); -
Re: [sqlsmith] Failed assertion in create_gather_path
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-04-07T18:43:54Z
Andreas Seltenreich wrote: > Hi, > > sqlsmith found a query that triggers the following assertion in master > as of 039eb6e92f: > > TRAP: FailedAssertion("!(subpath->parallel_safe)", File: "pathnode.c", Line: 1813) > > Backtrace and recipe against the regression database below. Uh, that's pretty strange -- that patch did not touch the planner at all, and the relcache.c changes are pretty trivial. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -
Re: [sqlsmith] Failed assertion in create_gather_path
Tom Lane <tgl@sss.pgh.pa.us> — 2018-04-07T18:46:08Z
Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > Andreas Seltenreich wrote: >> sqlsmith found a query that triggers the following assertion in master >> as of 039eb6e92f: >> TRAP: FailedAssertion("!(subpath->parallel_safe)", File: "pathnode.c", Line: 1813) > Uh, that's pretty strange -- that patch did not touch the planner at > all, and the relcache.c changes are pretty trivial. I don't think Andreas meant that the bug is necessarily new in 039eb6e92f, only that that's the version he happened to be testing. regards, tom lane -
Re: [sqlsmith] Failed assertion in create_gather_path
Andreas Seltenreich <seltenreich@gmx.de> — 2018-04-07T18:56:22Z
Tom Lane writes: > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: >> Andreas Seltenreich wrote: >>> as of 039eb6e92f: >>> TRAP: FailedAssertion("!(subpath->parallel_safe)", File: "pathnode.c", Line: 1813) > >> Uh, that's pretty strange -- that patch did not touch the planner at >> all, and the relcache.c changes are pretty trivial. > > I don't think Andreas meant that the bug is necessarily new in 039eb6e92f, > only that that's the version he happened to be testing. Right. I'm not testing often enough yet to be able to report on commit granularity :-). I'll try for a less ambiguos wording in future reports. -
Re: [sqlsmith] Failed assertion in create_gather_path
Jeevan Chalke <jeevan.chalke@enterprisedb.com> — 2018-04-08T07:34:02Z
Hi, At some places, I have observed that we are adding a partial path even when rel's consider_parallel is false. Due to this, the partial path added has parallel_safe set to false and then later in create_gather_path() assertion fails. Attached patch to fix that. On Sun, Apr 8, 2018 at 12:26 AM, Andreas Seltenreich <seltenreich@gmx.de> wrote: > Tom Lane writes: > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > >> Andreas Seltenreich wrote: > >>> as of 039eb6e92f: > >>> TRAP: FailedAssertion("!(subpath->parallel_safe)", File: > "pathnode.c", Line: 1813) > > > >> Uh, that's pretty strange -- that patch did not touch the planner at > >> all, and the relcache.c changes are pretty trivial. > > > > I don't think Andreas meant that the bug is necessarily new in > 039eb6e92f, > > only that that's the version he happened to be testing. > > Right. I'm not testing often enough yet to be able to report on commit > granularity :-). I'll try for a less ambiguos wording in future > reports. > > -- Jeevan Chalke Technical Architect, Product Development EnterpriseDB Corporation The Enterprise PostgreSQL Company -
Re: [sqlsmith] Failed assertion in create_gather_path
Amit Kapila <amit.kapila16@gmail.com> — 2018-04-09T12:22:49Z
On Sun, Apr 8, 2018 at 1:04 PM, Jeevan Chalke <jeevan.chalke@enterprisedb.com> wrote: > Hi, > > At some places, I have observed that we are adding a partial path even when > rel's consider_parallel is false. Due to this, the partial path added has > parallel_safe set to false and then later in create_gather_path() assertion > fails. > Few Comments: 1. @@ -2196,6 +2196,10 @@ set_subquery_pathlist(PlannerInfo *root, RelOptInfo *rel, pathkeys, required_outer)); } + /* If parallelism is not possible, return. */ + if (!rel->consider_parallel || !bms_is_empty(required_outer)) + return; In this case shouldn't we set the rel's consider_parallel flag correctly rather than avoiding adding the path to it as we do in recurse_set_operations? 2. @@ -331,7 +331,7 @@ recurse_set_operations(Node *setOp, PlannerInfo *root, * to build a partial path for this relation. But there's no point in * considering any path but the cheapest. */ - if (final_rel->partial_pathlist != NIL) + if (final_rel->consider_parallel && final_rel->partial_pathlist != NIL) What problem did you see here or is it just for additional safety? Ideally, if the consider_parallel is false for a rel, it's partial path list should also be NULL. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -
Re: [sqlsmith] Failed assertion in create_gather_path
Amit Kapila <amit.kapila16@gmail.com> — 2018-04-09T12:23:49Z
On Mon, Apr 9, 2018 at 5:52 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: > On Sun, Apr 8, 2018 at 1:04 PM, Jeevan Chalke > <jeevan.chalke@enterprisedb.com> wrote: >> Hi, >> >> At some places, I have observed that we are adding a partial path even when >> rel's consider_parallel is false. Due to this, the partial path added has >> parallel_safe set to false and then later in create_gather_path() assertion >> fails. >> > > Few Comments: > 1. > @@ -2196,6 +2196,10 @@ set_subquery_pathlist(PlannerInfo *root, RelOptInfo *rel, > pathkeys, required_outer)); > } > > + /* If parallelism is not possible, return. */ > + if (!rel->consider_parallel || !bms_is_empty(required_outer)) > + return; > > In this case shouldn't we set the rel's consider_parallel flag > correctly rather than avoiding adding the path to it > In the above sentence, I mean to say *partial* path. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
-
Re: [sqlsmith] Failed assertion in create_gather_path
Jeevan Chalke <jeevan.chalke@enterprisedb.com> — 2018-04-10T06:59:01Z
On Mon, Apr 9, 2018 at 5:52 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: > On Sun, Apr 8, 2018 at 1:04 PM, Jeevan Chalke > <jeevan.chalke@enterprisedb.com> wrote: > > Hi, > > > > At some places, I have observed that we are adding a partial path even > when > > rel's consider_parallel is false. Due to this, the partial path added has > > parallel_safe set to false and then later in create_gather_path() > assertion > > fails. > > > > Few Comments: > 1. > @@ -2196,6 +2196,10 @@ set_subquery_pathlist(PlannerInfo *root, > RelOptInfo *rel, > pathkeys, required_outer)); > } > > + /* If parallelism is not possible, return. */ > + if (!rel->consider_parallel || !bms_is_empty(required_outer)) > + return; > > In this case shouldn't we set the rel's consider_parallel flag > correctly rather than avoiding adding the path to it as we do in > recurse_set_operations? > In recurse_set_operations() we are building a new rel and setting its properties from the final_rel. consider_parallel there is just copied from the final_rel. However, in set_subquery_pathlist(), rel is the input parameter here and we are trying to add a partial path to it without looking at its consider_parallel field. This patch does that. And if we want to set consider_parallel for the rel, then it should have been done prior to this function itself. And I am not sure where that exactly but not in this function I guess. > 2. > @@ -331,7 +331,7 @@ recurse_set_operations(Node *setOp, PlannerInfo *root, > * to build a partial path for this relation. But there's no point in > * considering any path but the cheapest. > */ > - if (final_rel->partial_pathlist != NIL) > + if (final_rel->consider_parallel && final_rel->partial_pathlist != NIL) > > What problem did you see here or is it just for additional safety? > Ideally, if the consider_parallel is false for a rel, it's partial > path list should also be NULL. > I actually wanted to have rel->consider_parallel in the condition (yes, for additional safety) as we are adding a partial path into rel. But then observed that it is same as that of final_rel->consider_parallel and thus used it along with other condition. I have observed at many places that we do check consider_parallel flag before adding a partial path to it. Thus for consistency added here too, but yes, it just adds an additional safety here. > -- > With Regards, > Amit Kapila. > EnterpriseDB: http://www.enterprisedb.com > -- Jeevan Chalke Technical Architect, Product Development EnterpriseDB Corporation The Enterprise PostgreSQL Company
-
Re: [sqlsmith] Failed assertion in create_gather_path
Robert Haas <robertmhaas@gmail.com> — 2018-04-10T17:44:24Z
On Tue, Apr 10, 2018 at 2:59 AM, Jeevan Chalke <jeevan.chalke@enterprisedb.com> wrote: > I actually wanted to have rel->consider_parallel in the condition (yes, for > additional safety) as we are adding a partial path into rel. But then > observed that it is same as that of final_rel->consider_parallel and thus > used it along with other condition. > > I have observed at many places that we do check consider_parallel flag > before adding a partial path to it. Thus for consistency added here too, but > yes, it just adds an additional safety here. Thanks to Andreas for reporting this issue and to Jeevan for fixing it. My commit 0927d2f46ddd4cf7d6bf2cc84b3be923e0aedc52 seems clearly to blame. The change to set_subquery_pathlist() looks correct, but can we add a simple test case? Right now if I keep the new Assert() in add_partial_path() and leave out the rest of the changes, the regression tests still pass. That's not so good. Also, I think I would be inclined to wrap the if-statement around the rest of the function instead of returning early. The new Assert() in add_partial_path() is an excellent idea. I had the same thought before, but I didn't do anything about it. That was a bad idea; your plan is better. In fact, I suggest an additional Assert() that any relation to which we're adding a partial path is marked consider_parallel, like this: + /* Path to be added must be parallel safe. */ + Assert(new_path->parallel_safe); + + /* Relation should be OK for parallelism, too. */ + Assert(parent_rel->consider_parallel); Regarding recurse_set_operations, since the rel->consider_parallel is always the same as final_rel->consider_parallel there's really no value in testing it. If it were possible for rel->consider_parallel to be false even when final_rel->consider_parallel were true then the test would be necessary for correctness. That might or might not happen in the future, so I guess it wouldn't hurt to include this for future-proofing, but it's not actually a bug fix, so it also wouldn't hurt to leave it out. I could go either way, I guess. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: [sqlsmith] Failed assertion in create_gather_path
Jeevan Chalke <jeevan.chalke@enterprisedb.com> — 2018-04-11T10:00:33Z
On Tue, Apr 10, 2018 at 11:14 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Apr 10, 2018 at 2:59 AM, Jeevan Chalke > <jeevan.chalke@enterprisedb.com> wrote: > > I actually wanted to have rel->consider_parallel in the condition (yes, > for > > additional safety) as we are adding a partial path into rel. But then > > observed that it is same as that of final_rel->consider_parallel and thus > > used it along with other condition. > > > > I have observed at many places that we do check consider_parallel flag > > before adding a partial path to it. Thus for consistency added here too, > but > > yes, it just adds an additional safety here. > > Thanks to Andreas for reporting this issue and to Jeevan for fixing > it. My commit 0927d2f46ddd4cf7d6bf2cc84b3be923e0aedc52 seems clearly > to blame. > > The change to set_subquery_pathlist() looks correct, but can we add a > simple test case? I have tried adding simple testcase in this version of the patch. This test hits the Assertion added in add_partial_path() like you have tried. > Right now if I keep the new Assert() in > add_partial_path() and leave out the rest of the changes, the > regression tests still pass. That's not so good. Also, I think I > would be inclined to wrap the if-statement around the rest of the > function instead of returning early. > OK. Wrapped if block instead of returning mid-way. > > The new Assert() in add_partial_path() is an excellent idea. I had > the same thought before, but I didn't do anything about it. That was > a bad idea; your plan is better. In fact, I suggest an additional > Assert() that any relation to which we're adding a partial path is > marked consider_parallel, like this: > > + /* Path to be added must be parallel safe. */ > + Assert(new_path->parallel_safe); > + > + /* Relation should be OK for parallelism, too. */ > + Assert(parent_rel->consider_parallel); > Yep. Added this new one too. > > Regarding recurse_set_operations, since the rel->consider_parallel is > always the same as final_rel->consider_parallel there's really no > value in testing it. If it were possible for rel->consider_parallel > to be false even when final_rel->consider_parallel were true then the > test would be necessary for correctness. That might or might not > happen in the future, so I guess it wouldn't hurt to include this for > future-proofing, In that case, we should have rel in a condition rather than final_rel as we are adding a path into rel. For future-proofing added check on lateral_relids too. > but it's not actually a bug fix, so it also wouldn't > hurt to leave it out. I could go either way, I guess. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > -- Jeevan Chalke Technical Architect, Product Development EnterpriseDB Corporation The Enterprise PostgreSQL Company
-
Re: [sqlsmith] Failed assertion in create_gather_path
Robert Haas <robertmhaas@gmail.com> — 2018-04-25T19:33:08Z
On Wed, Apr 11, 2018 at 6:00 AM, Jeevan Chalke <jeevan.chalke@enterprisedb.com> wrote: > [ new patch ] Committed. Apologies for the delay. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company