Thread: Apparent bug in _make_subplan

Apparent bug in _make_subplan

From
Tom Lane
Date:
I have been looking at the planner's handling of subplans, and I see
something that I think is wrong, but I'm not quite certain.  In
_make_subplan() in backend/optimizer/plan/subselect.c, there is the
code
   /* make parParam list */   foreach(lst, plan->extParam)   {       Var      *var = nth(lfirsti(lst),
PlannerParamVar);
       if (var->varlevelsup == PlannerQueryLevel)           node->parParam = lappendi(node->parParam, lfirsti(lst));
}

It looks to me like this code is supposed to find parameters that
reference the immediate parent plan level, as opposed to higher levels.
So, shouldn't it be looking for varlevelsup == 1, not PlannerQueryLevel?

For a first-level subplan, PlannerQueryLevel will be 1 at the time
this code runs, so the result is the same anyway.  But I think it
does the wrong thing for more deeply nested subplans.  Am I right?
        regards, tom lane


Re: Apparent bug in _make_subplan

From
Vadim Mikheev
Date:
Tom Lane wrote:
> 
>     /* make parParam list */
>     foreach(lst, plan->extParam)
>     {
>         Var      *var = nth(lfirsti(lst), PlannerParamVar);
> 
>         if (var->varlevelsup == PlannerQueryLevel)
>             node->parParam = lappendi(node->parParam, lfirsti(lst));
>     }
> 
> It looks to me like this code is supposed to find parameters that
> reference the immediate parent plan level, as opposed to higher levels.
> So, shouldn't it be looking for varlevelsup == 1, not PlannerQueryLevel?
> 
> For a first-level subplan, PlannerQueryLevel will be 1 at the time
> this code runs, so the result is the same anyway.  But I think it

PlannerQueryLevel will be 0 here - subselect.c:140
   /* and now we are parent again */   PlannerInitPlan = saved_ip;   PlannerQueryLevel--;

> does the wrong thing for more deeply nested subplans.  Am I right?

I'm not sure. Seems that I made assumption here that 
varlevelsup is _absolute_ level number and seems that
_replace_var() and _new_param() replace parser' varlevelsup
with absolute level value.

Vadim


Re: [HACKERS] Re: Apparent bug in _make_subplan

From
Tom Lane
Date:
Vadim Mikheev <vadim@krs.ru> writes:
>> For a first-level subplan, PlannerQueryLevel will be 1 at the time
>> this code runs, so the result is the same anyway.  But I think it

> PlannerQueryLevel will be 0 here - subselect.c:140

No, it's never 0.  It starts out 1 in planner(), and _make_subplan
increments it at line 116 before recursing, then decrements again at
line 142.  So it's at least one when we arrive at the parParam code.

> I'm not sure. Seems that I made assumption here that 
> varlevelsup is _absolute_ level number and seems that
> _replace_var() and _new_param() replace parser' varlevelsup
> with absolute level value.

After looking through all the references to varlevelsup, it's clear
that all pieces of the system *except* subselect.c treat varlevelsup
as a relative level number, so-many-levels-out-from-current-subplan.
subselect.c has a couple of places that think nonzero varlevelsup
is an absolute level number, with 1 as the top plan.  This is certainly
a source of bugs --- it happens to work for two-level plans, but will
fail for anything more deeply nested.  I will work on fixing subselect.c
to bring it in line with the rest of the world...
        regards, tom lane


Re: [HACKERS] Re: Apparent bug in _make_subplan

From
Bruce Momjian
Date:
> After looking through all the references to varlevelsup, it's clear
> that all pieces of the system *except* subselect.c treat varlevelsup
> as a relative level number, so-many-levels-out-from-current-subplan.
> subselect.c has a couple of places that think nonzero varlevelsup
> is an absolute level number, with 1 as the top plan.  This is certainly
> a source of bugs --- it happens to work for two-level plans, but will
> fail for anything more deeply nested.  I will work on fixing subselect.c
> to bring it in line with the rest of the world...

varlevelsup was always intended to be relative.

--  Bruce Momjian                        |  http://www.op.net/~candle maillist@candle.pha.pa.us            |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026
 


Re: [HACKERS] Re: Apparent bug in _make_subplan

From
Vadim Mikheev
Date:
Tom Lane wrote:
> 
> > I'm not sure. Seems that I made assumption here that
> > varlevelsup is _absolute_ level number and seems that
> > _replace_var() and _new_param() replace parser' varlevelsup
> > with absolute level value.
> 
> After looking through all the references to varlevelsup, it's clear
> that all pieces of the system *except* subselect.c treat varlevelsup
> as a relative level number, so-many-levels-out-from-current-subplan.
> subselect.c has a couple of places that think nonzero varlevelsup
> is an absolute level number, with 1 as the top plan.  This is certainly
> a source of bugs --- it happens to work for two-level plans, but will
> fail for anything more deeply nested.  I will work on fixing subselect.c
> to bring it in line with the rest of the world...

subselect.c uses varlevelsup as absolute level number only
for correlation vars <--> params mapping, so why should it be
source of bugs? SS_replace_correlation_vars replaces all
correlation vars with parameters. Vars with absolute varlevelsup
are in PlannerParamVar only. To identify correlation vars and
to know is parameter already assigned to a var we obviously
need in absolute level number.

Vadim


Re: [HACKERS] Re: Apparent bug in _make_subplan

From
Bruce Momjian
Date:
> subselect.c uses varlevelsup as absolute level number only
> for correlation vars <--> params mapping, so why should it be
> source of bugs? SS_replace_correlation_vars replaces all
> correlation vars with parameters. Vars with absolute varlevelsup
> are in PlannerParamVar only. To identify correlation vars and
> to know is parameter already assigned to a var we obviously
> need in absolute level number.

But the varlevelsup I pass in from the parser are relative to the
current level, not absolute. 

--  Bruce Momjian                        |  http://www.op.net/~candle maillist@candle.pha.pa.us            |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026
 


Re: [HACKERS] Re: Apparent bug in _make_subplan

From
Vadim Mikheev
Date:
Bruce Momjian wrote:
> 
> > subselect.c uses varlevelsup as absolute level number only
> > for correlation vars <--> params mapping, so why should it be
> > source of bugs? SS_replace_correlation_vars replaces all
> > correlation vars with parameters. Vars with absolute varlevelsup
> > are in PlannerParamVar only. To identify correlation vars and
> > to know is parameter already assigned to a var we obviously
> > need in absolute level number.
> 
> But the varlevelsup I pass in from the parser are relative to the
> current level, not absolute.

subselect.c takes it into account, computes absolute numbers
and stores them in PlannerParamVar only...

Vadim


Re: [HACKERS] Re: Apparent bug in _make_subplan

From
Tom Lane
Date:
Vadim Mikheev <vadim@krs.ru> writes:
>> But the varlevelsup I pass in from the parser are relative to the
>> current level, not absolute.

> subselect.c takes it into account, computes absolute numbers
> and stores them in PlannerParamVar only...

Right, I eventually figured that out, and I see that it's probably the
best way.  I have added the following documentation to subselect.c:

/*--------------------* PlannerParamVar is a list of Var nodes, wherein the n'th entry* (n counts from 0) corresponds
toParam->paramid = n.  The Var nodes* are ordinary except for one thing: their varlevelsup field does NOT* have the
usualinterpretation of "subplan levels out from current".* Instead, it contains the absolute plan level, with the
outermost*plan being level 1 and nested plans having higher level numbers.* This nonstandardness is useful because we
don'thave to run around* and update the list elements when we enter or exit a subplan* recursion level.  But we must
payattention not to confuse this* meaning with the normal meaning of varlevelsup.*--------------------*/
 

along with other changes that I will commit once I get subselects in
HAVING working right ...
        regards, tom lane


Re: [HACKERS] Re: Apparent bug in _make_subplan

From
Bruce Momjian
Date:
> Bruce Momjian wrote:
> > 
> > > subselect.c uses varlevelsup as absolute level number only
> > > for correlation vars <--> params mapping, so why should it be
> > > source of bugs? SS_replace_correlation_vars replaces all
> > > correlation vars with parameters. Vars with absolute varlevelsup
> > > are in PlannerParamVar only. To identify correlation vars and
> > > to know is parameter already assigned to a var we obviously
> > > need in absolute level number.
> > 
> > But the varlevelsup I pass in from the parser are relative to the
> > current level, not absolute.
> 
> subselect.c takes it into account, computes absolute numbers
> and stores them in PlannerParamVar only...

Oh.

--  Bruce Momjian                        |  http://www.op.net/~candle maillist@candle.pha.pa.us            |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026