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

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