Обсуждение: Core dump in range_table_mutator()

Commit 64919aaab made pull_up_simple_subquery set rte->subquery = NULL
after doing the deed, so that we don't waste cycles copying a
now-useless subquery tree around.  I discovered today while
working on another patch that if you invoke query_tree_mutator
or range_table_mutator on the whole Query after that point,
range_table_mutator dumps core, because it's expecting subquery
links to never be NULL.  There's apparently noplace in our core
code that does that today, but I'm a bit surprised we've not heard
complaints from anyone else.  I propose to do this to harden it:

diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 876f84dd39..8d58265010 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -3788,7 +3788,9 @@ range_table_mutator(List *rtable,
                 /* we don't bother to copy eref, aliases, etc; OK? */
                 break;
             case RTE_SUBQUERY:
-                if (!(flags & QTW_IGNORE_RT_SUBQUERIES))
+                /* In the planner, subquery is null if it's been flattened */
+                if (!(flags & QTW_IGNORE_RT_SUBQUERIES) &&
+                    rte->subquery != NULL)
                 {
                     CHECKFLATCOPY(newrte->subquery, rte->subquery, Query);
                     MUTATE(newrte->subquery, newrte->subquery, Query *);


            regards, tom lane

On Fri, 24 Jun 2022 at 22:44, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Commit 64919aaab made pull_up_simple_subquery set rte->subquery = NULL
> after doing the deed, so that we don't waste cycles copying a
> now-useless subquery tree around.  I discovered today while
> working on another patch that if you invoke query_tree_mutator
> or range_table_mutator on the whole Query after that point,
> range_table_mutator dumps core, because it's expecting subquery
> links to never be NULL.  There's apparently noplace in our core
> code that does that today, but I'm a bit surprised we've not heard
> complaints from anyone else.  I propose to do this to harden it:
>

Makes sense.

Not directly related to that change ... I think it would be easier to
follow if the CHECKFLATCOPY() was replaced with a separate Assert()
and FLATCOPY() (I had to go and remind myself what CHECKFLATCOPY()
did).

Doing that would allow CHECKFLATCOPY() to be deleted, since this is
the only place that uses it -- every other case knows the node type is
correct before doing a FLATCOPY().

Well almost. The preceding FLATCOPY() of the containing RangeTblEntry
doesn't check the node type, but that could be fixed by using
lfirst_node() instead of lfirst() at the start of the loop, which
would be neater.

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> Not directly related to that change ... I think it would be easier to
> follow if the CHECKFLATCOPY() was replaced with a separate Assert()
> and FLATCOPY() (I had to go and remind myself what CHECKFLATCOPY()
> did).
> Doing that would allow CHECKFLATCOPY() to be deleted, since this is
> the only place that uses it -- every other case knows the node type is
> correct before doing a FLATCOPY().

Well, if we want to clean this up a bit rather than just doing the
minimum safe fix ... I spent some time why we were bothering with the
FLATCOPY step at all, rather than just mutating the Query* pointer.
I think the reason is to not fail if the QTW_DONT_COPY_QUERY flag is
set, but maybe we should clear that flag when recursing?

            regards, tom lane

On Sat, 25 Jun 2022 at 04:39, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Well, if we want to clean this up a bit rather than just doing the
> minimum safe fix ... I spent some time why we were bothering with the
> FLATCOPY step at all, rather than just mutating the Query* pointer.
> I think the reason is to not fail if the QTW_DONT_COPY_QUERY flag is
> set, but maybe we should clear that flag when recursing?
>

Hmm, interesting, but we don't actually pass on that flag when
recursing anyway. Since it is the mutator routine's responsibility to
make a possibly-modified copy of its input node, if it wants to
recurse into the subquery, it should always call query_tree_mutator()
with QTW_DONT_COPY_QUERY unset, and range_table_mutator() should never
need to FLATCOPY() the subquery.

But then, in the interests of further tidying up, why does
range_table_mutator() call copyObject() on the subquery if
QTW_IGNORE_RT_SUBQUERIES is set? If QTW_IGNORE_RT_SUBQUERIES isn't
set, the mutator routine will either copy and modify the subquery, or
it will return the original unmodified subquery node via
expression_tree_mutator(), without copying it. So then if
QTW_IGNORE_RT_SUBQUERIES is set, why not also just return the original
unmodified subquery node?

So then the RTE_SUBQUERY case in range_table_mutator() would only have to do:

    case RTE_SUBQUERY:
        if (!(flags & QTW_IGNORE_RT_SUBQUERIES))
            MUTATE(newrte->subquery, newrte->subquery, Query *);
        break;

which wouldn't fall over if the subquery were NULL.

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> On Sat, 25 Jun 2022 at 04:39, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Well, if we want to clean this up a bit rather than just doing the
>> minimum safe fix ... I spent some time why we were bothering with the
>> FLATCOPY step at all, rather than just mutating the Query* pointer.
>> I think the reason is to not fail if the QTW_DONT_COPY_QUERY flag is
>> set, but maybe we should clear that flag when recursing?

> Hmm, interesting, but we don't actually pass on that flag when
> recursing anyway. Since it is the mutator routine's responsibility to
> make a possibly-modified copy of its input node, if it wants to
> recurse into the subquery, it should always call query_tree_mutator()
> with QTW_DONT_COPY_QUERY unset, and range_table_mutator() should never
> need to FLATCOPY() the subquery.

Actually, QTW_DONT_COPY_QUERY is dead code AFAICS: we don't use it
anywhere, and Debian Code Search doesn't know of any outside users
either.  Removing it might be something to do in v16.  (I think
it's a bit late for unnecessary API changes in v15.)

> But then, in the interests of further tidying up, why does
> range_table_mutator() call copyObject() on the subquery if
> QTW_IGNORE_RT_SUBQUERIES is set?

I thought about that for a bit, but all of the QTW_IGNORE flags
work like that, and I'm hesitant to change it.  There may be
code that assumes it can modify those trees in-place afterwards.

Committed with just the change to use straight MUTATE, making
this case exactly like the other places with QTW_IGNORE options.
Thanks for the discussion!

            regards, tom lane