Aggregate FILTER option is broken in v10

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Andres Freund <andres(at)anarazel(dot)de>
Cc: pgsql-hackers(at)postgreSQL(dot)org
Subject: Aggregate FILTER option is broken in v10
Date: 2017-10-16 13:42:34
Message-ID: 30065.1508161354@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Consider

regression=# select sum(1/ten) filter (where ten>0) from tenk1;
ERROR: division by zero

This query works without error in versions before 10. It was evidently
broken by commit 8ed3f11bb, which rearranged nodeAgg.c to evaluate all
aggregate input expressions before considering any FILTERs.

This is not an acceptable behavioral change. This sort of thing seems
like perhaps the primary use-case for FILTER. It's stated to work by
our own manual --- see the last sentence in
https://www.postgresql.org/docs/devel/static/sql-expressions.html#syntax-express-eval
And it's required by the SQL spec, which states clearly that the
aggregate's input expression is only evaluated at rows for which
the filter expression yields true. (In SQL:2011, see 10.9 <aggregate
function> general rules 4 and 5a.)

I think possibly the best answer is to revert 8ed3f11bb. We could
think about some compromise solution like merging the projections
only for aggregates without FILTER. But that would require two
code paths through the relevant functions in nodeAgg.c, which would
be a substantial maintenance burden; and the extra branches required
means that this would be a net negative for performance in the
simplest case with only one aggregate. In any case, since that
patch went in before the v10 expression evaluation rewrite, I think
any argument that it's worth keeping would need to be made afresh.
The overhead that it was hoping to save should be much lower now.

Thoughts?

regards, tom lane

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2017-10-16 15:12:09 Re: Aggregate FILTER option is broken in v10
Previous Message Amit Kapila 2017-10-16 12:50:33 Re: BLK_DONE state in XLogReadBufferForRedoExtended