| From: | Jan Nidzwetzki <jan(at)planetscale(dot)com> |
|---|---|
| To: | Tomas Vondra <tomas(at)vondra(dot)me> |
| Cc: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: pg_class.reltuples can become non-finite and never recovers |
| Date: | 2026-07-27 12:06:56 |
| Message-ID: | 9887C8EB-A13E-4C74-BDF8-7048230320C9@planetscale.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hello Tomas,
> On 24. Jul 2026, at 15:55, Tomas Vondra <tomas(at)vondra(dot)me> wrote:
>>>
>>> 1. Uninitialized tuple counts in the parallel GIN build
>>> -------------------------------------------------------
[...]
> 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).
Thanks for looking into this and updating the patch. The updated version
looks good to me.
>>> 2. VACUUM can amplify a bad reltuples/relpages ratio to +Infinity
>>> ————————————————————————————————
[...]
>>> 3. Infinity defeats autovacuum's self-healing
>>> ———————————————————————
>>>
[...]
>>>
>>> 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.
I agree with that. My point in (2) was that there is some chance these
values tend to degrade toward infinity and it would be nice to
automatically fix obvious wrong entries. However, there is no guarantee
that these values degrade to infinity and as you pointed out, the other
cases are not addressed by this and they need some other kind of fix.
> 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?
This is also the only reliable way I see to fix all of the affected
installations.
Given all this, I'm happy to drop the autoanalyze patch. It covers too
narrow a slice, and value-based healing isn't the right layer. That
leaves two things worth pursuing: release-note guidance + manual ANALYZE
for already-affected installations, and validating/rejecting
clearly-bogus values at import (your point on section 4).
>>> 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 */
>>> }
[...]
>>>
>>> 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)?
This caught my attention as well. From my understanding, -1.0 should be
allowed, since in 3d351d916b2 the value -1 was introduced as a sentinel
meaning "unknown / never analyzed". But we also allow values between
-1.0 and 0 here. I think the looseness below zero is harmless because
every reader treats negative as "unknown".
For example, in plancat.c we do:
-+-
/* estimate number of tuples from previous tuple density */
if (reltuples >= 0 && relpages > 0)
density = reltuples / (double) relpages;
-+-
Same applies to checks in other places like in tableam.c. So, every
negative value is treated as unknown and the check here is a bit broader
than it has to be.
The direction that actually matters is the opposite one, though:
+Infinity passes both this check and the reltuples >= 0 tests. I think
it's worth rejecting at import.
Best regards
Jan
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Jakub Wartak | 2026-07-27 12:15:51 | Re: amcheck: add index-all-keys-match verification for B-Tree |
| Previous Message | Jimmy Angelakos | 2026-07-27 12:03:36 | Re: [PATCH] pg_dump: Restore extension config table data before user objects during pg_upgrade |