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?
>>
>> 4. Statistics import accepts Infinity and NaN
>> ---------------------------------------------
>>
>> Finally, the reltuples argument of pg_restore_relation_stats() (and the
>> shared relation_statistics_update_internal() path) is only validated
>> with:
>>
>> if (reltuples < -1.0) /* relation_stats.c:126 */
>> {
>> ereport(WARNING, ...);
>> result = false; /* update skipped */
>> }
>>
>> Both Infinity and NaN pass this check and are stored verbatim, while
>> an ordinary out-of-range value like -5 is correctly rejected:
>>
>> -+-
>> CREATE TABLE t();
>>
>> -- returns t, reltuples := Infinity
>> SELECT pg_restore_relation_stats('schemaname','public','relname','t',
>> 'version',190000,'reltuples','Infinity'::real);
>>
>> -- returns t, reltuples := NaN
>> SELECT pg_restore_relation_stats('schemaname','public','relname','t',
>> 'version',190000,'reltuples','NaN'::real);
>>
>> -- WARNING, returns f
>> SELECT pg_restore_relation_stats('schemaname','public','relname','t',
>> 'version',190000,'reltuples','-5'::real);
>> -+-
>>
>> Since pg_dump --statistics / pg_upgrade round-trip reltuples, a
>> corrupted value (from any of the above) also propagates across
>> dump/restore and major-version upgrades.
>>
>> We have not addressed this one in the attached patches, because
>> rejecting non-finite values here is a slightly larger policy question
>> (error vs. clamp-to -1). If there is agreement on the desired
>> behavior, we are glad to propose a patch for this as well.
>>
>
> OTOH this seems like something we might want to do, to validate and
> reject clearly bogus values.
>
No opinion on this. But I was checking how we validate the values when
importing stats, and I noticed relation_statistics_update_internal does
this:
if (reltuples < -1.0)
{
ereport(WARNING,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("argument \"%s\" must not be less than -1.0",
"reltuples")));
result = false;
}
Isn't that a bit strange it's not (reltuplees < 0.0)?
regards
--
Tomas Vondra
From 6be22ef20afdaaa718caee31a56a85f18fbed750 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <[email protected]>
Date: Fri, 24 Jul 2026 15:03:07 +0200
Subject: [PATCH v2 2/2] Force autoanalyze for bogus reltuples values
When checking if a relation needs ANALYZE, we calculate thresholds based
on pg_class.reltuples. If the value is bogus for some reason (e.g. due
to importing invalid stats or a bug somewhere), we may never trigger the
autovacuum/autoanalyze again.
Fixed by checking for incorrect reltuples values, and forcing ANALYZE.
Reported-by: Jan Nidzwetzki <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/postmaster/autovacuum.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 45abf48768a..3d6b4991679 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -3314,10 +3314,22 @@ relation_needs_vacanalyze(Oid relid,
if (relid != StatisticRelationId &&
classForm->relkind != RELKIND_TOASTVALUE)
{
+ double tupdensity = reltuples / Max(1.0, relpages);
+
scores->anl = (double) anltuples / Max(anlthresh, 1);
scores->anl *= autovacuum_analyze_score_weight;
scores->max = Max(scores->max, scores->anl);
- if (av_enabled && anltuples > anlthresh)
+
+ /*
+ * Analyze when enough tuples have changed.
+ *
+ * Force analyze when reltuples is not finite, or just impossibly
+ * high. The invalid value (e.g. from a bad statistics import)
+ * propagates into anlthresh, so "anltuples > anlthresh" may never
+ * hold and the bogus value would persist.
+ */
+ if (av_enabled &&
+ (anltuples > anlthresh || !isfinite(reltuples) || tupdensity > MaxHeapTuplesPerPage))
*doanalyze = true;
}
--
2.55.0
From 834ec73bad5967b99a1ab2497d02b4af532567bf Mon Sep 17 00:00:00 2001
From: Tomas Vondra <[email protected]>
Date: Fri, 24 Jul 2026 14:33:58 +0200
Subject: [PATCH v2 1/2] Initialize bs_reltuples field in GIN parallel builds
In parallel GIN builds, each worker reports the number of processed
table rows and reports it to the leader, who then stores the total as
thhe table's pg_class.reltuples. But gin_parallel_build_main failed to
initialize the bs_reltuples field of the worker's GinBuildState, before
accumulating the number of scanned tuples into it.
The initial value is therefore whatever was on the stack, and it may be
set to values like Infinity, NaN or extremely high value. That means the
total stored in pg_class.reltuples may be arbitrary too.
This can have serious consequences. The pg_class.reltuples value is used
to decide when a table is due for autovacuum or autoanalyze, and if it
happens to such bogus value, that may never happen. The value is also
used by query optimizer when calculating various costs.
Fixed by properly initializing bs_reltuples together with the rest of
the build state. The bs_numtuples was initialized later, but it seems
cleaner to just initialize all the fields at once.
Backpatch to 18, where parallel GIN builds were introduced.
Reported-by: Jan Nidzwetzki <[email protected]>
Discussion: https://postgr.es/m/[email protected]
Backpatch-through: 18
---
src/backend/access/gin/gininsert.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/backend/access/gin/gininsert.c b/src/backend/access/gin/gininsert.c
index cb9ed3b563c..16abb76c001 100644
--- a/src/backend/access/gin/gininsert.c
+++ b/src/backend/access/gin/gininsert.c
@@ -1875,9 +1875,6 @@ _gin_process_worker_data(GinBuildState *state, Tuplesortstate *worker_sort,
tuplesort_performsort(state->bs_worker_sort);
- /* reset the number of GIN tuples produced by this worker */
- state->bs_numtuples = 0;
-
if (progress)
pgstat_progress_update_param(PROGRESS_CREATEIDX_SUBPHASE,
PROGRESS_GIN_PHASE_MERGE_1);
@@ -2158,6 +2155,11 @@ _gin_parallel_build_main(dsm_segment *seg, shm_toc *toc)
/* initialize the GIN build state */
initGinState(&buildstate.ginstate, indexRel);
buildstate.indtuples = 0;
+
+ /* Initialize counters used to report tuple counts to the leader */
+ buildstate.bs_numtuples = 0;
+ buildstate.bs_reltuples = 0;
+
memset(&buildstate.buildStats, 0, sizeof(GinStatsData));
memset(&buildstate.tid, 0, sizeof(ItemPointerData));
--
2.55.0
