Thread: [HACKERS] [sqlsmith] Failed assertion in adjust_appendrel_attrs_mutator

Hi,

testing master as of 7c981590c2, sqlsmith just triggered the following
assertion:

TRAP: FailedAssertion("!(!((((const Node*)(node))->type) == T_SubLink))", File: "prepunion.c", Line: 2231)

I can reproduce it on a vanilla regression database with the following
query:

--8<---------------cut here---------------start------------->8---
explain delete from public.road
where
EXISTS ( select     ref_1.tablename   from     public.renamecolumnchild as ref_0right join pg_catalog.pg_policies as
ref_1on(true)   where EXISTS (      select      public.road.name as c1,      ref_1.with_check as c3,      ref_2.b as c6
  from      public.itest4 as ref_2));
 
--8<---------------cut here---------------end--------------->8---

Backtrace below.

regards,
Andreas

Core was generated by `postgres: smith regression [local] DELETE                        '.
Program terminated with signal SIGABRT, Aborted.
#0  0x00007f70c2dacfcf in raise () from /lib/x86_64-linux-gnu/libc.so.6
#1  0x00007f70c2dae3fa in abort () from /lib/x86_64-linux-gnu/libc.so.6
#2  0x0000564d8ce587b3 in ExceptionalCondition (conditionName=conditionName@entry=0x564d8cfe87b0 "!(!((((const
Node*)(node))->type)== T_SubLink))", errorType=errorType@entry=0x564d8cea483d "FailedAssertion",
fileName=fileName@entry=0x564d8cfea866"prepunion.c", lineNumber=lineNumber@entry=2231) at assert.c:54
 
#3  0x0000564d8ccaf7dc in adjust_appendrel_attrs_mutator (node=0x564d91e0d2a0, context=0x7fff0a33caa0) at
prepunion.c:2231
#4  0x0000564d8cc4d8ef in expression_tree_mutator (node=0x564d91e0ccd8, mutator=0x564d8ccaf1d0
<adjust_appendrel_attrs_mutator>,context=0x7fff0a33caa0) at nodeFuncs.c:2554
 
#5  0x0000564d8cc4d88f in expression_tree_mutator (node=0x564d91e0df90, mutator=mutator@entry=0x564d8ccaf1d0
<adjust_appendrel_attrs_mutator>,context=context@entry=0x7fff0a33caa0) at nodeFuncs.c:3008
 
#6  0x0000564d8ccaf29f in adjust_appendrel_attrs_mutator (node=0x564d91e0df90, context=0x7fff0a33caa0) at
prepunion.c:2151
#7  0x0000564d8cc4dafb in expression_tree_mutator (node=<optimized out>, mutator=0x564d8ccaf1d0
<adjust_appendrel_attrs_mutator>,context=0x7fff0a33caa0) at nodeFuncs.c:2903
 
#8  0x0000564d8cc4e589 in range_table_mutator (rtable=<optimized out>, mutator=mutator@entry=0x564d8ccaf1d0
<adjust_appendrel_attrs_mutator>,context=context@entry=0x7fff0a33caa0, flags=flags@entry=3) at nodeFuncs.c:3146
 
#9  0x0000564d8cc4e78e in query_tree_mutator (query=0x564d91e30c20, query@entry=0x564d90fbe090,
mutator=mutator@entry=0x564d8ccaf1d0<adjust_appendrel_attrs_mutator>, context=context@entry=0x7fff0a33caa0,
flags=flags@entry=3)at nodeFuncs.c:3096
 
#10 0x0000564d8ccb1499 in adjust_appendrel_attrs (root=root@entry=0x564d910bab58, node=node@entry=0x564d90fbe090,
nappinfos=nappinfos@entry=1,appinfos=appinfos@entry=0x7fff0a33cba8) at prepunion.c:1964
 
#11 0x0000564d8cca3406 in inheritance_planner (root=<optimized out>) at planner.c:1200
#12 subquery_planner (glob=glob@entry=0x564d910baac0, parse=parse@entry=0x564d90fbe090,
parent_root=parent_root@entry=0x0,hasRecursion=hasRecursion@entry=0 '\000', tuple_fraction=tuple_fraction@entry=0) at
planner.c:857
#13 0x0000564d8cca3dbf in standard_planner (parse=0x564d90fbe090, cursorOptions=256, boundParams=0x0) at planner.c:352
#14 0x0000564d8cd4ef5d in pg_plan_query (querytree=0x564d90fbe090, cursorOptions=256, boundParams=0x0) at
postgres.c:807
#15 0x0000564d8cd4f03e in pg_plan_queries (querytrees=<optimized out>, cursorOptions=cursorOptions@entry=256,
boundParams=boundParams@entry=0x0)at postgres.c:873
 
#16 0x0000564d8cd4f519 in exec_simple_query (query_string=0x564d8ed5c330 "[...]") at postgres.c:1048
#17 0x0000564d8cd51151 in PostgresMain (argc=<optimized out>, argv=argv@entry=0x564d8ed66c50, dbname=<optimized out>,
username=<optimizedout>) at postgres.c:4139
 
#18 0x0000564d8ca56032 in BackendRun (port=0x564d8ed6d460) at postmaster.c:4364
#19 BackendStartup (port=0x564d8ed6d460) at postmaster.c:4036
#20 ServerLoop () at postmaster.c:1755
#21 0x0000564d8ccd628c in PostmasterMain (argc=3, argv=0x564d8ed3c5a0) at postmaster.c:1363
#22 0x0000564d8ca5761d in main (argc=3, argv=0x564d8ed3c5a0) at main.c:228


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andreas Seltenreich <seltenreich@gmx.de> writes:
> testing master as of 7c981590c2, sqlsmith just triggered the following
> assertion:
> TRAP: FailedAssertion("!(!((((const Node*)(node))->type) == T_SubLink))", File: "prepunion.c", Line: 2231)

Hmm.  adjust_appendrel_attrs() thinks it's only used after conversion
of sublinks to subplans, but this is a counterexample.  I wonder if
that assumption was ever correct?  Or maybe we need to rethink what
it does when recursing into RTE subqueries.
        regards, tom lane


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2017/10/23 2:07, Tom Lane wrote:
> Andreas Seltenreich <seltenreich@gmx.de> writes:
>> testing master as of 7c981590c2, sqlsmith just triggered the following
>> assertion:
>> TRAP: FailedAssertion("!(!((((const Node*)(node))->type) == T_SubLink))", File: "prepunion.c", Line: 2231)
> 
> Hmm.  adjust_appendrel_attrs() thinks it's only used after conversion
> of sublinks to subplans, but this is a counterexample.  I wonder if
> that assumption was ever correct?  Or maybe we need to rethink what
> it does when recursing into RTE subqueries.

Looking at the backtrace, the crashing SubLink seems to have been
referenced from a PlaceHolderVar that is in turn referenced by
joinaliasvars of a JOIN rte in query->rtable.

I wonder if we shouldn't just ignore those (joinaliasvars in JOIN rtes)
while adjust_appendrel_attrs() is doing its job on a Query, just like we
ask to ignore subqueries by passing QTW_IGNORE_RC_SUBQUERIES to
query_tree_mutator()?  IOW, it doesn't look like anything critically
depends on the Vars referenced from joinaliasvars being translated at that
point in inheritance_planner(), but I may be missing something.

The attached patch to do the same, while didn't break any existing tests,
fixed the crash reported by OP.

Thoughts?

Thanks,
Amit

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> On 2017/10/23 2:07, Tom Lane wrote:
>> Hmm.  adjust_appendrel_attrs() thinks it's only used after conversion
>> of sublinks to subplans, but this is a counterexample.  I wonder if
>> that assumption was ever correct?  Or maybe we need to rethink what
>> it does when recursing into RTE subqueries.

> Looking at the backtrace, the crashing SubLink seems to have been
> referenced from a PlaceHolderVar that is in turn referenced by
> joinaliasvars of a JOIN rte in query->rtable.

Right.  The core of the issue is that joinaliasvars lists don't get run
through preprocess_expression, so (among other things) any SubLinks in
them don't get expanded to subplans.  Probably the reason we've not
heard field complaints about this is that in a non-assert build there
would be no observable bad effects --- the scan would simply ignore
the subquery, and whether the joinaliasvars entry has been correctly
mutated doesn't matter at all because it will never be used again.

> I wonder if we shouldn't just ignore those (joinaliasvars in JOIN rtes)
> while adjust_appendrel_attrs() is doing its job on a Query, just like we
> ask to ignore subqueries by passing QTW_IGNORE_RC_SUBQUERIES to
> query_tree_mutator()?

I don't really like this fix, because ISTM it's fixing one symptom rather
than the root of the problem.  The root is that joinaliasvars lists
diverge from the representation of expressions elsewhere in the tree,
and not only with respect to SubLinks --- another example is that function
calls with named arguments won't have been rearranged into executable
form.  That could easily be a dangerous thing, if we allow arbitrary
expression processing to get done on them.  Moreover, your patch is
letting the divergence get even bigger: now the joinaliasvars lists don't
even have the right varnos, making them certainly unusable for anything.

So what I'm thinking is that we would be well advised to actually remove
the untransformed joinaliasvars from the tree as soon as we're done with
preprocessing expressions.  We'd drop them at the end of planning anyway
(cf. add_rte_to_flat_rtable) so this is just getting rid of them a bit
sooner, and it won't affect anything after the planner.

In short, I'm thinking something more like the attached.

Comments?

            regards, tom lane

diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index ecdd728..c0ce3c3 100644
*** a/src/backend/optimizer/plan/planner.c
--- b/src/backend/optimizer/plan/planner.c
*************** subquery_planner(PlannerGlobal *glob, Qu
*** 777,782 ****
--- 777,803 ----
      }

      /*
+      * Now that we are done preprocessing expressions, and in particular done
+      * flattening JOIN alias variables, get rid of the joinaliasvars lists.
+      * They no longer match what expressions in the rest of the tree look
+      * like, because we have not preprocessed expressions in those lists (and
+      * do not want to; for example, expanding a SubLink there would result in
+      * a useless unreferenced subplan).  Leaving them in place simply creates
+      * a hazard for later scans of the tree.  We could try to prevent that by
+      * using QTW_IGNORE_JOINALIASES in every tree scan done after this point,
+      * but that doesn't sound very reliable.
+      */
+     if (root->hasJoinRTEs)
+     {
+         foreach(l, parse->rtable)
+         {
+             RangeTblEntry *rte = lfirst_node(RangeTblEntry, l);
+
+             rte->joinaliasvars = NIL;
+         }
+     }
+
+     /*
       * In some cases we may want to transfer a HAVING clause into WHERE. We
       * cannot do so if the HAVING clause contains aggregates (obviously) or
       * volatile functions (since a HAVING clause is supposed to be executed

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2017/10/24 0:22, Tom Lane wrote:
> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
>> On 2017/10/23 2:07, Tom Lane wrote:
>>> Hmm.  adjust_appendrel_attrs() thinks it's only used after conversion
>>> of sublinks to subplans, but this is a counterexample.  I wonder if
>>> that assumption was ever correct?  Or maybe we need to rethink what
>>> it does when recursing into RTE subqueries.
> 
>> Looking at the backtrace, the crashing SubLink seems to have been
>> referenced from a PlaceHolderVar that is in turn referenced by
>> joinaliasvars of a JOIN rte in query->rtable.
> 
> Right.  The core of the issue is that joinaliasvars lists don't get run
> through preprocess_expression, so (among other things) any SubLinks in
> them don't get expanded to subplans.

Ah, I missed that.

> Probably the reason we've not
> heard field complaints about this is that in a non-assert build there
> would be no observable bad effects --- the scan would simply ignore
> the subquery, and whether the joinaliasvars entry has been correctly
> mutated doesn't matter at all because it will never be used again.

I see.

>> I wonder if we shouldn't just ignore those (joinaliasvars in JOIN rtes)
>> while adjust_appendrel_attrs() is doing its job on a Query, just like we
>> ask to ignore subqueries by passing QTW_IGNORE_RC_SUBQUERIES to
>> query_tree_mutator()?
> 
> I don't really like this fix, because ISTM it's fixing one symptom rather
> than the root of the problem.

That's true.

> The root is that joinaliasvars lists
> diverge from the representation of expressions elsewhere in the tree,
> and not only with respect to SubLinks --- another example is that function
> calls with named arguments won't have been rearranged into executable
> form.  That could easily be a dangerous thing, if we allow arbitrary
> expression processing to get done on them.  Moreover, your patch is
> letting the divergence get even bigger: now the joinaliasvars lists don't
> even have the right varnos, making them certainly unusable for anything.

Hmm, yes.  Although, I only proposed that patch because I had convinced
myself that joinaliasvars lists weren't looked at anymore.

> So what I'm thinking is that we would be well advised to actually remove
> the untransformed joinaliasvars from the tree as soon as we're done with
> preprocessing expressions.  We'd drop them at the end of planning anyway
> (cf. add_rte_to_flat_rtable) so this is just getting rid of them a bit
> sooner, and it won't affect anything after the planner.
>
> In short, I'm thinking something more like the attached.

Yeah, that makes more sense.

Thanks,
Amit



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers