Fix a missed case in code for "moving average" estimate of reltuples.

tglsfdc · tglsfdc · commit 5bba65de9449 · 2011-08-30T14:51:38.000-04:00
It is possible for VACUUM to scan no pages at all, if the visibility map shows that all pages are all-visible. In this situation VACUUM has no new information to report about the relation's tuple density, so it wasn't changing pg_class.reltuples ... but it updated pg_class.relpages anyway. That's wrong in general, since there is no evidence to justify changing the density ratio reltuples/relpages, but it's particularly bad if the previous state was relpages=reltuples=0, which means "unknown tuple density". We just replaced "unknown" with "zero". ANALYZE would eventually recover from this, but it could take a lot of repetitions of ANALYZE to do so if the relation size is much larger than the maximum number of pages ANALYZE will scan, because of the moving-average behavior introduced by commit b4b6923. The only known situation where we could have relpages=reltuples=0 and yet the visibility map asserts everything's visible is immediately following a pg_upgrade. It might be advisable for pg_upgrade to try to preserve the relpages/reltuples statistics; but in any case this code is wrong on its own terms, so fix it. Per report from Sergey Koposov. Back-patch to 8.4, where the visibility map was introduced, same as the previous change.
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
@@ -493,7 +493,9 @@ vac_estimate_reltuples(Relation relation, bool is_analyze,
 
 	/*
 	 * If scanned_pages is zero but total_pages isn't, keep the existing value
-	 * of reltuples.
+	 * of reltuples.  (Note: callers should avoid updating the pg_class
+	 * statistics in this situation, since no new information has been
+	 * provided.)
 	 */
 	if (scanned_pages == 0)
 		return old_rel_tuples;
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
@@ -84,6 +84,7 @@ typedef struct LVRelStats
 	/* hasindex = true means two-pass strategy; false means one-pass */
 	bool		hasindex;
 	/* Overall statistics about rel */
+	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 */
@@ -154,6 +155,9 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
 	TimestampTz starttime = 0;
 	bool		scan_all;
 	TransactionId freezeTableLimit;
+	BlockNumber new_rel_pages;
+	double		new_rel_tuples;
+	TransactionId new_frozen_xid;
 
 	/* measure elapsed time iff autovacuum logging requires it */
 	if (IsAutoVacuumWorkerProcess() && Log_autovacuum_min_duration >= 0)
@@ -178,6 +182,7 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
 
 	vacrelstats = (LVRelStats *) palloc0(sizeof(LVRelStats));
 
+	vacrelstats->old_rel_pages = onerel->rd_rel->relpages;
 	vacrelstats->old_rel_tuples = onerel->rd_rel->reltuples;
 	vacrelstats->num_index_scans = 0;
 
@@ -207,20 +212,39 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
 	FreeSpaceMapVacuum(onerel);
 
 	/*
-	 * Update statistics in pg_class.  But don't change relfrozenxid if we
-	 * skipped any pages.
+	 * Update statistics in pg_class.
+	 *
+	 * A corner case here is that if we scanned no pages at all because every
+	 * page is all-visible, we should not update relpages/reltuples, because
+	 * we have no new information to contribute.  In particular this keeps
+	 * us from replacing relpages=reltuples=0 (which means "unknown tuple
+	 * density") with nonzero relpages and reltuples=0 (which means "zero
+	 * tuple density") unless there's some actual evidence for the latter.
+	 *
+	 * Also, don't change relfrozenxid if we skipped any pages, since then
+	 * we don't know for certain that all tuples have a newer xmin.
 	 */
+	new_rel_pages = vacrelstats->rel_pages;
+	new_rel_tuples = vacrelstats->new_rel_tuples;
+	if (vacrelstats->scanned_pages == 0 && new_rel_pages > 0)
+	{
+		new_rel_pages = vacrelstats->old_rel_pages;
+		new_rel_tuples = vacrelstats->old_rel_tuples;
+	}
+
+	new_frozen_xid = FreezeLimit;
+	if (vacrelstats->scanned_pages < vacrelstats->rel_pages)
+		new_frozen_xid = InvalidTransactionId;
+
 	vac_update_relstats(onerel,
-						vacrelstats->rel_pages, vacrelstats->new_rel_tuples,
+						new_rel_pages, new_rel_tuples,
 						vacrelstats->hasindex,
-					  (vacrelstats->scanned_pages < vacrelstats->rel_pages) ?
-						InvalidTransactionId :
-						FreezeLimit);
+						new_frozen_xid);
 
 	/* report results to the stats collector, too */
 	pgstat_report_vacuum(RelationGetRelid(onerel),
 						 onerel->rd_rel->relisshared,
-						 vacrelstats->new_rel_tuples);
+						 new_rel_tuples);
 
 	/* and log the action if appropriate */
 	if (IsAutoVacuumWorkerProcess() && Log_autovacuum_min_duration >= 0)
@@ -240,7 +264,7 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
 							vacrelstats->pages_removed,
 							vacrelstats->rel_pages,
 							vacrelstats->tuples_deleted,
-							vacrelstats->new_rel_tuples,
+							new_rel_tuples,
 							pg_rusage_show(&ru0))));
 	}
 }
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
@@ -1415,8 +1415,8 @@ formrdesc(const char *relationName, Oid relationReltype,
 	/* formrdesc is used only for permanent relations */
 	relation->rd_rel->relpersistence = RELPERSISTENCE_PERMANENT;
 
-	relation->rd_rel->relpages = 1;
-	relation->rd_rel->reltuples = 1;
+	relation->rd_rel->relpages = 0;
+	relation->rd_rel->reltuples = 0;
 	relation->rd_rel->relkind = RELKIND_RELATION;
 	relation->rd_rel->relhasoids = hasoids;
 	relation->rd_rel->relnatts = (int16) natts;
