Re: Why does pg_class.reltuples count only live tuples in indexes (after VACUUM runs)?
On Mon, Apr 18, 2022 at 1:12 PM Peter Geoghegan wrote: > I would argue that it would be correct for the first time -- at least > if we take the behavior within heapam_index_build_range_scan (and > everywhere else) as authoritative. That's a feature, not a bug. Attached draft patch shows what I have in mind. I think that the issue should be treated as a bug, albeit a minor one that's not worth backpatching. I would like to target Postgres 15 here. Addressing this issue allowed me to move more state out of vacrel. This patch continues the trend of moving everything that deals with pg_class, statistics, or instrumentation from lazy_scan_heap into its caller, heap_vacuum_rel(). I noticed GIN gives us another reason to go this way: ginvacuumcleanup() always instructs lazyvacuum.c to set each GIN index's pg_class.reltuples to whatever the value is that will be set in the heap relation, even with a partial index. So defining reltuples differently for indexes just doesn't seem reasonable to me. (Actually, lazyvacuum.c won't end up setting the value in the GIN index's pg_class entry when IndexVacuumInfo.estimated_count is set to true. But that hardly matters -- either way, num_index_tuples comes from num_heap_tuples in a GIN index, no matter what.) -- Peter Geoghegan v1-0001-Have-vacuum-use-live-reltuples-for-indexes.patch Description: Binary data
Re: Why does pg_class.reltuples count only live tuples in indexes (after VACUUM runs)?
On Mon, Apr 18, 2022 at 1:10 PM Tom Lane wrote: > The places where index AMs refer to num_heap_tuples seem to be using > it as a ceiling on estimated index tuple counts. Given that we should > be counting dead index entries, redefining it as you suggest would be > wrong. I would argue that it would be correct for the first time -- at least if we take the behavior within heapam_index_build_range_scan (and everywhere else) as authoritative. That's a feature, not a bug. -- Peter Geoghegan
Re: Why does pg_class.reltuples count only live tuples in indexes (after VACUUM runs)?
Peter Geoghegan writes: > I think that this doesn't really belong here; new_rel_tuples should > only be used for VACUUM VERBOSE/server log output, once we return to > heap_vacuum_rel from lazy_scan_heap. We should use > vacrel->new_live_tuples as our IndexVacuumInfo.num_heap_tuples value > in the amvacuumcleanup path (instead of new_rel_tuples). That way the > rule about IndexVacuumInfo.num_heap_tuples is simple: it's always > taken from pg_class.reltuples (for the heap rel). Either the existing > value, or the new value. The places where index AMs refer to num_heap_tuples seem to be using it as a ceiling on estimated index tuple counts. Given that we should be counting dead index entries, redefining it as you suggest would be wrong. regards, tom lane
Re: Why does pg_class.reltuples count only live tuples in indexes (after VACUUM runs)?
On Mon, Apr 18, 2022 at 12:41 PM Tom Lane wrote: > If the planner looks at index reltuples at all, it's doing so > for cost estimation purposes, where the count including dead > entries is probably the right thing to use. Then why does heapam_index_build_range_scan do it the other way around? I think that it probably doesn't matter that much in practice. The inconsistency should be noted in update_relstats_all_indexes, though. > If you want to make this cleaner, maybe there's a case for > splitting reltuples into two columns. But then index AMs > would be on the hook to determine how many of their entries > are live, which is not really an index's concern. The main concern behind this is that we're using vacrel->new_rel_tuples for the IndexVacuumInfo.num_heap_tuples value in amvacuumcleanup (but not in ambulkdelete), which is calculated towards the end of lazy_scan_heap, like so: /* * Also compute the total number of surviving heap entries. In the * (unlikely) scenario that new_live_tuples is -1, take it as zero. */ vacrel->new_rel_tuples = Max(vacrel->new_live_tuples, 0) + vacrel->recently_dead_tuples + vacrel->missed_dead_tuples; I think that this doesn't really belong here; new_rel_tuples should only be used for VACUUM VERBOSE/server log output, once we return to heap_vacuum_rel from lazy_scan_heap. We should use vacrel->new_live_tuples as our IndexVacuumInfo.num_heap_tuples value in the amvacuumcleanup path (instead of new_rel_tuples). That way the rule about IndexVacuumInfo.num_heap_tuples is simple: it's always taken from pg_class.reltuples (for the heap rel). Either the existing value, or the new value. -- Peter Geoghegan
Re: Why does pg_class.reltuples count only live tuples in indexes (after VACUUM runs)?
Peter Geoghegan writes: > On Mon, Apr 18, 2022 at 12:15 PM Tom Lane wrote: >> Huh? This is not pg_class.reltuples. If an index AM wants that, it >> knows where to find it. > It's not, but it is how we calculate > IndexBulkDeleteResult.num_index_tuples, which is related. Well, the number of entries in an index needn't be exactly the same as the number in the underlying heap. If we're setting an index's reltuples to the number of actual index entries including dead entries, I don't have a problem with that: unlike the case for table reltuples, it's not going to result in bad estimates of the number of rows resulting from a query. If the planner looks at index reltuples at all, it's doing so for cost estimation purposes, where the count including dead entries is probably the right thing to use. If you want to make this cleaner, maybe there's a case for splitting reltuples into two columns. But then index AMs would be on the hook to determine how many of their entries are live, which is not really an index's concern. regards, tom lane
Re: Why does pg_class.reltuples count only live tuples in indexes (after VACUUM runs)?
On Mon, Apr 18, 2022 at 12:15 PM Tom Lane wrote: > > I don't see why it makes sense to treat indexes differently here. Why > > allow the special case? Why include dead tuples like this? > > The index has presumably got entries corresponding to dead tuples, > so that the number of entries it has ought to be more or less > num_heap_tuples, not reltuples (with discrepancies for concurrent > insertions of course). I guess that pg_class.reltuples has to include some "recently dead" tuples in the case of an index, just because of the impracticality of accurately counting index tuples while knowing if they're dead or alive. However, it would be practical to update pg_class.reltuples to a value "IndexBulkDeleteResult.num_index_tuples - recently_dead_tuples" in update_relstats_all_indexes to compensate. Then everything is consistent. > > We make a general assumption that pg_class.reltuples only includes > > live tuples, which this code contravenes. > > Huh? This is not pg_class.reltuples. If an index AM wants that, it > knows where to find it. It's not, but it is how we calculate IndexBulkDeleteResult.num_index_tuples, which is related. Granted, that won't be used to update pg_class for the index in the case where it's just an estimate anyway. -- Peter Geoghegan
Re: Why does pg_class.reltuples count only live tuples in indexes (after VACUUM runs)?
Peter Geoghegan writes: > Commit 7c91a0364f standardized the approach we take to estimating > pg_class.reltuples, so that everybody agrees on what that means. > Follow-up work by commit 3d351d91 defined a pg_class.reltuples of -1 > as "unknown, probably never vacuumed". > The former commit added this code and comment to vacuumlazy.c: > /* > * Now we can provide a better estimate of total number of surviving > * tuples (we assume indexes are more interested in that than in the > * number of nominally live tuples). > */ > ivinfo.num_heap_tuples = vacrelstats->new_rel_tuples; > ivinfo.strategy = vac_strategy; > I don't see why it makes sense to treat indexes differently here. Why > allow the special case? Why include dead tuples like this? The index has presumably got entries corresponding to dead tuples, so that the number of entries it has ought to be more or less num_heap_tuples, not reltuples (with discrepancies for concurrent insertions of course). > We make a general assumption that pg_class.reltuples only includes > live tuples, which this code contravenes. Huh? This is not pg_class.reltuples. If an index AM wants that, it knows where to find it. regards, tom lane
Why does pg_class.reltuples count only live tuples in indexes (after VACUUM runs)?
Commit 7c91a0364f standardized the approach we take to estimating pg_class.reltuples, so that everybody agrees on what that means. Follow-up work by commit 3d351d91 defined a pg_class.reltuples of -1 as "unknown, probably never vacuumed". The former commit added this code and comment to vacuumlazy.c: /* * Now we can provide a better estimate of total number of surviving * tuples (we assume indexes are more interested in that than in the * number of nominally live tuples). */ ivinfo.num_heap_tuples = vacrelstats->new_rel_tuples; ivinfo.strategy = vac_strategy; I don't see why it makes sense to treat indexes differently here. Why allow the special case? Why include dead tuples like this? We make a general assumption that pg_class.reltuples only includes live tuples, which this code contravenes. It's quite clear that indexes are no exception to the general rule, since CREATE INDEX quite deliberately does reltuples accounting in a way that fits with the usual definition (live tuples only), per comments in heapam_index_build_range_scan. One of these code paths must be doing it wrong -- I think it's vacuumlazy.c. This also confuses the index AM definitions. Whenever we call ambulkdelete routines, IndexVacuumInfo.num_heap_tuples will always come from the heap relation's existing pg_class.reltuples, which could even be -1 -- so clearly its value can only be a count of live tuples. On the other hand IndexVacuumInfo.num_heap_tuples might include some dead tuples when we call amvacuumcleanup routines, since (as shown) the value comes from vacuumlazy.c's vacrelstats->new_rel_tuples. It would be more logical if IndexVacuumInfo.num_heap_tuples was always the pg_class.reltuples for the table (either the original/existing value, or the value that it's just about to be updated to). That said, I can see why we wouldn't want to allow pg_class.reltuples to ever be -1 in the case of an index. So I think we should bring vacuumlazy.c in line with everything else here, without allowing that case. I believe that the "pg_class.reltuples is -1 even after a VACUUM" case is completely impossible following the Postgres 15 work on VACUUM, but we should still clamp for safety in update_relstats_all_indexes (though not in the amvacuumcleanup path). -- Peter Geoghegan