Re: BUG #14057: vacuum setting reltuples=0 for tables with >0 tuples

On 2017-03-16 06:55:54 +0000, Andrew Gierth wrote:
> >>>>> "Andres" == Andres Freund <andres(at)anarazel(dot)de> writes:
>
> >> It looks like fixing this requires breaking scanned_pages out into
> >> at least two separate counters, since we currently use it to track
> >> both whether we can update stats, and whether we can update
> >> relfrozenxid.
>
> Andres> Are you planning to submit a fix?
>
> Somewhat belatedly, yes.
>
> Attached are patches against master and all the back branches back to
> 9.2.

Cool.

> I've reproduced the bug on all of them, and confirmed that this
> fixes it on all of them. Is it worth also including the isolation
> tester script in the changes?

Hm, I haven't seen the isolationtester test (it's not in this thread,
right?) - how fragile and how slow is it?

> diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
> index 8aefa7aaa9..a33541a115 100644
> --- a/src/backend/commands/vacuumlazy.c
> +++ b/src/backend/commands/vacuumlazy.c
> @@ -100,7 +100,8 @@ typedef struct LVRelStats
> BlockNumber old_rel_pages; /* previous value of pg_class.relpages */
> BlockNumber rel_pages; /* total number of pages */
> BlockNumber scanned_pages; /* number of pages we examined */
> - double scanned_tuples; /* counts only tuples on scanned pages */
> + BlockNumber tupcount_pages; /* pages whose tuples we counted */
> + double scanned_tuples; /* counts only tuples on tupcount_pages */
> double old_rel_tuples; /* previous value of pg_class.reltuples */
> double new_rel_tuples; /* new estimated total # of tuples */
> BlockNumber pages_removed;
> @@ -258,6 +259,10 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
> * density") with nonzero relpages and reltuples=0 (which means "zero
> * tuple density") unless there's some actual evidence for the latter.
> *
> + * It's important that we use tupcount_pages and not scanned_pages for the
> + * check described above; scanned_pages counts pages where we could not get
> + * cleanup lock, and which were processed only for frozenxid purposes.
> + *
> * We do update relallvisible even in the corner case, since if the table
> * is all-visible we'd definitely like to know that. But clamp the value
> * to be not more than what we're setting relpages to.
> @@ -267,7 +272,7 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
> */
> new_rel_pages = vacrelstats->rel_pages;
> new_rel_tuples = vacrelstats->new_rel_tuples;
> - if (vacrelstats->scanned_pages == 0 && new_rel_pages > 0)
> + if (vacrelstats->tupcount_pages == 0 && new_rel_pages > 0)
> {
> new_rel_pages = vacrelstats->old_rel_pages;
> new_rel_tuples = vacrelstats->old_rel_tuples;
> @@ -423,6 +428,7 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
> nblocks = RelationGetNumberOfBlocks(onerel);
> vacrelstats->rel_pages = nblocks;
> vacrelstats->scanned_pages = 0;
> + vacrelstats->tupcount_pages = 0;
> vacrelstats->nonempty_pages = 0;
> vacrelstats->latestRemovedXid = InvalidTransactionId;
>
> @@ -613,6 +619,7 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
> }
>
> vacrelstats->scanned_pages++;
> + vacrelstats->tupcount_pages++;
>
> page = BufferGetPage(buf);
>
> @@ -999,7 +1006,7 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
> /* now we can compute the new value for pg_class.reltuples */
> vacrelstats->new_rel_tuples = vac_estimate_reltuples(onerel, false,
> nblocks,
> - vacrelstats->scanned_pages,
> + vacrelstats->tupcount_pages,
> num_tuples);
>
> /*
> @@ -1264,7 +1271,7 @@ lazy_cleanup_index(Relation indrel,
>
> ivinfo.index = indrel;
> ivinfo.analyze_only = false;
> - ivinfo.estimated_count = (vacrelstats->scanned_pages < vacrelstats->rel_pages);
> + ivinfo.estimated_count = (vacrelstats->tupcount_pages < vacrelstats->rel_pages);
> ivinfo.message_level = elevel;
> ivinfo.num_heap_tuples = vacrelstats->new_rel_tuples;
> ivinfo.strategy = vac_strategy;

Without having tested it, this looks sane.

Greetings,

Andres Freund