Thread: BUG #1188: evaluation order of select seems to be wrong

The following bug has been logged online:

Bug reference:      1188
Logged by:          Holger Jakobs

Email address:      holger@jakobs.com

PostgreSQL version: 7.4

Operating system:   Linux

Description:        evaluation order of select seems to be wrong

Details:

There is a table like

  create table games (
    teamnr serial,
    player integer not null,
    win    integer not null,
    lose   integer not null
  );

I have the following select statement:

  select teamnr, sum(win)/sum(lose)
  from games
  group by teamnr
  having sum(lose) > 0;

According to what I know about select, the expressions of the from clause
have to be evaluated last, so that the case that sum(lose) is zero will be
filtered _before_ the division by 0 takes place.

Unfortunately, PostgreSQL 7.4.2 gives a "division by zero" error if there
are teams which have not yet won any game. Those teams should just not
appear in the output.

Example insert statements:

insert into games values (1, 11, 3, 0);
insert into games values (1, 12, 5, 2);
insert into games values (2, 21, 6, 0);

Without the last insert everything works fine. Adding the last insert
produces a team with zero numbers of games won leading to the error.

Btw, Oracle 8 handels this correctly.

Am Mittwoch, 7. Juli 2004 14:58 schrieb PostgreSQL Bugs List:
> Description:        evaluation order of select seems to be wrong

Please read this:
http://www.postgresql.org/docs/7.4/static/sql-expressions.html#SYNTAX-EXPRESS-EVAL

On Wed, 7 Jul 2004, Peter Eisentraut wrote:

> Am Mittwoch, 7. Juli 2004 14:58 schrieb PostgreSQL Bugs List:
> > Description:        evaluation order of select seems to be wrong
>
> Please read this:
> http://www.postgresql.org/docs/7.4/static/sql-expressions.html#SYNTAX-EXPRESS-EVAL

The issue is that from what he said the standard may mandate that HAVING
is applied before the select list is evaluated because it's part of the
table expression.  In that case, the statement he gave would be required
to work because the select list would be evaluated on the output of the
table expression that's already had the having clause filter out the bad
rows.  This is different from the case where both are in where conditions.

Stephan Szabo <sszabo@megazone.bigpanda.com> writes:
> On Wed, 7 Jul 2004, Peter Eisentraut wrote:
>> Am Mittwoch, 7. Juli 2004 14:58 schrieb PostgreSQL Bugs List:
> Description:        evaluation order of select seems to be wrong
>>
>> Please read this:
>> http://www.postgresql.org/docs/7.4/static/sql-expressions.html#SYNTAX-EXPRESS-EVAL

> The issue is that from what he said the standard may mandate that HAVING
> is applied before the select list is evaluated because it's part of the
> table expression.

I think this is indubitably a bug.  We get it right in the non-aggregate
case, e.g.
    select teamnr,win/lose from games where lose>0;
does not result in any divide-by-0 error.

I think (but haven't tested) that the fix just involves altering the
order of operations in nodeAgg.c so that we don't ExecProject until we
have checked the qual condition.

What surprises me is that no one has complained of this before.  The
error exists at least back to 7.0, possibly forever...

            regards, tom lane

I wrote:
> I think (but haven't tested) that the fix just involves altering the
> order of operations in nodeAgg.c so that we don't ExecProject until we
> have checked the qual condition.

I have applied the attached patch to fix this in CVS HEAD and 7.4
branches.  The error does in fact date back to the very first support
for HAVING, many years ago now.  A quick search does not find any other
executor node types making the same mistake.

            regards, tom lane

Index: nodeAgg.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/backend/executor/nodeAgg.c,v
retrieving revision 1.116.2.1
diff -c -r1.116.2.1 nodeAgg.c
*** nodeAgg.c    13 Mar 2004 00:54:35 -0000    1.116.2.1
--- nodeAgg.c    10 Jul 2004 18:25:17 -0000
***************
*** 674,680 ****
      AggStatePerGroup pergroup;
      TupleTableSlot *outerslot;
      TupleTableSlot *firstSlot;
-     TupleTableSlot *resultSlot;
      int            aggno;

      /*
--- 674,679 ----
***************
*** 696,706 ****
       * We loop retrieving groups until we find one matching
       * aggstate->ss.ps.qual
       */
!     do
      {
-         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.
--- 695,702 ----
       * We loop retrieving groups until we find one matching
       * aggstate->ss.ps.qual
       */
!     while (!aggstate->agg_done)
      {
          /*
           * If we don't already have the first tuple of the new group,
           * fetch it from the outer plan.
***************
*** 815,821 ****
          /*
           * 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,
           * 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
--- 811,817 ----
          /*
           * If we have no first tuple (ie, the outerPlan didn't return
           * anything), create a dummy all-nulls input tuple for use by
!          * 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,878 ****
          }

          /*
!          * 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 ...
           */
          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.
           */
      }
-     while (!ExecQual(aggstate->ss.ps.qual, econtext, false));

!     return resultSlot;
  }

  /*
--- 853,880 ----
          }

          /*
!          * Use the representative input tuple for any references to
!          * non-aggregated input columns in the qual and tlist.
           */
          econtext->ecxt_scantuple = firstSlot;

          /*
!          * 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);
+         }
      }

!     /* No more groups */
!     return NULL;
  }

  /*
***************
*** 934,940 ****
      AggStatePerGroup pergroup;
      AggHashEntry entry;
      TupleTableSlot *firstSlot;
-     TupleTableSlot *resultSlot;
      int            aggno;

      /*
--- 936,941 ----
***************
*** 952,962 ****
       * We loop retrieving groups until we find one satisfying
       * aggstate->ss.ps.qual
       */
!     do
      {
-         if (aggstate->agg_done)
-             return NULL;
-
          /*
           * Find the next entry in the hash table
           */
--- 953,960 ----
       * We loop retrieving groups until we find one satisfying
       * aggstate->ss.ps.qual
       */
!     while (!aggstate->agg_done)
      {
          /*
           * Find the next entry in the hash table
           */
***************
*** 999,1020 ****
          }

          /*
!          * 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 ...
           */
          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.
           */
      }
-     while (!ExecQual(aggstate->ss.ps.qual, econtext, false));

!     return resultSlot;
  }

  /* -----------------
--- 997,1024 ----
          }

          /*
!          * Use the representative input tuple for any references to
!          * non-aggregated input columns in the qual and tlist.
           */
          econtext->ecxt_scantuple = firstSlot;

          /*
!          * 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);
+         }
      }

!     /* No more groups */
!     return NULL;
  }

  /* -----------------