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 <[email protected]> wrote: > > On Tue, Aug 25, 2026 at 7:36 AM Ewan Young <[email protected]> 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-7...@mail.gmail.com > > [2] https://www.postgresql.org/message-id/[email protected] > > [3] https://www.postgresql.org/message-id/[email protected] > 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
v2-0001-Reject-non-finite-values-when-restoring-attribute-st.patch
Description: Binary data
