Re: Properly mark NULL returns in numeric aggregates

2020-04-14 Thread David Rowley
On Wed, 15 Apr 2020 at 03:41, Tom Lane wrote: > > David Rowley writes: > > For testing, can't we just have an Assert() in > > advance_transition_function that verifies isnull matches the > > nullability of the return value for INTERNAL returning transfns? i.e, > > the attached > > FTR, I do not

Re: Properly mark NULL returns in numeric aggregates

2020-04-14 Thread Tom Lane
David Rowley writes: > For testing, can't we just have an Assert() in > advance_transition_function that verifies isnull matches the > nullability of the return value for INTERNAL returning transfns? i.e, > the attached FTR, I do not like this Assert one bit. nodeAgg.c has NO business inquiring

Re: Properly mark NULL returns in numeric aggregates

2020-04-14 Thread Jesse Zhang
Hi David, On Mon, Apr 13, 2020 at 10:46 PM David Rowley wrote: > > On Tue, 14 Apr 2020 at 06:14, Tom Lane wrote: > > Yeah, they're relying exactly on the assumption that nodeAgg is not > > going to try to copy a value declared "internal", and therefore they > > can be loosey-goosey about

Re: Properly mark NULL returns in numeric aggregates

2020-04-13 Thread David Rowley
On Tue, 14 Apr 2020 at 06:14, Tom Lane wrote: > Yeah, they're relying exactly on the assumption that nodeAgg is not > going to try to copy a value declared "internal", and therefore they > can be loosey-goosey about whether the value pointer is null or not. > However, if you want to claim that

Re: Properly mark NULL returns in numeric aggregates

2020-04-13 Thread Tom Lane
Jesse Zhang writes: > On Fri, Apr 10, 2020 at 3:59 PM Tom Lane wrote: >> They can't be strict because the initial iteration needs to produce >> something from a null state and non-null input. nodeAgg's default >> behavior won't work for those because nodeAgg doesn't know how to >> copy a value

Re: Properly mark NULL returns in numeric aggregates

2020-04-13 Thread Jesse Zhang
On Fri, Apr 10, 2020 at 3:59 PM Tom Lane wrote: > > They can't be strict because the initial iteration needs to produce > something from a null state and non-null input. nodeAgg's default > behavior won't work for those because nodeAgg doesn't know how to > copy a value of type "internal". > >

Re: Properly mark NULL returns in numeric aggregates

2020-04-10 Thread Tom Lane
Jesse Zhang writes: > On the other hand, we examined the corresponding final functions > (numeric_stddev_pop and friends), they all seem to carefully treat a > NULL trans value the same as a "zero input" (as in, state.N == 0 && > state.NaNcount ==0). That does suggest to me that it should be fine

Re: Properly mark NULL returns in numeric aggregates

2020-04-10 Thread Jesse Zhang
Hi Andres, On Fri, Apr 10, 2020 at 12:14 PM Andres Freund wrote: > > Shouldn't these just be marked as strict? > Are you suggesting that because none of the corresponding trans functions (int8_avg_accum, int2_accum, and friends) ever output NULL? That's what we thought, but then I realized that

Re: Properly mark NULL returns in numeric aggregates

2020-04-10 Thread Tom Lane
Andres Freund writes: > On 2020-04-09 16:22:11 -0700, Jesse Zhang wrote: >> We found that several functions -- namely numeric_combine, >> numeric_avg_combine, numeric_poly_combine, and int8_avg_combine -- are >> returning NULL without signaling the nullity of datum in fcinfo.isnull. >> This is

Re: Properly mark NULL returns in numeric aggregates

2020-04-10 Thread Andres Freund
Hi, On 2020-04-09 16:22:11 -0700, Jesse Zhang wrote: > We found that several functions -- namely numeric_combine, > numeric_avg_combine, numeric_poly_combine, and int8_avg_combine -- are > returning NULL without signaling the nullity of datum in fcinfo.isnull. > This is obscured by the fact that

Properly mark NULL returns in numeric aggregates

2020-04-09 Thread Jesse Zhang
Mon Sep 17 00:00:00 2001 From: Denis Smirnov Date: Thu, 9 Apr 2020 16:10:29 -0700 Subject: [PATCH] Properly mark NULL returns in numeric aggregates When a function returns NULL (in the SQL sense), it is expected to signal that in fcinfo.isnull . numeric_combine and friends break this rule