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@anarazel.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