On 7/24/26 15:55, Tomas Vondra wrote: > On 7/20/26 15:02, Tomas Vondra wrote: >> On 7/20/26 14:40, Jan Nidzwetzki wrote: >>> Hi hackers, >>> >>> We recently tracked down an issue where pg_class.reltuples for a >>> heap relation had become Infinity, which in turn produced >>> cost=Infinity / NaN query plans. While investigating, we found a >>> chain of issues around reltuples that we would like to report, plus >>> two patches that address the parts we consider clear bugs. >>> >>> >>> 1. Uninitialized tuple counts in the parallel GIN build >>> ------------------------------------------------------- >>> >>> The root cause is in the parallel GIN build. _gin_parallel_build_main() >>> sets up a fresh GinBuildState in the worker but, unlike ginbuild(), >>> never initializes bs_numtuples or bs_reltuples. bs_numtuples happens to >>> be reset before use, but bs_reltuples is not: the worker accumulates its >>> scanned tuple count into it and publishes it to the leader, which stores >>> it as the heap relation's reltuples via index_update_stats(). >>> >>> The value read back is therefore whatever happened to be on the stack, >>> and the store goes through an unchecked narrowing cast: >>> >>> rd_rel->reltuples = (float4) reltuples; /* index.c:2960 */ >>> >>> The existing guard in index_update_stats() only rejects reltuples < 0, >>> which a large positive or +Infinity double passes. Any stack residue >>> whose double representation is >= FLT_MAX therefore lands in pg_class as >>> +Infinity; smaller residue lands as a bogus-but-finite count that is >>> unrelated to relpages. >>> >>> This was introduced together with the parallel GIN build: >>> >>> commit 8492feb98f6df3f0f03e84ed56f0d1cbb2ac514c >>> "Allow parallel CREATE INDEX for GIN indexes" >>> >>> which first appeared in v18. >>> >>> Fix in the first attached patch: initialize bs_numtuples and >>> bs_reltuples to 0 in the worker, matching how the leader's state is >>> set up. We recommend back-patching this to 18. >>> >> >> Yeah, this seems like my bug. I guess I got confused regarding what >> fields get initialized, and how the values propagate. I suppose we could >> also just change >> >> state->bs_reltuples += reltuples; >> >> to >> >> state->bs_reltuples = reltuples; >> >> And that'd fix the issue. But I agree it's the explicit initialization >> is better / cleaner. >> >> I'll get this fixed and backpatched. >> > > Here's a proposed fix - it's pretty much your patch, except that I > removed the second initialization of the bs_numtuples field (which would > be confusing, IMHO). And a slightly expanded commit message. > > There's not much to discuss, it's pretty clear (both the bug / fix). > >>> >>> 2. VACUUM can amplify a bad reltuples/relpages ratio to +Infinity >>> ----------------------------------------------------------------- >>> >>> Independently of how the bad value first appears, it can get worse >>> during normal operation. Once reltuples/relpages is inconsistent, a >>> partial VACUUM extrapolates the stored density over the unscanned >>> pages in vac_estimate_reltuples(): >>> >>> old_density * unscanned_pages + scanned_tuples >>> >>> and the result is again stored through an unchecked (float4) cast in >>> vac_update_relstats(). A merely large-but-finite reltuples can thus >>> flip to +Infinity after a routine VACUUM. We mention this mostly to >>> explain why the value tends to degrade toward Infinity in the field >>> rather than staying at a "just wrong" finite number. >>> >>> This is easy to demonstrate starting from statistics whose individual >>> values are perfectly valid but whose ratio is not: relpages = 1 and >>> reltuples = 3e38 are each finite, yet together they imply an >>> impossible density of 3e38 tuples per page: >>> >>> -+- >>> CREATE TABLE vac_inf (a int); >>> INSERT INTO vac_inf SELECT generate_series(1, 100000); >>> VACUUM ANALYZE vac_inf; >>> >>> SELECT pg_restore_relation_stats( >>> 'schemaname', 'public', 'relname', 'vac_inf', >>> 'version', 190000, >>> 'relpages', 1::integer, -- finite, valid >>> 'reltuples', '3e38'::real); -- finite, valid, near FLT_MAX >>> >>> INSERT INTO vac_inf SELECT generate_series(1, 10000); >>> VACUUM vac_inf; >>> >>> SELECT relpages, reltuples FROM pg_class WHERE relname = 'vac_inf'; >>> relpages | reltuples >>> ----------+----------- >>> 487 | Infinity >>> (1 row) >>> -+- >>> >>> The single VACUUM extrapolates the 3e38-per-page density over the >>> newly added pages and overflows float4. >>> >> >> Yeah. Garbage in, garbage out. > > Yeah. It's kinda orthogonal to the bug,, but I wonder if there's > something we can do to mitigate this. But it's hard, if we don't know > which of the values is wrong (if any). > > I guess we could refuse to update reltuples to non-finite value, but > that doesn't make the existing reltuples value any less bogus. And I'm > not sure it's less harmful than infinity in practice. > > >>> 3. Infinity defeats autovacuum's self-healing >>> ---------------------------------------------- >>> >>> Normally, a wrong reltuples is self-correcting: ANALYZE recomputes it >>> from a fresh sample. But a non-finite reltuples makes the analyze >>> threshold non-finite as well: >>> >>> anlthresh = analyze_threshold + analyze_scale_factor * reltuples >>> >>> so in relation_needs_vacanalyze() the test "anltuples > anlthresh" can >>> never be true, autoanalyze never fires, and the bad value is stuck >>> forever and the relation never recovers on its own. >>> >>> The second attached patch makes autovacuum force an analyze when >>> reltuples is not finite, so a corrupted value heals itself on the next >>> autovacuum cycle. It includes a TAP test that plants an infinite >>> reltuples and checks that autoanalyze restores a finite value. >>> >> >> I'm skeptical about this change. Yeah, it'd probably help in this >> particular Infinity case, but it can be bogus in many other ways. We >> should really be careful to not put bogus values into reltuples. >> > > I experimented with this a bit more, but I'm rather skeptical. > > I understand the desire to self-heal, but I don't think we do that > elsewhere. We assume the data is correct. I'm sure we had a bunch of > data corruption bugs, and I don't quite see why this particular case > would be any different. > > I don't think Infinity/NaN values are somewhat special - sure, they're > defined as special, but those are two singular values. If the stack has > a random value, what's the chance it's Infinity/Nan? > > Isn't it more likely it'll be a valid float value? And the consequences > will be almost exactly the same - the threshold may be so high it'll > never be hit, right? But the isfinite() check won't catch that. > > So I think we'd need to check the tuple density. The 0002 patch does > that, and it compares it to MaxHeapTuplesPerPage - it we got a higher > density, something has to be wrong and analyze is forced. > > But MaxHeapTuplesPerPage is still way higher than normal densities. It's > ~290 for 8K pages, and regular densities may be ~50, so an order of > magnitude less. So we still need to mitigate these cases, most likely by > suggesting a manual ANALYZE on some tables in the release notes. Maybe > we should just do that for all cases? > > Another possible argument just occurred to me - one of the statistics > import use cases is to allow messing with these fields, to import stats > to simulate a much larger database, etc. Wouldn't this self-healing > potentially interfere with that? > Oh, one more thing - I'm trying how to figure out how widespread this issue could be, which I guess could affect how far we want to go.
I did a lot of experiments, and I only ever saw the field initialized to 0 (so it produced the right result). Maybe I'm just lucky, but more likely it depends on platform / kernel version / compiler? What do I need to do to actually see a garbage value? In fact, not even valgrind reports the access as invalid. I guess that's one reason why I missed the issue when working on the feature :-/ regards -- Tomas Vondra
