0001-VACUUM-ANALYZE-Always-set-pg_class.reltuples.patch

application/x-patch

Filename: 0001-VACUUM-ANALYZE-Always-set-pg_class.reltuples.patch
Type: application/x-patch
Part: 0
Message: Re: Removing vacuum_cleanup_index_scale_factor

Patch

Format: format-patch
Series: patch 0001
Subject: VACUUM ANALYZE: Always set pg_class.reltuples.
File+
src/backend/access/heap/vacuumlazy.c 2 1
src/backend/commands/analyze.c 29 20
From 29079ec92eed077c8d4d65e4c3db16ce41578657 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <pg@bowt.ie>
Date: Mon, 1 Mar 2021 14:40:57 -0800
Subject: [PATCH 1/2] VACUUM ANALYZE: Always set pg_class.reltuples.

Stop assuming that pg_class must have been updated with accurate
statistics within VACUUM ANALYZE -- update pg_class for indexes at the
same time as the table relation in all cases.

VACUUM sometimes gives less accurate statistics than ANALYZE, or no
statistics at all.  It seems advisable to effectively assume that that
always happens.  The worst that can happen is that we'll inaccurately
set pg_class.reltuples for indexes whose heap relation would already
have had an inaccurate pg_class.reltuples anyway.

Author: Peter Geoghegan <pg@bowt.ie>
Discussion: https://postgr.es/m/CAH2-WzknxdComjhqo4SUxVFk_Q1171GJO2ZgHZ1Y6pion6u8rA@mail.gmail.com
---
 src/backend/access/heap/vacuumlazy.c |  3 +-
 src/backend/commands/analyze.c       | 49 ++++++++++++++++------------
 2 files changed, 31 insertions(+), 21 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index d8f847b0e6..22ca7f5c54 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1737,7 +1737,8 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
 		end_parallel_vacuum(indstats, lps, nindexes);
 
 	/* Update index statistics */
-	update_index_statistics(Irel, indstats, nindexes);
+	if (vacrelstats->useindex)
+		update_index_statistics(Irel, indstats, nindexes);
 
 	/* If no indexes, make log report that lazy_vacuum_heap would've made */
 	if (vacuumed_pages)
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 7295cf0215..7ae1cd426c 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -600,8 +600,22 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
 								 PROGRESS_ANALYZE_PHASE_FINALIZE_ANALYZE);
 
 	/*
-	 * Update pages/tuples stats in pg_class ... but not if we're doing
-	 * inherited stats.
+	 * Update pages/tuples stats in pg_class, and report ANALYZE to the stats
+	 * collector ... but not if we're doing inherited stats.
+	 *
+	 * We make a uniform assumption that VACUUM hasn't accurately set a
+	 * pg_class.reltuples value, regardless of whether this is a VACUUM
+	 * ANALYZE.  This is possible with a "VACUUM (ANALYZE, INDEX_CLEANUP OFF)"
+	 * command.  It's also possible with index AMs that opt to return NULL
+	 * from their amvacuumcleanup() routine (which is permitted by the index
+	 * AM interface in cases where the index doesn't have any dead tuples).
+	 * Besides, it seems like a good idea to only update pg_class.reltuples in
+	 * the heap relation when we'll do the same for its indexes.
+	 *
+	 * We don't report to the stats collector here because the stats collector
+	 * only tracks per-table stats.  Reset the changes_since_analyze counter
+	 * only if we analyzed all columns; otherwise, there is still work for
+	 * auto-analyze to do.
 	 */
 	if (!inh)
 	{
@@ -609,6 +623,7 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
 
 		visibilitymap_count(onerel, &relallvisible, NULL);
 
+		/* Update pg_class for table relation */
 		vac_update_relstats(onerel,
 							relpages,
 							totalrows,
@@ -617,15 +632,8 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
 							InvalidTransactionId,
 							InvalidMultiXactId,
 							in_outer_xact);
-	}
 
-	/*
-	 * Same for indexes. Vacuum always scans all indexes, so if we're part of
-	 * VACUUM ANALYZE, don't overwrite the accurate count already inserted by
-	 * VACUUM.
-	 */
-	if (!inh && !(params->options & VACOPT_VACUUM))
-	{
+		/* Same for indexes */
 		for (ind = 0; ind < nindexes; ind++)
 		{
 			AnlIndexData *thisdata = &indexdata[ind];
@@ -641,20 +649,21 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
 								InvalidMultiXactId,
 								in_outer_xact);
 		}
+
+		/* Now report ANALYZE to the stats collector */
+		pgstat_report_analyze(onerel, totalrows, totaldeadrows,
+							  (va_cols == NIL));
 	}
 
 	/*
-	 * Report ANALYZE to the stats collector, too.  However, if doing
-	 * inherited stats we shouldn't report, because the stats collector only
-	 * tracks per-table stats.  Reset the changes_since_analyze counter only
-	 * if we analyzed all columns; otherwise, there is still work for
-	 * auto-analyze to do.
+	 * If this isn't part of VACUUM ANALYZE, let index AMs do cleanup.
+	 *
+	 * Note that most index AMs perform a no-op as a matter of policy for
+	 * amvacuumcleanup() when called in ANALYZE-only mode.  In practice only
+	 * ginvacuumcleanup() currently does real work when called through here
+	 * (and even GIN makes it a no-op unless we're running is an autovacuum
+	 * worker).
 	 */
-	if (!inh)
-		pgstat_report_analyze(onerel, totalrows, totaldeadrows,
-							  (va_cols == NIL));
-
-	/* If this isn't part of VACUUM ANALYZE, let index AMs do cleanup */
 	if (!(params->options & VACOPT_VACUUM))
 	{
 		for (ind = 0; ind < nindexes; ind++)
-- 
2.27.0