Thread: Apparent bug in _make_subplan
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
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
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
> 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
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
> 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
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
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
> 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