Re: array_agg(anyarray) silently produces corrupt results with parallel workers when inputs mix NULL and non-NULL array elements

Dmytro Astapov <dastapov(at)gmail(dot)com> writes:
> array_agg(ARRAY[...]) produces silently corrupted 2-D arrays when the query
> uses parallel partial aggregation and the input arrays contain a mix of
> NULL and non-NULL elements. NULL values appear at the wrong positions in
> the output, and non-NULL values disappear or shift.

Right you are.

> I think that the following two changes to array_agg_array_combine() in
> array_userfuncs.c fix the issue:

> 1. Change the condition guarding the null bitmap block from
> "if (state2->nullbitmap)" to
> "if (state1->nullbitmap || state2->nullbitmap)".

> 2. Change the bitmap reallocation size from
> "state1->aitems + state2->aitems" to
> "Max(state1->aitems + state2->aitems, newnitems)"
> to ensure the bitmap is always large enough.

This seems basically right, but I think we could simplify the code
some more: AFAICS the required bitmap size is newnitems, full stop.
The initial-setup path is confused about that too, allocating
newnitems+1 which is pointless.

It also troubled me that there's no checks for integer overflow
when calculating the new sizes. I believe that the pg_nextpower2_32
bits are okay even with large inputs (we'll end in palloc rejecting
the request size if there's an overflow), but if reqsize or newnitems
overflows it could be bad.

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?

regards, tom lane