public inbox for [email protected]
help / color / mirror / Atom feedFrom: Dmytro Astapov <[email protected]>
To: Tom Lane <[email protected]>
Cc: [email protected]
Subject: Re: array_agg(anyarray) silently produces corrupt results with parallel workers when inputs mix NULL and non-NULL array elements
Date: Mon, 6 Apr 2026 13:04:18 +0100
Message-ID: <CAFQUnFiK=j6AudmHLWsdFTazeUd3o3CgEKiqk97V-1kSibfaZA@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAFQUnFj2pQ1HbGp69+w2fKqARSfGhAi9UOb+JjyExp7kx3gsqA@mail.gmail.com>
<[email protected]>
On Sat, Apr 4, 2026 at 5:41 PM Tom Lane <[email protected]> wrote:
> Dmytro Astapov <[email protected]> writes:
>
> The initial-setup path is confused about that too, allocating
> newnitems+1 which is pointless.
>
Yes, using newnitems directly is cleaner than my Max() approach, thank you.
>
> It also troubled me that there's no checks for integer overflow
> when calculating the new sizes.
Good catch, I hadn't considered that.
> So I end with the attached revised patch, where I also made one
> or two cosmetic adjustments like putting the type-comparison checks
> next to the dimension comparisons. Look good to you?
>
Looks good, thank you for improving it.
I built and tested your v2 patch on REL_17_9 and REL_18_3 (with minor
adaptation for the slightly different context lines, like bits8 vs uint8 on
17.x), using the same 10M-row synthetic reproduction from my original
report. They both pass (as expected).
I am attaching the amended patch files for REL_17_9 and REL_18_3 just in
case.
Thank you for the feedback and for the quick turnaround on this!
Best regards, Dmytro
Attachments:
[text/x-patch] v2-fix_array_agg_parallel_nullbitmap-REL_17_9.patch (3.2K, 3-v2-fix_array_agg_parallel_nullbitmap-REL_17_9.patch)
download | inline diff:
diff --git a/src/backend/utils/adt/array_userfuncs.c b/src/backend/utils/adt/array_userfuncs.c
index 6599be2ec5e..bc8b59134e3 100644
--- a/src/backend/utils/adt/array_userfuncs.c
+++ b/src/backend/utils/adt/array_userfuncs.c
@@ -21,6 +21,7 @@
#include "utils/builtins.h"
#include "utils/datum.h"
#include "utils/lsyscache.h"
+#include "utils/memutils.h"
#include "utils/typcache.h"
/*
@@ -957,10 +958,11 @@ array_agg_array_combine(PG_FUNCTION_ARGS)
}
/* We only need to combine the two states if state2 has any items */
- else if (state2->nitems > 0)
+ if (state2->nitems > 0)
{
MemoryContext oldContext;
- int reqsize = state1->nbytes + state2->nbytes;
+ int reqsize;
+ int newnitems;
int i;
/*
@@ -983,6 +985,17 @@ array_agg_array_combine(PG_FUNCTION_ARGS)
errmsg("cannot accumulate arrays of different dimensionality")));
}
+ /* Types should match already. */
+ Assert(state1->array_type == state2->array_type);
+ Assert(state1->element_type == state2->element_type);
+
+ /* Calculate new sizes, guarding against overflow. */
+ if (pg_add_s32_overflow(state1->nbytes, state2->nbytes, &reqsize) ||
+ pg_add_s32_overflow(state1->nitems, state2->nitems, &newnitems))
+ ereport(ERROR,
+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("array size exceeds the maximum allowed (%zu)",
+ MaxArraySize)));
oldContext = MemoryContextSwitchTo(state1->mcontext);
@@ -997,17 +1010,16 @@ array_agg_array_combine(PG_FUNCTION_ARGS)
state1->data = (char *) repalloc(state1->data, state1->abytes);
}
- if (state2->nullbitmap)
+ /* Combine the null bitmaps, if present. */
+ if (state1->nullbitmap || state2->nullbitmap)
{
- int newnitems = state1->nitems + state2->nitems;
-
if (state1->nullbitmap == NULL)
{
/*
* First input with nulls; we must retrospectively handle any
* previous inputs by marking all their items non-null.
*/
- state1->aitems = pg_nextpower2_32(Max(256, newnitems + 1));
+ state1->aitems = pg_nextpower2_32(Max(256, newnitems));
state1->nullbitmap = (bits8 *) palloc((state1->aitems + 7) / 8);
array_bitmap_copy(state1->nullbitmap, 0,
NULL, 0,
@@ -1015,17 +1027,17 @@ array_agg_array_combine(PG_FUNCTION_ARGS)
}
else if (newnitems > state1->aitems)
{
- int newaitems = state1->aitems + state2->aitems;
-
- state1->aitems = pg_nextpower2_32(newaitems);
+ state1->aitems = pg_nextpower2_32(newnitems);
state1->nullbitmap = (bits8 *)
repalloc(state1->nullbitmap, (state1->aitems + 7) / 8);
}
+ /* This will do the right thing if state2->nullbitmap is NULL: */
array_bitmap_copy(state1->nullbitmap, state1->nitems,
state2->nullbitmap, 0,
state2->nitems);
}
+ /* Finally, combine the data and adjust sizes. */
memcpy(state1->data + state1->nbytes, state2->data, state2->nbytes);
state1->nbytes += state2->nbytes;
state1->nitems += state2->nitems;
@@ -1033,9 +1045,6 @@ array_agg_array_combine(PG_FUNCTION_ARGS)
state1->dims[0] += state2->dims[0];
/* remaining dims already match, per test above */
- Assert(state1->array_type == state2->array_type);
- Assert(state1->element_type == state2->element_type);
-
MemoryContextSwitchTo(oldContext);
}
[text/x-patch] v2-fix_array_agg_parallel_nullbitmap-REL_18_3.patch (3.3K, 4-v2-fix_array_agg_parallel_nullbitmap-REL_18_3.patch)
download | inline diff:
diff --git a/src/backend/utils/adt/array_userfuncs.c b/src/backend/utils/adt/array_userfuncs.c
index 8eb342e3382..f2d588fd4a3 100644
--- a/src/backend/utils/adt/array_userfuncs.c
+++ b/src/backend/utils/adt/array_userfuncs.c
@@ -24,6 +24,7 @@
#include "utils/builtins.h"
#include "utils/datum.h"
#include "utils/lsyscache.h"
+#include "utils/memutils.h"
#include "utils/tuplesort.h"
#include "utils/typcache.h"
@@ -1033,10 +1034,11 @@ array_agg_array_combine(PG_FUNCTION_ARGS)
}
/* We only need to combine the two states if state2 has any items */
- else if (state2->nitems > 0)
+ if (state2->nitems > 0)
{
MemoryContext oldContext;
- int reqsize = state1->nbytes + state2->nbytes;
+ int reqsize;
+ int newnitems;
int i;
/*
@@ -1059,6 +1061,17 @@ array_agg_array_combine(PG_FUNCTION_ARGS)
errmsg("cannot accumulate arrays of different dimensionality")));
}
+ /* Types should match already. */
+ Assert(state1->array_type == state2->array_type);
+ Assert(state1->element_type == state2->element_type);
+
+ /* Calculate new sizes, guarding against overflow. */
+ if (pg_add_s32_overflow(state1->nbytes, state2->nbytes, &reqsize) ||
+ pg_add_s32_overflow(state1->nitems, state2->nitems, &newnitems))
+ ereport(ERROR,
+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("array size exceeds the maximum allowed (%zu)",
+ MaxArraySize)));
oldContext = MemoryContextSwitchTo(state1->mcontext);
@@ -1073,17 +1086,16 @@ array_agg_array_combine(PG_FUNCTION_ARGS)
state1->data = (char *) repalloc(state1->data, state1->abytes);
}
- if (state2->nullbitmap)
+ /* Combine the null bitmaps, if present. */
+ if (state1->nullbitmap || state2->nullbitmap)
{
- int newnitems = state1->nitems + state2->nitems;
-
if (state1->nullbitmap == NULL)
{
/*
* First input with nulls; we must retrospectively handle any
* previous inputs by marking all their items non-null.
*/
- state1->aitems = pg_nextpower2_32(Max(256, newnitems + 1));
+ state1->aitems = pg_nextpower2_32(Max(256, newnitems));
state1->nullbitmap = (bits8 *) palloc((state1->aitems + 7) / 8);
array_bitmap_copy(state1->nullbitmap, 0,
NULL, 0,
@@ -1091,17 +1103,17 @@ array_agg_array_combine(PG_FUNCTION_ARGS)
}
else if (newnitems > state1->aitems)
{
- int newaitems = state1->aitems + state2->aitems;
-
- state1->aitems = pg_nextpower2_32(newaitems);
+ state1->aitems = pg_nextpower2_32(newnitems);
state1->nullbitmap = (bits8 *)
repalloc(state1->nullbitmap, (state1->aitems + 7) / 8);
}
+ /* This will do the right thing if state2->nullbitmap is NULL: */
array_bitmap_copy(state1->nullbitmap, state1->nitems,
state2->nullbitmap, 0,
state2->nitems);
}
+ /* Finally, combine the data and adjust sizes. */
memcpy(state1->data + state1->nbytes, state2->data, state2->nbytes);
state1->nbytes += state2->nbytes;
state1->nitems += state2->nitems;
@@ -1109,9 +1121,6 @@ array_agg_array_combine(PG_FUNCTION_ARGS)
state1->dims[0] += state2->dims[0];
/* remaining dims already match, per test above */
- Assert(state1->array_type == state2->array_type);
- Assert(state1->element_type == state2->element_type);
-
MemoryContextSwitchTo(oldContext);
}
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: array_agg(anyarray) silently produces corrupt results with parallel workers when inputs mix NULL and non-NULL array elements
In-Reply-To: <CAFQUnFiK=j6AudmHLWsdFTazeUd3o3CgEKiqk97V-1kSibfaZA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox