Re: pg_restore_attribute_stats() accepts non-finite values

Thanks for the thorough review, and for the history -- that context on why
the checks were removed is helpful.

On Thu, Aug 27, 2026 at 5:21 AM Corey Huinker <corey(dot)huinker(at)gmail(dot)com> wrote:
>
> On Tue, Aug 25, 2026 at 7:36 AM Ewan Young <kdbase(dot)hack(at)gmail(dot)com> wrote:
>>
>> Hi,
>>
>> 7cb9060dcde taught pg_restore_relation_stats() to reject a non-finite
>> reltuples, on the grounds that Infinity and NaN pass the existing range
>> check and then get stored and used verbatim. pg_restore_attribute_stats()
>> has the same gap for its float arguments, which that thread did not cover.
>>
>> CREATE TABLE t (a int);
>> INSERT INTO t SELECT g FROM generate_series(1, 1000) g;
>> ANALYZE t;
>>
>> SELECT pg_restore_attribute_stats('schemaname', 'public', 'relname', 't',
>> 'attname', 'a', 'inherited', false,
>> 'null_frac', 'NaN'::real, 'n_distinct', 'Infinity'::real);
>> t
>>
>> SELECT stanullfrac, stadistinct FROM pg_statistic
>> WHERE starelid = 't'::regclass;
>> NaN | Infinity
>>
>> The values are stored, and the planner does not defend against them.
>> CLAMP_PROBABILITY() is two comparisons, both false for NaN, so it does not
>> neutralise a non-finite value the way it clamps an out-of-range finite one.
>> The effect is visible immediately:
>>
>> -- with the NaN null_frac above:
>> EXPLAIN SELECT * FROM t WHERE a = 5;
>> Seq Scan on t (cost=0.00..17.50 rows=10000000000...000 width=4)
>>
>> A non-finite n_distinct or correlation is worse than a bad row estimate: a
>> NaN correlation puts a literal "cost=0.29..NaN" on an index scan, which
>> then takes part in path cost comparisons.
>>
>> This only comes in through the restore path -- ANALYZE never produces a
>> non-finite value, even for a column that itself contains Infinity/NaN,
>> since the stats are frequencies and ratios rather than the data. So the
>> realistic trigger is a corrupt or cross-version dump fed through
>> pg_restore_attribute_stats(), and once stored the value survives until the
>> next ANALYZE.
>>
>> Patch attached. It rejects non-finite values for the scalar arguments
>> null_frac, n_distinct, correlation and range_empty_frac, and for the
>> float4[] arguments most_common_freqs, most_common_elem_freqs and
>> elem_count_histogram, dropping the bad value with a WARNING as the other
>> non-fatal checks do and letting the rest of the import proceed. The two
>> new checks live in stat_utils.c alongside the existing ones.
>>
>> Two things I decided deliberately, happy to be overruled:
>>
>> - A negative n_distinct encodes a distinct-value ratio rather than a
>> count, so it is still accepted, matching the -1.0 special case kept for
>> reltuples.
>>
>> - These functions do only superficial validation by design (per
>> ce207d2a790), so I did not add range checks for finite-but-bogus values;
>> the planner does clamp those. This only closes the non-finite hole,
>> which the planner cannot.
>>
>> --
>> Regards,
>> Ewan Young
>
>
>
> Back when this was being developed, there were extremely tight proposed checks [1] on all parameters.
>
> At the time, the need for that was questioned [2], though the relevant comment was focusing on the array parameters.
>
> > I'm dubious that we can fully vet the contents of these arrays,
> > and even a little dubious that we need to try. As an example,
> > what's the worst that's going to happen if a histogram array isn't
> > sorted precisely? You might get bogus selectivity estimates
> > from the planner, but that's no worse than you would've got with
> > no stats at all.
> ...
> > We do need to verify data types, lack of nulls, and maybe
> > 1-dimensional-ness, which could break the accessing code at a fairly
> > low level; but I'm not sure that we need more than that.
>
> Later, it was suggested that leaving such checks out was a form of fuzzing tool [3].
>
> > It could be argued that feeding bogus data to the planner for testing
> > purposes is a valid use-case for this feature. (Of course, as
> > superuser we could inject bogus data into pg_statistic manually,
> > so it's not necessary to have this feature for that purpose.)
> > I guess I'm a great deal more sanguine than other people about the
> > planner's ability to tolerate inconsistent data; but in any case
> > I don't have a lot of faith in relying on checks in
> > pg_set_attribute_stats to substitute for that ability. That idea
> > mainly leads to having a whole lot of code that has to be kept in
> > sync with other code that's far away from it and probably isn't
> > coded in a parallel fashion either.
>
> The net result was I removed most of the proposed data validation checks. So every time we add one such check in (or back in, depending on your perspective), we need to balance the value of the check vs the burden of the code sync that goes with it. We're clearly on a trajectory for re-adding checks like this, so I'm in favor of a patch like this one.
>
> As for the patch itself, there's currently a thread [4] proposing a change from FunctionCallInfo to NullableDatum[] for the stats args, so this patch would have to be coordinated with that.

Happy to rebase onto that whenever it lands -- the finite checks are a
mechanical conversion (PG_ARGISNULL(n) -> args[n].isnull, PG_GETARG_DATUM(n)
-> args[n].value). Since you're driving both, I'll follow whatever order
you prefer.

>
> I like the stats_check_arg_finite() and how it is used.
>
> I'm less happy with the change to stats_check_arg_array(), specifically adding the is_finite check based on whether it happens to be float4 or not, rather than whether we know we need it. Currently those two things are in sync, but they may not be in the future. I'm especially concerned about future stat types covering values of user-defined datatypes, which store as an ANYARRAY which would then conditionally execute based on the datatype the user had chosen. I grant you that's a weird hypothetical, but it would result in very POLA-violating behavior. Maybe the better thing is to have a separate check.

Agreed, and done in v2. stats_check_arg_array() is back to being a purely
structural check (1-D, no NULLs). The finiteness test now lives in a
separate stats_check_arg_array_finite(), which is called explicitly for the
three arguments that must be finite -- most_common_freqs,
most_common_elem_freqs and elem_count_histogram -- rather than being keyed
on the element type. So the check is driven by "we know this argument is a
frequency/count array that has to be finite", not by "this array happens to
be float4". A future stat type that passes user data values through an
ANYARRAY won't accidentally pick it up, even if the user's type is float4.

>
> Another concern about the array value testing is that if we're walking back the suggestion made in [2], then do we also bring back things like making sure that the frequency arrays are monotonically non-increasing? We will need some sort of consensus on where to draw the new line.

I'd keep this patch to finiteness only, and leave monotonicity for the
consensus discussion. They're different in kind: a NaN/infinity isn't
neutralized by CLAMP_PROBABILITY() and propagates into selectivity and cost
estimates, and ANALYZE never emits one, so rejecting it is unambiguous. A
non-monotonic histogram is exactly the "bogus selectivity, no worse than no
stats" case from [2] -- softer, and more of a policy call about where the
new line goes. Happy to help draw that line separately.

>
> The test cases seem sufficient for the time being.
>
> --
>
> [1] https://www.postgresql.org/message-id/CADkLM=e=_6dtacmrvd2NJWacOnQ3Zu5iaRZFgePL1=0L5-7P_w@mail.gmail.com
>
> [2] https://www.postgresql.org/message-id/790773.1711910899@sss.pgh.pa.us
>
> [3] https://www.postgresql.org/message-id/864345.1711932452@sss.pgh.pa.us
> note: the quote cites a function named pg_set_attribute_stats which was eventually renamed to pg_restore_attribute_stats
>
> [4] https://www.postgresql.org/message-id/flat/CADkLM%3Deo7MtuCE%3DYjovW%2B%3DASw1%3Dq39qQ3qarrsw%2BEKfU901ztA%40mail.gmail.com

--
Regards,
Ewan Young