[sqlsmith] Failed assertion in create_gather_path
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);
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
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
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.
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
Attachments:
0001-Add-partial-path-only-when-rel-s-consider_parallel-i.patchtext/x-patch; charset=US-ASCII; name=0001-Add-partial-path-only-when-rel-s-consider_parallel-i.patchDownload+8-2
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
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
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
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
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
Attachments:
0001-Add-partial-path-only-when-rel-s-consider_parallel-i.patchtext/x-patch; charset=US-ASCII; name=0001-Add-partial-path-only-when-rel-s-consider_parallel-i.patchDownload+56-20
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