Horton

Thread

Commits

  1. Fix handling of NULLs returned by aggregate combine functions.

  2. Support multi-stage aggregation.

  1. Combine function returning NULL unhandled?

    Andres Freund <andres@anarazel.de> — 2017-11-21T03:36:42Z

    Hi,

Since
commit a7de3dc5c346e07e0439275982569996e645b3c2
Author: Robert Haas <rhaas@postgresql.org>
Date:   2016-01-20 13:46:50 -0500

    Support multi-stage aggregation.
    
    Aggregate nodes now have two new modes: a "partial" mode where they
    output the unfinalized transition state, and a "finalize" mode where
    they accept unfinalized transition states rather than individual
    values as input.
    
    These new modes are not used anywhere yet, but they will be necessary
    for parallel aggregation.  The infrastructure also figures to be
    useful for cases where we want to aggregate local data and remote
    data via the FDW interface, and want to bring back partial aggregates
    from the remote side that can then be combined with locally generated
    partial aggregates to produce the final value.  It may also be useful
    even when neither FDWs nor parallelism are in play, as explained in
    the comments in nodeAgg.c.
    
    David Rowley and Simon Riggs, reviewed by KaiGai Kohei, Heikki
    Linnakangas, Haribabu Kommi, and me.

there's both advance_transition_function and advance_combine_function,
fulfilling closely related duties.

While working on that code (making transition and combine functions go
through expression evaluation, so they can be JITed), I noticed a small
difference in behaviour beteween the two:

The plain transition case contains:
		if (pergroupstate->transValueIsNull)
		{
			/*
			 * Don't call a strict function with NULL inputs.  Note it is
			 * possible to get here despite the above tests, if the transfn is
			 * strict *and* returned a NULL on a prior cycle. If that happens
			 * we will propagate the NULL all the way to the end.
			 */
			return;
		}

how come similar logic is not present for combine functions? I don't see
any checks preventing a combinefunc from returning NULL, nor do I see
https://www.postgresql.org/docs/devel/static/sql-createaggregate.html
spell out a requirement that that not be the case.

Greetings,

Andres Freund

  2. Re: Combine function returning NULL unhandled?

    Robert Haas <robertmhaas@gmail.com> — 2017-11-21T20:51:59Z

    On Mon, Nov 20, 2017 at 10:36 PM, Andres Freund <andres@anarazel.de> wrote:
> The plain transition case contains:
>                 if (pergroupstate->transValueIsNull)
>                 {
>                         /*
>                          * Don't call a strict function with NULL inputs.  Note it is
>                          * possible to get here despite the above tests, if the transfn is
>                          * strict *and* returned a NULL on a prior cycle. If that happens
>                          * we will propagate the NULL all the way to the end.
>                          */
>                         return;
>                 }
>
> how come similar logic is not present for combine functions? I don't see
> any checks preventing a combinefunc from returning NULL, nor do I see
> https://www.postgresql.org/docs/devel/static/sql-createaggregate.html
> spell out a requirement that that not be the case.

I don't know of a reason why that logic shouldn't be present for the
combine-function case as well.  It seems like it should be pretty
straightforward to write a test that hits that case and watch it blow
up ... assuming it does, then I guess we should back-patch the
addition of that logic.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  3. Re: Combine function returning NULL unhandled?

    Andres Freund <andres@anarazel.de> — 2017-11-24T01:20:32Z

    On 2017-11-21 15:51:59 -0500, Robert Haas wrote:
> On Mon, Nov 20, 2017 at 10:36 PM, Andres Freund <andres@anarazel.de> wrote:
> > The plain transition case contains:
> >                 if (pergroupstate->transValueIsNull)
> >                 {
> >                         /*
> >                          * Don't call a strict function with NULL inputs.  Note it is
> >                          * possible to get here despite the above tests, if the transfn is
> >                          * strict *and* returned a NULL on a prior cycle. If that happens
> >                          * we will propagate the NULL all the way to the end.
> >                          */
> >                         return;
> >                 }
> >
> > how come similar logic is not present for combine functions? I don't see
> > any checks preventing a combinefunc from returning NULL, nor do I see
> > https://www.postgresql.org/docs/devel/static/sql-createaggregate.html
> > spell out a requirement that that not be the case.
> 
> I don't know of a reason why that logic shouldn't be present for the
> combine-function case as well.  It seems like it should be pretty
> straightforward to write a test that hits that case and watch it blow
> up ... assuming it does, then I guess we should back-patch the
> addition of that logic.

Found it surprisingly not that straightforward ;)

Pushed a fix to the relevant branches, including tests of the
trans/combine functions returns NULL cases.

Greetings,

Andres Freund

  4. Re: Combine function returning NULL unhandled?

    David Rowley <david.rowley@2ndquadrant.com> — 2017-12-01T00:33:04Z

    On 24 November 2017 at 14:20, Andres Freund <andres@anarazel.de> wrote:
> Pushed a fix to the relevant branches, including tests of the
> trans/combine functions returns NULL cases.

Apologies for my silence here. I've been on leave and out of internet
range for two weeks.

Thank you for making the fix.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services