Test HAVING condition before computing targetlist of an Aggregate node.

This is required by SQL spec to avoid failures in cases like
  SELECT sum(win)/sum(lose) FROM ... GROUP BY ... HAVING sum(lose) > 0;
AFAICT we have gotten this wrong since day one.  Kudos to Holger Jakobs
for being the first to notice.

diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 119f5da346c9f3f7408e9890a5313bed3c60d078..ceddae7a4eb9d8025db7ca73a2712a1affdc252d 100644 (file)
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -45,7 +45,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/executor/nodeAgg.c,v 1.122 2004/06/06 00:41:26 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/executor/nodeAgg.c,v 1.123 2004/07/10 18:39:23 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -674,7 +674,6 @@ agg_retrieve_direct(AggState *aggstate)
        AggStatePerGroup pergroup;
        TupleTableSlot *outerslot;
        TupleTableSlot *firstSlot;
-       TupleTableSlot *resultSlot;
        int                     aggno;
 
        /*
@@ -696,11 +695,8 @@ agg_retrieve_direct(AggState *aggstate)
         * We loop retrieving groups until we find one matching
         * aggstate->ss.ps.qual
         */
-       do
+       while (!aggstate->agg_done)
        {
-               if (aggstate->agg_done)
-                       return NULL;
-
                /*
                 * If we don't already have the first tuple of the new group,
                 * fetch it from the outer plan.
@@ -815,7 +811,7 @@ agg_retrieve_direct(AggState *aggstate)
                /*
                 * If we have no first tuple (ie, the outerPlan didn't return
                 * anything), create a dummy all-nulls input tuple for use by
-                * ExecProject. 99.44% of the time this is a waste of cycles,
+                * ExecQual/ExecProject. 99.44% of the time this is a waste of cycles,
                 * because ordinarily the projected output tuple's targetlist
                 * cannot contain any direct (non-aggregated) references to input
                 * columns, so the dummy tuple will not be referenced. However
@@ -857,22 +853,28 @@ agg_retrieve_direct(AggState *aggstate)
                }
 
                /*
-                * Form a projection tuple using the aggregate results and the
-                * representative input tuple.  Store it in the result tuple slot.
-                * Note we do not support aggregates returning sets ...
+                * Use the representative input tuple for any references to
+                * non-aggregated input columns in the qual and tlist.
                 */
                econtext->ecxt_scantuple = firstSlot;
-               resultSlot = ExecProject(projInfo, NULL);
 
                /*
-                * If the completed tuple does not match the qualifications, it is
-                * ignored and we loop back to try to process another group.
-                * Otherwise, return the tuple.
+                * Check the qual (HAVING clause); if the group does not match,
+                * ignore it and loop back to try to process another group.
                 */
+               if (ExecQual(aggstate->ss.ps.qual, econtext, false))
+               {
+                       /*
+                        * Form and return a projection tuple using the aggregate results
+                        * and the representative input tuple.  Note we do not support
+                        * aggregates returning sets ...
+                        */
+                       return ExecProject(projInfo, NULL);
+               }
        }
-       while (!ExecQual(aggstate->ss.ps.qual, econtext, false));
 
-       return resultSlot;
+       /* No more groups */
+       return NULL;
 }
 
 /*
@@ -934,7 +936,6 @@ agg_retrieve_hash_table(AggState *aggstate)
        AggStatePerGroup pergroup;
        AggHashEntry entry;
        TupleTableSlot *firstSlot;
-       TupleTableSlot *resultSlot;
        int                     aggno;
 
        /*
@@ -952,11 +953,8 @@ agg_retrieve_hash_table(AggState *aggstate)
         * We loop retrieving groups until we find one satisfying
         * aggstate->ss.ps.qual
         */
-       do
+       while (!aggstate->agg_done)
        {
-               if (aggstate->agg_done)
-                       return NULL;
-
                /*
                 * Find the next entry in the hash table
                 */
@@ -999,22 +997,28 @@ agg_retrieve_hash_table(AggState *aggstate)
                }
 
                /*
-                * Form a projection tuple using the aggregate results and the
-                * representative input tuple.  Store it in the result tuple slot.
-                * Note we do not support aggregates returning sets ...
+                * Use the representative input tuple for any references to
+                * non-aggregated input columns in the qual and tlist.
                 */
                econtext->ecxt_scantuple = firstSlot;
-               resultSlot = ExecProject(projInfo, NULL);
 
                /*
-                * If the completed tuple does not match the qualifications, it is
-                * ignored and we loop back to try to process another group.
-                * Otherwise, return the tuple.
+                * Check the qual (HAVING clause); if the group does not match,
+                * ignore it and loop back to try to process another group.
                 */
+               if (ExecQual(aggstate->ss.ps.qual, econtext, false))
+               {
+                       /*
+                        * Form and return a projection tuple using the aggregate results
+                        * and the representative input tuple.  Note we do not support
+                        * aggregates returning sets ...
+                        */
+                       return ExecProject(projInfo, NULL);
+               }
        }
-       while (!ExecQual(aggstate->ss.ps.qual, econtext, false));
 
-       return resultSlot;
+       /* No more groups */
+       return NULL;
 }
 
 /* -----------------