Set calcSumX2 = true in numeric_(poly_)deserialize
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t139643psql -h localhost -U postgresBuilt from patchset v1 (message #1), September 29, 2026 at 03:27 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t139643_1 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t139643_1 && git checkout t139643_1Patchset v1 (message #1) is on t139643_1
Hi hackers,
numeric_poly_deserialize and numeric_deserialize describe themselves
(in their header comments) as the deserializers for aggregate functions
which require sumX2: var_pop, var_samp, stddev_pop, stddev_samp, and
the regr_* family. Both nevertheless construct their result with
calcSumX2 = false:
/* numeric_poly_deserialize */
result = makeInt128AggStateCurrentContext(false);
/* numeric_deserialize */
result = makeNumericAggStateCurrentContext(false);
Within the in-tree pipeline this is harmless: deserialize is only ever
followed by numeric_(poly_)combine, neither of which reads the flag.
But for callers that follow a deserialize with a transition call
(extensions round-tripping aggregate state through external storage,
in my case), it's wrong: do_int128_accum / do_numeric_accum gate
"sumX2 += value * value" on state->calcSumX2, so a deserialized
var/stddev state silently stops accumulating sumX2 while N and sumX
continue to update.
There's precedent for setting the flag to its semantically correct
value even when the in-tree pipeline doesn't observe it: numeric_combine
creates its fresh state with calcSumX2 = true. Extending that convention
to the deserialize side would align the two sites.
I've attached a two-line patch. Would this be worth fixing, or is
there a reason the current default is intentional that I'm missing?
Thanks,
Maxime Schoemans
Bumping this, it's a small (2-line), low-risk fix and hasn't gotten any
feedback yet. I've added it to the current commitfest
(https://commitfest.postgresql.org/patch/7334/) if anyone has a moment
to take a look.
Thanks,
Maxime
Hi Maxime,
On 14 May 2026, Maxime Schoemans wrote:
Would this be worth fixing, or is
there a reason the current default is intentional that I'm missing?
The patch looks correct to me based on the original discussion from
2016 [0]/messages/by-id/CAKJS1f8AtOiUaQb1ipz--K5G1aXH4+GUEsttAYS9GpKhk2_9tA@mail.gmail.com.
David, since you wrote the original code, could you take a look when
you have time? I realize everyone is busy with the release, so this
can certainly wait until afterwards.
Thank you!
Best regards, Andrey Borodin.
[0]: /messages/by-id/CAKJS1f8AtOiUaQb1ipz--K5G1aXH4+GUEsttAYS9GpKhk2_9tA@mail.gmail.com
On Fri, 15 May 2026 at 05:39, Maxime Schoemans
<maxime.schoemans@enterprisedb.com> wrote:
I've attached a two-line patch. Would this be worth fixing, or is
there a reason the current default is intentional that I'm missing?
Is this something you need fixed in backbraches, or is master enough?
David
On 23 Sep 2026, at 23:55, David Rowley <dgrowleyml@gmail.com> wrote:
Is this something you need fixed in backbraches, or is master enough?
I don’t think this needs backpatching, master should be enough.
Best,
Maxime
On Thu, 24 Sept 2026 at 21:48, Maxime Schoemans
<maxime.schoemans@enterprisedb.com> wrote:
On 23 Sep 2026, at 23:55, David Rowley <dgrowleyml@gmail.com> wrote:
Is this something you need fixed in backbraches, or is master enough?
I don’t think this needs backpatching, master should be enough.
I ended up backpatching this. My thoughts are that doing that has a
lower chance of having to revisit than not backpatching.
David
On 29 Sep 2026, at 04:53, David Rowley <dgrowleyml@gmail.com> wrote:
I ended up backpatching this. My thoughts are that doing that has a
lower chance of having to revisit than not backpatching.
Sounds good, thank you very much for committing this.
Best,
Maxime