| From: | Vaibhav Dalvi <vaibhav(dot)dalvi(at)enterprisedb(dot)com> |
|---|---|
| To: | Jeevan Chalke <jeevan(dot)chalke(at)enterprisedb(dot)com> |
| Cc: | PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
| Subject: | Re: Add PRODUCT() aggregate function |
| Date: | 2026-09-10 11:08:54 |
| Message-ID: | CA+vB=AFvV+5FuAR3rTnEZmBVwUG872m9rUC+6U7wxZW=5ZE=sw@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi Jeevan,
Nice feature; I tested it locally and it works correctly. NULL
handling, parallel aggregate (combine), and the moving-window.
Fallback to recalculation are all fine, no correctness bug was found.
I only have the following point with a short description.
*There is no fast path for the common case; it always goes through Numeric:*
For int2/int4/int8/float4/float8, every row undergoes a full
arbitrary-precision
Numeric conversion plus numeric_mul, even when the running product
would easily fit in int64/int128 for most rows. This file already has a
pattern
for exactly this problem (int8 SUM uses int128 internally, only promoting to
numeric on real overflow). I think PRODUCT(int4)/PRODUCT(int2) over a
large table will be much slower per row than SUM for the same data, because
of this.
So, if possible, consider using the same native-then-promote-on-overflow
approach here.
Thanks,
Vaibhav Dalvi
EnterpriseDB
On Fri, Jun 26, 2026 at 11:24 AM Jeevan Chalke <
jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
> Hello,
>
> CFbot flagged this for a rebase. The conflicts were due to the catalog
> version bump, so I've dropped it here and noted in the commit message
> that the committer should bump catversion at commit time to avoid
> recurring conflicts.
>
> Also added tests as suggested by Jim.
>
> Thanks
>
> On Tue, Jun 23, 2026 at 5:26 PM Jeevan Chalke <
> jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
>
>>
>>
>> On Tue, Jun 23, 2026 at 4:32 PM Jim Jones <jim(dot)jones(at)uni-muenster(dot)de>
>> wrote:
>>
>>> Hi Jeevan
>>>
>>> On 23/06/2026 10:37, Dean Rasheed wrote:
>>> > On Tue, 23 Jun 2026 at 08:49, Jeevan Chalke
>>> > <jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
>>> >> PRODUCT() returns the product of all non-null input values. It is
>>> defined for
>>> >> int2, int4, int8, float4, float8 and numeric input, and always
>>> returns numeric.
>>> > I don't think that you need to define it for all those types. I
>>> > suspect that you could just define it for numeric and float8, and let
>>> > implicit casting do the rest.
>>>
>>> +1
>>>
>>> I've tested the patch in many different scenarios and all results look
>>> fine -- valgrind also didn't report anything :)
>>>
>>> The test coverage is comprehensive! For the sake of completeness I'd add
>>> numeric tests for NaN and Infitinty with positive numeric values in the
>>> set, e.g:
>>>
>>> postgres=# WITH j (v) AS (VALUES
>>> ('NaN'::numeric),('Infinity'::numeric),(3.14))
>>> SELECT product(v) FROM j;
>>> product
>>> ---------
>>> NaN
>>> (1 row)
>>>
>>> Other than that and the point mentioned by Dean I have nothing to add at
>>> this point.
>>>
>>
>> Thanks, Jim, for the thorough testing.
>>
>> I'll include that test case in the next version of the patch.
>>
>>
>>
>>>
>>> Thanks for the patch.
>>>
>>> Best, Jim
>>>
>>
>>
>> --
>> *Jeevan Chalke*
>> *Senior Principal Engineer, Engineering Manager*
>> *Product Development*
>>
>> enterprisedb.com <https://www.enterprisedb.com>
>>
>
>
> --
> *Jeevan Chalke*
> *Senior Principal Engineer, Engineering Manager*
> *Product Development*
>
> enterprisedb.com <https://www.enterprisedb.com>
>
| From | Date | Subject | |
|---|---|---|---|
| Next Message | vignesh C | 2026-09-10 11:12:47 | Re: Review items for EXCEPT TABLE publication |
| Previous Message | Hayato Kuroda (Fujitsu) | 2026-09-10 11:06:27 | RE: Review items for EXCEPT TABLE publication |