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.
Links
1. https://www.postgresql.org/message-id/1551957.1698417686%40sss.pgh.pa.us
------
Regards,
Alexander Korotkov