Re: BUG #19857: Stats restore rejects empty arrays, losing range and array stats

On Wed, Nov 11, 2026 at 2:41 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:

> On Tue, Nov 10, 2026 at 08:50:39PM -0500, Corey Huinker wrote:
> > Same patch, commit message updated to have both discussion threads.
>
> arr = DatumGetArrayTypeP(PG_GETARG_DATUM(argnum));
>
> - if (ARR_NDIM(arr) != 1)
> + if (ARR_NDIM(arr) > 1)
> {
> ereport(WARNING,
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>

Nitpick: I think the error message itself needs to change based on
allow_empty, as true allows 1-D or empty, and false allows only 1-D.

> Instead of allowing all these arrays to be empty, we should snipe at
> range_length_histogram *and* most_common_elems, and only all these
> two without generalizing the rule. The other three are rejected
> anyway at load, so blocking them at restore would be a consistency
> move.
>

I thought about that, and wanted to at least try this method first. I'm
glad we could rule it out early.

Perhaps we should update get_attstatsslot() as well to avoid the
> over-read issue to begin with; that feels like a sloppy thing to do if
> there is an empty array to process.
>
> We'll likely need to do something similar for the extstats. One step
> at a time. We could of course do a more relaxing approach like you
> are proposing, but I cannot really get excited to let that go through
> at restore time as the loads would fail even if get_attstatsslot() is
> adjusted (actually this path should be adjusted anyway), and the
> option-basis thing is not that invasive in the attached, at least it
> seems so to me.
>

The more relaxing approach just means a lot more NDIM-checks, which undoes
the elegance of the initial fix.

This way seems not invasive at all, so I like it better.

> Attached is a cleaned-up patch. So, thoughts or comments?

I'll redo the ext stats patchset to this pattern. Do you intend to commit
this soon? Asking so I know whether to rebase-away the 0001 patch.