Apparent bug in _make_subplan

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, Pennsylvania 19026

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, Pennsylvania 19026

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 to Param->paramid = n. The Var nodes
* are ordinary except for one thing: their varlevelsup field does NOT
* have the usual interpretation 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't have to run around
* and update the list elements when we enter or exit a subplan
* recursion level. But we must pay attention 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, Pennsylvania 19026