Thread: [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.

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 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 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