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

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

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