    While  looking  at all these parsetrees I wonder why the hell
    the GroupClause contains a complete copy of the TLE  at  all?
    The  planner  depends on finding a corresponding entry in the
    targetlist which should contain the same expression. At least
    it needs an equal junk TLE. For the query

        SELECT a, b FROM t1 GROUP BY b + 1;

    the  parser  in  fact creates 3 TLE's where the last one is a
    junk result named "resjunk" for the "b +  1"  expression  and
    the GroupClause contains a totally equal TLE.

    Could someone explain that please?

    Wouldn't it be better to have another field (resgroupno e.g.)
    in the resdom  which  the  GroupClause  can  reference?  Then
    changing  the resno's or even replacing the entire expression
    wouldn't hurt because  make_subplanTargetList()  could  match
    them  this  way  and  the expressions for the subplans can be
    pulled out directly from the targetlist. And  it  would  save
    processing  the  group  clauses in the rewriting because they
    cannot contain Var nodes anymore and the entire list  can  be
    ignored.


Jan

--

#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me.                                  #
#======================================== jwieck@debis.com (Jan Wieck) #



Tue, 11 May 1999 19:07:31 +0200 (MET DST)
jwieck@debis.com (Jan Wieck)
More on GROUP BY
>     While  looking  at all these parsetrees I wonder why the hell
>     the GroupClause contains a complete copy of the TLE  at  all?
>     The  planner  depends on finding a corresponding entry in the
>     targetlist which should contain the same expression. At least
>     it needs an equal junk TLE. For the query
> 
>         SELECT a, b FROM t1 GROUP BY b + 1;
> 
>     the  parser  in  fact creates 3 TLE's where the last one is a
>     junk result named "resjunk" for the "b +  1"  expression  and
>     the GroupClause contains a totally equal TLE.
> 
>     Could someone explain that please?
> 
>     Wouldn't it be better to have another field (resgroupno e.g.)
>     in the resdom  which  the  GroupClause  can  reference?  Then
>     changing  the resno's or even replacing the entire expression
>     wouldn't hurt because  make_subplanTargetList()  could  match
>     them  this  way  and  the expressions for the subplans can be
>     pulled out directly from the targetlist. And  it  would  save
>     processing  the  group  clauses in the rewriting because they
>     cannot contain Var nodes anymore and the entire list  can  be
>     ignored.

I think I can comment on this.  Aggregates had the similar problem.  It
was so long ago, I don't remember the solution, but it was a pain to
keep the aggs up-to-date with the target list and varno changes.  If you
think a redesign will fix the problem, go ahead.

I think the old problem may have been that the old Aggreg's kept
pointers to matching target list entries, so there was the aggregate in
the target list, and another separate list of aggregates in the Query
structure.  I think I removed the second copy, and just generated it in
the executor, where it was needed.

Please see parser/parse_agg.c for a description of how count(*) is
handled differently than other aggregates.


-- 
  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

Tue, 11 May 1999 14:01:31 -0400 (EDT)
Bruce Momjian <maillist@candle.pha.pa.us>
Re: [HACKERS] More on GROUP BY
jwieck@debis.com (Jan Wieck) writes:
>     While  looking  at all these parsetrees I wonder why the hell
>     the GroupClause contains a complete copy of the TLE  at  all?
>     The  planner  depends on finding a corresponding entry in the
>     targetlist which should contain the same expression. At least
>     it needs an equal junk TLE. For the query
>         SELECT a, b FROM t1 GROUP BY b + 1;
>     the  parser  in  fact creates 3 TLE's where the last one is a
>     junk result named "resjunk" for the "b +  1"  expression  and
>     the GroupClause contains a totally equal TLE.

>     Could someone explain that please?

All true, but so what?  It wastes a few bytes of memory during
planning, I suppose...

>     Wouldn't it be better to have another field (resgroupno e.g.)
>     in the resdom  which  the  GroupClause  can  reference?  Then
>     changing  the resno's or even replacing the entire expression
>     wouldn't hurt because  make_subplanTargetList()  could  match
>     them  this  way  and  the expressions for the subplans can be
>     pulled out directly from the targetlist. And  it  would  save
>     processing  the  group  clauses in the rewriting because they
>     cannot contain Var nodes anymore and the entire list  can  be
>     ignored.

I think I like better the idea of leaving the representation alone,
but using equal() on the exprs to match groupclause items to targetlist
entries.  That way, manipulation of the targetlist can't accidentally
cause the grouplist to look like it contains something different than
what it should have.  It doesn't bother me that the planner can fail
if it is unable to match a group item to a targetlist item --- that's
a good crosscheck that nothing's gone wrong.  (But matching on just
the resno is unsafe, as you said before.)

I think it's true that using a TLE for each grouplist item is a waste of
space, and that representing the grouplist as simply a list of expr's
would be good enough.  But pulling out the TLE decoration seems like
it's not an appropriate use of time at this stage of the release cycle.
I'd say hold off till after 6.5, then fold it in with the parsetree
redesign that you keep muttering we need (I agree!).

BTW, you keep using the term "RTE" which I'm not familiar with ---
I assume it's just referring to the parse tree nodes?

			regards, tom lane

Tue, 11 May 1999 14:30:20 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: [HACKERS] More on GROUP BY 
> >     Wouldn't it be better to have another field (resgroupno e.g.)
> >     in the resdom  which  the  GroupClause  can  reference?  Then
> >     changing  the resno's or even replacing the entire expression
> >     wouldn't hurt because  make_subplanTargetList()  could  match
> >     them  this  way  and  the expressions for the subplans can be
> >     pulled out directly from the targetlist. And  it  would  save
> >     processing  the  group  clauses in the rewriting because they
> >     cannot contain Var nodes anymore and the entire list  can  be
> >     ignored.
> 
> I think I like better the idea of leaving the representation alone,
> but using equal() on the exprs to match groupclause items to targetlist
> entries.  That way, manipulation of the targetlist can't accidentally
> cause the grouplist to look like it contains something different than
> what it should have.  It doesn't bother me that the planner can fail
> if it is unable to match a group item to a targetlist item --- that's
> a good crosscheck that nothing's gone wrong.  (But matching on just
> the resno is unsafe, as you said before.)

The real problem is that it is hard to keep all these items synchronized
as they pass around through the stages.  I looked at the aggregates, and
it looks like that has a separate copy too, so it may not be that bad. 
We may just be missing a pass that makes appropriate changes in GROUP
clauses, but I am not sure.


> I think it's true that using a TLE for each grouplist item is a waste of
> space, and that representing the grouplist as simply a list of expr's
> would be good enough.  But pulling out the TLE decoration seems like
> it's not an appropriate use of time at this stage of the release cycle.
> I'd say hold off till after 6.5, then fold it in with the parsetree
> redesign that you keep muttering we need (I agree!).

Basically, it is my fault that I am bringing up the issue so late.  If I
had done the Open Items list earlier, we would not be so close to the
final.

Jan is currently researching it, and we have the regression tests and
three weeks.  He usually does a fine job of fixing things.  Jan, find
out what you think is required to get this working, and if it is not too
bad, maybe we should go ahead.

What does the Aggreg do?  Does it has similar duplication?

-- 
  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

Tue, 11 May 1999 15:01:10 -0400 (EDT)
Bruce Momjian <maillist@candle.pha.pa.us>
Re: [HACKERS] More on GROUP BY
Tom Lane wrote:

> All true, but so what?  It wastes a few bytes of memory during
> planning, I suppose...

    Not  only  memory,  rewriting takes time too. If the GROUP BY
    clause has view columns, the rewriter has to change  all  the
    expressions  twice,  one time in the targetlist, another time
    in the group clause. Wasted efford if they  should  still  be
    equal after rewriting.

> I think it's true that using a TLE for each grouplist item is a waste of
> space, and that representing the grouplist as simply a list of expr's
> would be good enough.  But pulling out the TLE decoration seems like
> it's not an appropriate use of time at this stage of the release cycle.
> I'd say hold off till after 6.5, then fold it in with the parsetree
> redesign that you keep muttering we need (I agree!).

    I'm  sure  since  I  began to look at it that it's not such a
    good idea to make those changes at this stage.  But  in  fact
    some  of  them have to be done to make GROUP BY in views more
    robust.

>
> BTW, you keep using the term "RTE" which I'm not familiar with ---
> I assume it's just referring to the parse tree nodes?

    RTE == RangeTableEntry. The relations where the data  in  the
    querytree is coming from. The varno in Var nodes is the index
    into that table so the varno plus the varattno  identify  one
    relations attribute.


Jan

--

#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me.                                  #
#======================================== jwieck@debis.com (Jan Wieck) #



Wed, 12 May 1999 12:22:03 +0200 (MET DST)
jwieck@debis.com (Jan Wieck)
Re: [HACKERS] More on GROUP BY
Bruce Momjian wrote:

> The real problem is that it is hard to keep all these items synchronized
> as they pass around through the stages.  I looked at the aggregates, and
> it looks like that has a separate copy too, so it may not be that bad.
> We may just be missing a pass that makes appropriate changes in GROUP
> clauses, but I am not sure.

    Exactly  the probability of missing some changes somewhere is
    why  I  would  like  to  get  rid  of  the  field  entry   in
    GroupClause.  Having to keep duplicate items synced isn't the
    spirit of a relational database. It's like using triggers  to
    keep  two  tables in sync where a simple reference and a view
    would do a better job.

> > I think it's true that using a TLE for each grouplist item is a waste of
> > space, and that representing the grouplist as simply a list of expr's
> > would be good enough.  But pulling out the TLE decoration seems like
> > it's not an appropriate use of time at this stage of the release cycle.
> > I'd say hold off till after 6.5, then fold it in with the parsetree
> > redesign that you keep muttering we need (I agree!).
>
> Basically, it is my fault that I am bringing up the issue so late.  If I
> had done the Open Items list earlier, we would not be so close to the
> final.

    And my fault to spend too much time  playing  around  with  a
    raytracer.   Developers  should  develop,  publishers  should
    publish.

>
> Jan is currently researching it, and we have the regression tests and
> three weeks.  He usually does a fine job of fixing things.

    Thanks for the compliment :-)

>                                                            Jan, find
> out what you think is required to get this working, and if it is not too
> bad, maybe we should go ahead.
>
> What does the Aggreg do?  Does it has similar duplication?

    Not AFAICS. Aggregates can only appear in the  targetlist  by
    default.   and  the  nodes  below  them are the expression to
    aggregate over.  If an aggregate should appear in  the  WHERE
    clause  it  must  be placed into a proper subselect (the rule
    system already tries to do so if an aggregate column is  used
    in the WHERE of a view select, but fails sometimes).

    I'll go ahead now in little steps.

    1.  Get rid of the TLE copy in GroupClause.

    2.  Move the targetlist expansion into the rule system.

    3.  Rewrite the subquery creation for aggregates in the WHERE
        clause to take view grouping into account.

    4.  Allow qualifications against aggregates to  be  given  as
        "AggCol  op  Value"  by swapping the expression and using
        the negator operator (if one exists).

    As you said, we have three weeks. Let's see  what  one  Wieck
    can do in this time :-)


Jan

--

#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me.                                  #
#======================================== jwieck@debis.com (Jan Wieck) #



Wed, 12 May 1999 13:13:08 +0200 (MET DST)
jwieck@debis.com (Jan Wieck)
Re: [HACKERS] More on GROUP BY
I wrote:

>     I'll go ahead now in little steps.
>
>     1.  Get rid of the TLE copy in GroupClause.

    Done. GroupClause now identifies the TLE by a number which is
    placed into the Resdom by parser/rewriter.

    New  initdb  required  because  of  modifications   in   node
    print/read functions.

>
>     2.  Move the targetlist expansion into the rule system.

    Not   required   anymore   AFAICS.  Instead  I  modified  the
    targetlist  preprocessing  in  the  planner  so   that   junk
    attributes  used  by  group  clauses  get  added again to the
    expanded list.


Jan

--

#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me.                                  #
#======================================== jwieck@debis.com (Jan Wieck) #



Wed, 12 May 1999 17:16:11 +0200 (MET DST)
jwieck@debis.com (Jan Wieck)
Re: [HACKERS] More on GROUP BY