Fix incorrect index->reltuples after VACUUM

src/backend/access/nbtree/nbtpage.c

@@ -36,6 +36,7 @@
 #include "utils/memdebug.h"
 #include "utils/memutils.h"
 #include "utils/snapmgr.h"
+#include "commands/vacuum.h"
 
 static BTMetaPageData *_bt_getmeta(Relation rel, Buffer metabuf);
 static void _bt_log_reuse_page(Relation rel, BlockNumber blkno,
@@ -186,6 +187,10 @@ _bt_vacuum_needs_cleanup(Relation rel)
 	uint32		btm_version;
 	BlockNumber prev_num_delpages;
 
+	/* Return true directly on QE for stats collection from QD. */
+	if (gp_vacuum_needs_update_stats())
+		return true;
+
 	/*
 	 * Copy details from metapage to local variables quickly.
 	 *

src/backend/access/nbtree/nbtree.c

@@ -956,7 +956,6 @@ btvacuumcleanup(IndexVacuumInfo *info, IndexBulkDeleteResult *stats)
 		 */
 		stats = (IndexBulkDeleteResult *) palloc0(sizeof(IndexBulkDeleteResult));
 		btvacuumscan(info, stats, NULL, NULL, 0);
-		stats->estimated_count = true;
 	}
 
 	/*

src/backend/cdb/dispatcher/cdbdispatchresult.c

@@ -821,6 +821,8 @@ cdbdisp_returnResults(CdbDispatchResults *primaryResults, CdbPgResults *cdb_pgre
 
 	/* tell the caller how many sets we're returning. */
 	cdb_pgresults->numResults = nresults;
+	/* set the number of dispatched QE */
+	cdb_pgresults->numDispatches = primaryResults->resultCount;
 }
 
 /*

src/backend/commands/vacuum.c

@@ -78,7 +78,8 @@
 
 typedef struct VacuumStatsContext
 {
-	List	   *updated_stats;
+	List		*updated_stats;
+	int			nsegs;
 } VacuumStatsContext;
 
 /*
@@ -122,7 +123,7 @@ static void dispatchVacuum(VacuumParams *params, Oid relid,
 static List *vacuum_params_to_options_list(VacuumParams *params);
 static void vacuum_combine_stats(VacuumStatsContext *stats_context,
 								 CdbPgResults *cdb_pgresults);
-static void vac_update_relstats_from_list(List *updated_stats);
+static void vac_update_relstats_from_list(VacuumStatsContext *stats_context);
 
 /*
  * Primary entry point for manual VACUUM and ANALYZE commands
@@ -2687,7 +2688,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 
 		stats_context.updated_stats = NIL;
 		dispatchVacuum(params, relid, &stats_context);
-		vac_update_relstats_from_list(stats_context.updated_stats);
+		vac_update_relstats_from_list(&stats_context);
 
 		/* Also update pg_stat_last_operation */
 		if (IsAutoVacuumWorkerProcess())
@@ -3081,6 +3082,8 @@ vacuum_combine_stats(VacuumStatsContext *stats_context, CdbPgResults *cdb_pgresu
 	if (cdb_pgresults == NULL || cdb_pgresults->numResults <= 0)
 		return;
 
+	stats_context->nsegs = cdb_pgresults->numDispatches;
+
 	/*
 	 * Process the dispatch results from the primary. Note that the QE
 	 * processes also send back the new stats info, such as stats on
@@ -3093,9 +3096,7 @@ vacuum_combine_stats(VacuumStatsContext *stats_context, CdbPgResults *cdb_pgresu
 	 *
 	 */
 	for(result_no = 0; result_no < cdb_pgresults->numResults; result_no++)
-	{
-		VPgClassStats *pgclass_stats = NULL;
-		ListCell *lc = NULL;
+	{		
 		struct pg_result *pgresult = cdb_pgresults->pg_results[result_no];
 
 		if (pgresult->extras == NULL || pgresult->extraType != PGExtraTypeVacuumStats)
@@ -3107,30 +3108,38 @@ vacuum_combine_stats(VacuumStatsContext *stats_context, CdbPgResults *cdb_pgresu
 		 * Process the stats for pg_class. We simply compute the maximum
 		 * number of rel_tuples and rel_pages.
 		 */
-		pgclass_stats = (VPgClassStats *) pgresult->extras;
+		VPgClassStatsCombo *pgclass_stats_combo = (VPgClassStatsCombo *) pgresult->extras;
+		ListCell *lc = NULL;
+
 		foreach (lc, stats_context->updated_stats)
 		{
-			VPgClassStats *tmp_stats = (VPgClassStats *) lfirst(lc);
+			VPgClassStatsCombo *tmp_stats_combo = (VPgClassStatsCombo *) lfirst(lc);
 
-			if (tmp_stats->relid == pgclass_stats->relid)
+			if (tmp_stats_combo->relid == pgclass_stats_combo->relid)
 			{
-				tmp_stats->rel_pages += pgclass_stats->rel_pages;
-				tmp_stats->rel_tuples += pgclass_stats->rel_tuples;
-				tmp_stats->relallvisible += pgclass_stats->relallvisible;
+				tmp_stats_combo->rel_pages += pgclass_stats_combo->rel_pages;
+				tmp_stats_combo->rel_tuples += pgclass_stats_combo->rel_tuples;
+				tmp_stats_combo->relallvisible += pgclass_stats_combo->relallvisible;
+				/* 
+				 * Accumulate the number of QEs, assuming sending only once
+				 * per QE for each relid in the VACUUM scenario.
+				 */
+				tmp_stats_combo->count++;
 				break;
 			}
 		}
 
-		if (lc == NULL)
+		if (lc == NULL) /* get the first stats result of the current relid */
 		{
 			Assert(pgresult->extraslen == sizeof(VPgClassStats));
 
 			old_context = MemoryContextSwitchTo(vac_context);
-			pgclass_stats = palloc(sizeof(VPgClassStats));
-			memcpy(pgclass_stats, pgresult->extras, pgresult->extraslen);
+			pgclass_stats_combo = palloc(sizeof(VPgClassStatsCombo));
+			memcpy(pgclass_stats_combo, pgresult->extras, pgresult->extraslen);
+			pgclass_stats_combo->count = 1;
 
 			stats_context->updated_stats =
-				lappend(stats_context->updated_stats, pgclass_stats);
+				lappend(stats_context->updated_stats, pgclass_stats_combo);
 			MemoryContextSwitchTo(old_context);
 		}
 	}
@@ -3140,8 +3149,9 @@ vacuum_combine_stats(VacuumStatsContext *stats_context, CdbPgResults *cdb_pgresu
  * Update relpages/reltuples of all the relations in the list.
  */
 static void
-vac_update_relstats_from_list(List *updated_stats)
+vac_update_relstats_from_list(VacuumStatsContext *stats_context)
 {
+	List *updated_stats = stats_context->updated_stats;
 	ListCell *lc;
 
 	/*
@@ -3152,14 +3162,16 @@ vac_update_relstats_from_list(List *updated_stats)
 
 	foreach (lc, updated_stats)
 	{
-		VPgClassStats *stats = (VPgClassStats *) lfirst(lc);
+		VPgClassStatsCombo *stats = (VPgClassStatsCombo *) lfirst(lc);
 		Relation	rel;
 		int16		ao_segfile_count = 0;
 
 		rel = relation_open(stats->relid, AccessShareLock);
 
 		if (GpPolicyIsReplicated(rel->rd_cdbpolicy))
 		{
+			Assert(stats->count == rel->rd_cdbpolicy->numsegments);
+
 			stats->rel_pages = stats->rel_pages / rel->rd_cdbpolicy->numsegments;
 			stats->rel_tuples = stats->rel_tuples / rel->rd_cdbpolicy->numsegments;
 			stats->relallvisible = stats->relallvisible / rel->rd_cdbpolicy->numsegments;
@@ -3201,17 +3213,55 @@ vac_update_relstats_from_list(List *updated_stats)
 		}
 
 		/*
-		 * Pass 'false' for isvacuum, so that the stats are
-		 * actually updated.
+		 * Update QD stats only when receiving all dispatched QEs' stats, to
+		 * avoid being overwritten by a partial accumulated value (i.e., index->reltuples)
+		 * in case when not receiving all QEs' stats.
 		 */
-		vac_update_relstats(rel,
-							stats->rel_pages, stats->rel_tuples,
-							stats->relallvisible,
-							rel->rd_rel->relhasindex,
-							InvalidTransactionId,
-							InvalidMultiXactId,
-							false,
-							false /* isvacuum */);
+		if (stats_context->nsegs > 0 && stats->count == stats_context->nsegs)
+		{
+			/*
+			 * Pass 'false' for isvacuum, so that the stats are
+			 * actually updated.
+			 */
+			vac_update_relstats(rel,
+								stats->rel_pages, stats->rel_tuples,
+								stats->relallvisible,
+								rel->rd_rel->relhasindex,
+								InvalidTransactionId,
+								InvalidMultiXactId,
+								false,
+								false /* isvacuum */);
+		}
+		else
+		{
+			/*
+			 * We do have chance to enter this branch in the case when in compact phase.
+			 * For example, in compact phase, some QEs may need to drop dead segfiles,
+			 * while others may not. Only the QEs which dropping dead segfiles could go to
+			 * vacuum indexes path then update and send the statistics to QD, QD just
+			 * collected part of QEs' stats hence should not be as the final result to
+			 * overwrite QD's stats. 
+			 * 
+			 * One may think why not having the stats update only happens in the final
+			 * phase (POST_CLEANUP_PHASE), yes that's an alternative to get a final stats
+			 * accurately for QD. 
+			 * 
+			 * Given the AO/CO VACUUM is a multi-phases process which may have an interval
+			 * between each phase. In real circumstance, concurrent VACUUM is mostly a heavy
+			 * job and this interval could get longer than normal cases, hence it seems
+			 * better to collect and update QD's stats timely. So current strategy is, QD always 
+			 * collect QE's stats across phases, once we collected the expected number (means
+			 * same as dispatched QE number) of QE's stats, we update QD's stats subsequently,
+			 * instead of updating at the final phase.
+			 * 
+			 * Set the logging level to LOG as skipping sending stats here is not considered as
+			 * a real issue, displaying it in log may be helpful to hint.
+			 */
+			elog(LOG, "Vacuum update stats oid=%u pages=%d tuples=%f was skipped because "
+				 "collected segment number %d didn't match the expected %d.", stats->relid,
+				 stats->rel_pages, stats->rel_tuples, stats->count, stats_context->nsegs);
+		}
+
 		relation_close(rel, AccessShareLock);
 	}
 }
@@ -3277,3 +3327,18 @@ vacuumStatement_IsTemporary(Relation onerel)
 		bTemp = isAnyTempNamespace(RelationGetNamespace(onerel));
 	return bTemp;
 }
+
+/*
+ * GPDB: Check whether needs to update or send stats from QE to QD.
+ * This is GPDB specific check in vacuum-index scenario for collecting
+ * QEs' stats (such as index->relpages and index->reltuples) on QD.
+ * GPDB needs accumulating all QEs' stats for updating corresponding 
+ * statistics into QD's pg_class correctly. So if current instance is
+ * acting as QE, it should scan and send its current stats to QD instead
+ * of skipping them for cost saving.
+ */
+bool
+gp_vacuum_needs_update_stats(void)
+{
+	return (Gp_role == GP_ROLE_EXECUTE);
+}

src/backend/commands/vacuum_ao.c

@@ -145,7 +145,7 @@
 
 
 static void vacuum_appendonly_index(Relation indexRelation,
-									double rel_tuple_count,
+									Relation aoRelation,
 									Bitmapset *dead_segs,
 									int elevel,
 									BufferAccessStrategy bstrategy);
@@ -491,15 +491,18 @@ vacuum_appendonly_indexes(Relation aoRelation, int options, Bitmapset *dead_segs
 		{
 			for (i = 0; i < nindexes; i++)
 			{
-				scan_index(Irel[i], aoRelation , elevel, bstrategy);
+				scan_index(Irel[i],
+						   aoRelation,
+						   elevel,
+						   bstrategy);
 			}
 		}
 		else
 		{
 			for (i = 0; i < nindexes; i++)
 			{
 				vacuum_appendonly_index(Irel[i],
-										aoRelation->rd_rel->reltuples,
+										aoRelation,
 										dead_segs,
 										elevel,
 										bstrategy);
@@ -516,11 +519,10 @@ vacuum_appendonly_indexes(Relation aoRelation, int options, Bitmapset *dead_segs
  *
  * This is called after an append-only segment file compaction to move
  * all tuples from the compacted segment files.
- * The segmentFileList is an
  */
 static void
 vacuum_appendonly_index(Relation indexRelation,
-						double rel_tuple_count,
+						Relation aoRelation,
 						Bitmapset *dead_segs,
 						int elevel,
 						BufferAccessStrategy bstrategy)
@@ -534,8 +536,14 @@ vacuum_appendonly_index(Relation indexRelation,
 	pg_rusage_init(&ru0);
 
 	ivinfo.index = indexRelation;
+	ivinfo.analyze_only = false;
 	ivinfo.message_level = elevel;
-	ivinfo.num_heap_tuples = rel_tuple_count;
+	/* 
+	 * We can only provide the AO rel's reltuples as an estimate
+	 * (similar to heapam. See: lazy_vacuum_index()).
+	 */
+	ivinfo.num_heap_tuples = aoRelation->rd_rel->reltuples;
+	ivinfo.estimated_count = true;
 	ivinfo.strategy = bstrategy;
 
 	/* Do bulk deletion */
@@ -680,9 +688,7 @@ vacuum_appendonly_fill_stats(Relation aorel, Snapshot snapshot, int elevel,
  * We use this when we have no deletions to do.
  */
 void
-scan_index(Relation indrel,
-		   Relation aorel,
-		   int elevel, BufferAccessStrategy vac_strategy)
+scan_index(Relation indrel, Relation aorel, int elevel, BufferAccessStrategy vac_strategy)
 {
 	IndexBulkDeleteResult *stats;
 	IndexVacuumInfo ivinfo;
@@ -692,9 +698,13 @@ scan_index(Relation indrel,
 
 	ivinfo.index = indrel;
 	ivinfo.analyze_only = false;
-	ivinfo.estimated_count = false;
 	ivinfo.message_level = elevel;
+	/* 
+	 * We can only provide the AO rel's reltuples as an estimate
+	 * (similar to heapam. See: lazy_vacuum_index()).
+	 */
 	ivinfo.num_heap_tuples = aorel->rd_rel->reltuples;
+	ivinfo.estimated_count = true;
 	ivinfo.strategy = vac_strategy;
 
 

src/include/cdb/cdbdispatchresult.h

@@ -29,6 +29,7 @@ typedef struct CdbPgResults
 {
 	struct pg_result **pg_results;
 	int numResults;
+	int numDispatches; /* the number of dispatched QE, from CdbDispatchResults.resultCount */
 }CdbPgResults;
 
 /*

src/include/commands/vacuum.h

@@ -258,6 +258,16 @@ typedef struct VPgClassStats
 	BlockNumber relallvisible;
 } VPgClassStats;
 
+typedef struct VPgClassStatsCombo
+{
+	Oid			relid;
+	BlockNumber rel_pages;
+	double		rel_tuples;
+	BlockNumber relallvisible;
+
+	int			count; /* expect to equal to the number of dispatched segments */
+} VPgClassStatsCombo;
+
 /*
  * Parameters customizing behavior of VACUUM and ANALYZE.
  *
@@ -386,6 +396,7 @@ extern void analyze_rel(Oid relid, RangeVar *relation,
 extern void lazy_vacuum_rel_heap(Relation onerel,
 							VacuumParams *params, BufferAccessStrategy bstrategy);
 extern void scan_index(Relation indrel, Relation aorel, int elevel, BufferAccessStrategy bstrategy);
+
 /* in commands/vacuum_ao.c */
 extern void ao_vacuum_rel(Relation rel, VacuumParams *params, BufferAccessStrategy bstrategy);
 
@@ -407,4 +418,6 @@ extern int acquire_inherited_sample_rows(Relation onerel, int elevel,
 extern Datum gp_acquire_sample_rows(PG_FUNCTION_ARGS);
 extern Oid gp_acquire_sample_rows_col_type(Oid typid);
 
+extern bool gp_vacuum_needs_update_stats(void);
+
 #endif							/* VACUUM_H */