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

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

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