Re: Numeric multiplication overflow errors

2021-07-05 Thread Ranier Vilela
Em seg., 5 de jul. de 2021 às 09:02, David Rowley escreveu: > On Mon, 5 Jul 2021 at 23:07, Ranier Vilela wrote: > > > > Em seg., 5 de jul. de 2021 às 06:44, Dean Rasheed < > dean.a.rash...@gmail.com> escreveu: > >> Note, however, that it won't make any difference to performance in the > >> way t

Re: Numeric multiplication overflow errors

2021-07-05 Thread David Rowley
On Mon, 5 Jul 2021 at 23:07, Ranier Vilela wrote: > > Em seg., 5 de jul. de 2021 às 06:44, Dean Rasheed > escreveu: >> Note, however, that it won't make any difference to performance in the >> way that you're suggesting -- elog() in Postgres is used for "should >> never happen, unless there's a

Re: Numeric multiplication overflow errors

2021-07-05 Thread Ranier Vilela
Em seg., 5 de jul. de 2021 às 06:44, Dean Rasheed escreveu: > On Fri, 2 Jul 2021 at 19:48, Ranier Vilela wrote: > > > > If you allow me a small suggestion. > > Move the initializations of the variable tmp_var to after check if the > function can run. > > Saves some cycles, when not running. > >

Re: Numeric multiplication overflow errors

2021-07-05 Thread Dean Rasheed
On Sun, 4 Jul 2021 at 09:43, David Rowley wrote: > > On Sat, 3 Jul 2021 at 11:04, Dean Rasheed wrote: > > Thinking about this more, I think it's best not to risk back-patching. > > It *might* be safe, but it's difficult to really be sure of that. The > > bug itself is pretty unlikely to ever happ

Re: Numeric multiplication overflow errors

2021-07-05 Thread Dean Rasheed
On Fri, 2 Jul 2021 at 19:48, Ranier Vilela wrote: > > If you allow me a small suggestion. > Move the initializations of the variable tmp_var to after check if the > function can run. > Saves some cycles, when not running. > OK, thanks. I agree, on grounds of neatness and consistency with nearby

Re: Numeric multiplication overflow errors

2021-07-04 Thread David Rowley
On Sat, 3 Jul 2021 at 11:04, Dean Rasheed wrote: > Thinking about this more, I think it's best not to risk back-patching. > It *might* be safe, but it's difficult to really be sure of that. The > bug itself is pretty unlikely to ever happen in practice, hence the > lack of prior complaints, and in

Re: Numeric multiplication overflow errors

2021-07-02 Thread Dean Rasheed
On Fri, 2 Jul 2021 at 12:56, David Rowley wrote: > > On Fri, 2 Jul 2021 at 22:55, Dean Rasheed wrote: > > Here's an update with the > > last set of changes discussed. > > Looks good to me. Thanks for the review and testing! > Just the question of if we have any problems changing the serialized

Re: Numeric multiplication overflow errors

2021-07-02 Thread Ranier Vilela
On Fri, 2 Jul 2021 at 22:55, Dean Rasheed wrote: > Here's an update with the > last set of changes discussed. If you allow me a small suggestion. Move the initializations of the variable tmp_var to after check if the function can run. Saves some cycles, when not running. /* Ensure we disallow c

Re: Numeric multiplication overflow errors

2021-07-02 Thread David Rowley
On Fri, 2 Jul 2021 at 22:55, Dean Rasheed wrote: > Here's an update with the > last set of changes discussed. Looks good to me. Just the question of if we have any problems changing the serialized format in the back branches. I'm not sure if that's something we've done before. I only had a quic

Re: Numeric multiplication overflow errors

2021-07-02 Thread Dean Rasheed
On Fri, 2 Jul 2021 at 10:24, David Rowley wrote: > > I ran this again with a few different worker counts after tuning a few > memory settings so there was no spilling to disk and so everything was > in RAM. Mostly so I could get consistent results. > > Here's the results. Average over 3 runs on ea

Re: Numeric multiplication overflow errors

2021-07-02 Thread David Rowley
On Fri, 2 Jul 2021 at 00:28, Dean Rasheed wrote: > > On Thu, 1 Jul 2021 at 06:43, David Rowley wrote: > > > > Master @ 3788c6678 > > > > Execution Time: 8306.319 ms > > Execution Time: 8407.785 ms > > Execution Time: 8491.056 ms > > > > Master + numeric-agg-sumX2-overflow-v2.patch > > Execution T

Re: Numeric multiplication overflow errors

2021-07-01 Thread Dean Rasheed
On Thu, 1 Jul 2021 at 06:43, David Rowley wrote: > > Master @ 3788c6678 > > Execution Time: 8306.319 ms > Execution Time: 8407.785 ms > Execution Time: 8491.056 ms > > Master + numeric-agg-sumX2-overflow-v2.patch > Execution Time: 6633.278 ms > Execution Time: 6657.350 ms > Execution Time: 6568.18

Re: Numeric multiplication overflow errors

2021-07-01 Thread David Rowley
On Thu, 1 Jul 2021 at 22:03, Dean Rasheed wrote: > One other thing I'm wondering about is back-patching. I was originally > thinking of these as back-patchable bug fixes, but changing the binary > format of the aggregate serialization states feels dodgy for a > back-patch. I was wondering about t

Re: Numeric multiplication overflow errors

2021-07-01 Thread Dean Rasheed
On Thu, 1 Jul 2021 at 10:27, Dean Rasheed wrote: > > I'll post an update in a while. Thanks for the review. > One other thing I'm wondering about is back-patching. I was originally thinking of these as back-patchable bug fixes, but changing the binary format of the aggregate serialization states

Re: Numeric multiplication overflow errors

2021-07-01 Thread Dean Rasheed
On Thu, 1 Jul 2021 at 02:00, David Rowley wrote: > > I kinda disagree with the send/recv naming since all you're using them > for is to serialise/deserialise the NumericVar. Functions named > *send() and recv() I more expect to return a bytea so they can be used > for a type's send/recv function.

Re: Numeric multiplication overflow errors

2021-06-30 Thread David Rowley
patchOn Thu, 1 Jul 2021 at 13:00, David Rowley wrote: > I've only looked at the numeric-agg-sumX2-overflow-v2.patch one and it > all looks mostly ok. I forgot to mention earlier, after looking at the code a bit more I wondered if we should just serialise the NumericSumAccum instead of the Numeri

Re: Numeric multiplication overflow errors

2021-06-30 Thread David Rowley
Thanks for the updated patch On Tue, 29 Jun 2021 at 22:29, Dean Rasheed wrote: > I'm not a fan of the *serialize() function names in numeric.c though > (e.g., the name numeric_serialize() seems quite misleading for what it > actually does). So rather than adding to those, I've kept the original >

Re: Numeric multiplication overflow errors

2021-06-29 Thread Dean Rasheed
Thanks for looking! On Mon, 28 Jun 2021 at 12:27, David Rowley wrote: > > Instead of adding a send/recv function, unless I'm mistaken, it should > be possible to go the whole hog and optimizing this further by instead > of having numericvar_send(), add: > > static void numericvar_serialize(String

Re: Numeric multiplication overflow errors

2021-06-28 Thread David Rowley
On Mon, 28 Jun 2021 at 21:16, Dean Rasheed wrote: > So the second patch fixes this by adding new numericvar_send/recv() > functions capable of sending the full precision NumericVar's from each > worker, without rounding. The first test case in this patch is an > example that would round the wrong

Numeric multiplication overflow errors

2021-06-28 Thread Dean Rasheed
I found a couple of places where numeric multiplication suffers from overflow errors for inputs that aren't necessarily very large in magnitude. The first is with the numeric * operator, which attempts to always produce the exact result, even though the numeric type has a maximum of 16383 digits a