22.03.2023 06:07, Richard Guo wrote:
Alexander Lakhin <exclusion@gmail.com> writes:
> I'm afraid that zeroing only bytes behind nitems bits is not enough, as outDatum() doesn't bother to calculate the exact
> size of nulls bitmap, it just outputs all bytes of a datum (40 bytes in that case):
In that case, won't padding bytes between array elements also create
issues? Seems like we have to just zero the whole array area, like
those other functions do.
Yeah, this should be the right fix, to use palloc0 instead here. FWIW
currently in the codes there are 14 places that explicitly allocate
ArrayType, 13 of them using palloc0, the only exception is the one
discussed here.
BTW, in array_set_slice() and array_set_element() we explicitly zero the
null bitmap although the whole array area is allocated with palloc0. Is
this necessary?
I had also wondered, how come this call of array_bitmap_copy() shown as covered here:
https://coverage.postgresql.org/src/backend/executor/execExprInterp.c.gcov.html but valgrind doesn't complain during `make check` (with WRITE_READ_PARSE_PLAN_TREES).
I've found the query, which is reaching that call:
select percentile_disc(array[[null,1,0.5],[0.75,0.25,null]]) within group (order by thousand)
from tenk1;
and the absence of the valgrind complaint here is explained by the array coercion performed implicitly.
The query modification:
select percentile_disc(array[[null,1,0.5],[0.75,0.25,null]]::float8[]) within group (order by thousand)
from tenk1;
can confirm the fix (if needed).
With the following check:
if (result) VALGRIND_CHECK_MEM_IS_DEFINED(result, VARSIZE(result));
added at the end of ExecEvalArrayExpr() I see no failures during `make check` (except for select percentile_disc(...)) so it seems that uninitialised padding bytes between array elements are not produced with the existing regression tests. Though a comment in array.h says:
* The actual data starts on a MAXALIGN boundary. Individual items in the
* array are aligned as specified by the array element type. ...
I'll try to find cases where the alignment padding is possible...
Though if palloc0() used in 13 places, maybe the minor optimization (with zeroing only the null bitmap) doesn't justify the inconsistency.
Best regards,
Alexander