On Sat, Oct 28, 2023 at 1:10 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> On Sat, Oct 28, 2023 at 9:59 AM Andrei Lepikhov
> <a.lepikhov@postgrespro.ru> wrote:
> >
> > On 27/10/2023 21:10, Richard Guo wrote:
> > >
> > > On Fri, Oct 27, 2023 at 7:00 PM Andrei Lepikhov
> > > <a.lepikhov@postgrespro.ru <mailto:a.lepikhov@postgrespro.ru>> wrote:
> > >
> > > So, I can propose two options. First - don't clean only the current
> > > root
> > > structure, but also make cleanup of the parent. Although it looks safe,
> > > I am not happy with this approach - it seems too simple: we should have
> > > a genuine reason for such a cleaning because it potentially adds
> > > overhead.
> > > The second option is to add a flag for not altering queries in
> > > remove_nulling_relids() - it looks like a mistake when we have two
> > > different query trees in the root and its parent. Also, it reduces
> > > memory usage a bit.
> > > So, if my analysis is correct, it is better to use the second way (see
> > > attachment).
> > >
> > >
> > > Alternatively, can we look at subroot->parse->targetList instead of
> > > subquery->targetList where we call estimate_num_groups on the output of
> > > the subquery?
> >
> > It is a solution. But does it mask the real problem? In my mind, we copy
> > node trees to use somewhere else or probe a conjecture. Here, we have
> > two different representations of the same subquery. Keeping aside the
> > memory consumption issue, is it correct?
> > Make sense to apply both options: switch the groups estimation to
> > subroot targetList and keep one version of a subquery.
> > In attachment - second (combined) version of the change. Here I added
> > assertions to check identity of root->parse and incoming query tree.
>
> Andrei, did you read the comment just before the groups estimation as
> pointed by Tom [1]?
>
> * XXX you don't really want to know about this: we do the estimation
> * using the subquery's original targetlist expressions, not the
> * subroot->processed_tlist which might seem more appropriate. The
> * reason is that if the subquery is itself a setop, it may return a
> * processed_tlist containing "varno 0" Vars generated by
> * generate_append_tlist, and those would confuse estimate_num_groups
> * mightily. We ought to get rid of the "varno 0" hack, but that
> * requires a redesign of the parsetree representation of setops, so
> * that there can be an RTE corresponding to each setop's output.
>
> As I understand, it requires much more work to correctly switch the
> groups estimation to subroot targetList.
>
> +1 for asserts that parse trees are the same.
I made some beautification of the patch by Andrei. I also removed the
part which changes the target list for estimate_num_groups(). Any
objections to pushing this?
------
Regards,
Alexander Korotkov
