Horton

Thread

Commits

  1. Repair breakage of aggregate FILTER option.

  2. Perform one only projection to compute agg arguments.

  1. Aggregate FILTER option is broken in v10

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-10-16T13:42:34Z

    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

  2. Re: Aggregate FILTER option is broken in v10

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-10-16T15:12:09Z

    I wrote:
> 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.

Hmm ... on closer inspection, the only performance-critical place
where this matters is advance_aggregates, and that already has a check
for whether the particular aggregate has a filter.  So we could do
something like

        /* Skip anything FILTERed out */
        if (filter)
        {
            // existing code to eval/check filter expr
+
+           /* Now it's safe to evaluate this agg's arguments */
+           slot = ExecProject(pertrans->argproj);
        }
+       else
+           slot = aggstate->evalslot;

which seems like a pretty minimal extra cost for the normal case
with no filter.

Let me see what I can make of that ...

			regards, tom lane

  3. Re: Aggregate FILTER option is broken in v10

    Andres Freund <andres@anarazel.de> — 2017-10-16T20:19:56Z

    On 2017-10-16 11:12:09 -0400, Tom Lane wrote:
> I wrote:
> > 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.
> 
> Hmm ... on closer inspection, the only performance-critical place
> where this matters is advance_aggregates, and that already has a check
> for whether the particular aggregate has a filter.  So we could do
> something like
> 
>         /* Skip anything FILTERed out */
>         if (filter)
>         {
>             // existing code to eval/check filter expr
> +
> +           /* Now it's safe to evaluate this agg's arguments */
> +           slot = ExecProject(pertrans->argproj);
>         }
> +       else
> +           slot = aggstate->evalslot;
> 
> which seems like a pretty minimal extra cost for the normal case
> with no filter.

Thanks, that looks like a reasonable fix.

Greetings,

Andres Freund