Date: Mon, 30 Mar 2020 11:26:16 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>,
	Masahiko Sawada <masahiko.sawada@2ndquadrant.com>,
	Andres Freund <andres@anarazel.de>,
	Michael Paquier <michael@paquier.xyz>, pgsql-hackers@postgresql.org
Subject: Re: error context for vacuum to include block number
Message-ID: <20200330162616.GP20103@telsasoft.com>
References: <20200326221752.GR17431@telsasoft.com>
 <20200326224951.GA20085@alvherre.pgsql>
 <20200326233321.GA15224@telsasoft.com>
 <CAA4eK1LLSGHgyOdRApxuJdMvXMChoCPHMqWetnUTEK0bPpjM1g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="TD8GDToEDw0WLGOL"
Content-Disposition: inline
In-Reply-To: 
 <CAA4eK1LLSGHgyOdRApxuJdMvXMChoCPHMqWetnUTEK0bPpjM1g@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk


--TD8GDToEDw0WLGOL
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Mon, Mar 30, 2020 at 02:31:53PM +0530, Amit Kapila wrote:
> Now that the main patch is committed, I have reviewed the other two patches.

Thanks for that

On Mon, Mar 30, 2020 at 02:31:53PM +0530, Amit Kapila wrote:
> The v37-0003-Avoid-some-calls-to-RelationGetRelationName.patch looks
> good to me.  I have added the commit message in the patch.

I realized the 0003 patch has an error in lazy_vacuum_index; it should be:

-                                       RelationGetRelationName(indrel),
+                                       vacrelstats->indname,

That was maybe due to originally using a separate errinfo for each phase, with
one "char *relname" and no "char *indrel".

> I don't think the above change is correct.  How will vacrelstats have
> correct values when vacuum_one_index is called via parallel workers
> (via parallel_vacuum_main)?

You're right: parallel main's vacrelstats was added by this patchset and only
the error context fields were initialized.  I fixed it up in the attached by
also setting vacrelstats->new_rel_tuples and old_live_tuples.  It's not clear
if this is worth it just to save an argument to two functions?

-- 
Justin

--TD8GDToEDw0WLGOL
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v39-0001-Avoid-some-calls-to-RelationGetRelationName.patch"

From 85672d7f071c91f3ec9190be7feb293f0e49cf8a Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Wed, 26 Feb 2020 19:22:55 -0600
Subject: [PATCH v39 1/2] Avoid some calls to RelationGetRelationName

---
 src/backend/access/heap/vacuumlazy.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 0d2e724a7d..803e7660f7 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -655,8 +655,8 @@ heap_vacuum_rel(Relation onerel, VacuumParams *params,
 			}
 			appendStringInfo(&buf, msgfmt,
 							 get_database_name(MyDatabaseId),
-							 get_namespace_name(RelationGetNamespace(onerel)),
-							 RelationGetRelationName(onerel),
+							 vacrelstats->relnamespace,
+							 vacrelstats->relname,
 							 vacrelstats->num_index_scans);
 			appendStringInfo(&buf, _("pages: %u removed, %u remain, %u skipped due to pins, %u skipped frozen\n"),
 							 vacrelstats->pages_removed,
@@ -827,7 +827,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
 			if (params->nworkers > 0)
 				ereport(WARNING,
 						(errmsg("disabling parallel option of vacuum on \"%s\" --- cannot vacuum temporary tables in parallel",
-								RelationGetRelationName(onerel))));
+								vacrelstats->relname)));
 		}
 		else
 			lps = begin_parallel_vacuum(RelationGetRelid(onerel), Irel,
@@ -1722,7 +1722,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
 	if (vacuumed_pages)
 		ereport(elevel,
 				(errmsg("\"%s\": removed %.0f row versions in %u pages",
-						RelationGetRelationName(onerel),
+						vacrelstats->relname,
 						tups_vacuumed, vacuumed_pages)));
 
 	/*
@@ -1751,7 +1751,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
 
 	ereport(elevel,
 			(errmsg("\"%s\": found %.0f removable, %.0f nonremovable row versions in %u out of %u pages",
-					RelationGetRelationName(onerel),
+					vacrelstats->relname,
 					tups_vacuumed, num_tuples,
 					vacrelstats->scanned_pages, nblocks),
 			 errdetail_internal("%s", buf.data)));
@@ -1883,7 +1883,7 @@ lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats)
 
 	ereport(elevel,
 			(errmsg("\"%s\": removed %d row versions in %d pages",
-					RelationGetRelationName(onerel),
+					vacrelstats->relname,
 					tupindex, npages),
 			 errdetail_internal("%s", pg_rusage_show(&ru0))));
 
@@ -2431,7 +2431,7 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats,
 
 	ereport(elevel,
 			(errmsg(msg,
-					RelationGetRelationName(indrel),
+					vacrelstats->indname,
 					dead_tuples->num_tuples),
 			 errdetail_internal("%s", pg_rusage_show(&ru0))));
 
@@ -2602,7 +2602,7 @@ lazy_truncate_heap(Relation onerel, LVRelStats *vacrelstats)
 				vacrelstats->lock_waiter_detected = true;
 				ereport(elevel,
 						(errmsg("\"%s\": stopping truncate due to conflicting lock request",
-								RelationGetRelationName(onerel))));
+								vacrelstats->relname)));
 				return;
 			}
 
@@ -2668,7 +2668,7 @@ lazy_truncate_heap(Relation onerel, LVRelStats *vacrelstats)
 
 		ereport(elevel,
 				(errmsg("\"%s\": truncated %u to %u pages",
-						RelationGetRelationName(onerel),
+						vacrelstats->relname,
 						old_rel_pages, new_rel_pages),
 				 errdetail_internal("%s",
 									pg_rusage_show(&ru0))));
@@ -2733,7 +2733,7 @@ count_nondeletable_pages(Relation onerel, LVRelStats *vacrelstats)
 				{
 					ereport(elevel,
 							(errmsg("\"%s\": suspending truncate due to conflicting lock request",
-									RelationGetRelationName(onerel))));
+									vacrelstats->relname)));
 
 					vacrelstats->lock_waiter_detected = true;
 					return blkno;
-- 
2.17.0


--TD8GDToEDw0WLGOL
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="v39-0002-Drop-reltuples.patch"

From e69a3feb5dcf3860a34771c826cd3a1bdfbf9c83 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Wed, 4 Mar 2020 12:28:50 -0600
Subject: [PATCH v39 2/2] Drop reltuples

---
 src/backend/access/heap/vacuumlazy.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 803e7660f7..7f5e177ac4 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -331,10 +331,10 @@ static void lazy_vacuum_all_indexes(Relation onerel, Relation *Irel,
 									LVRelStats *vacrelstats, LVParallelState *lps,
 									int nindexes);
 static void lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats,
-							  LVDeadTuples *dead_tuples, double reltuples, LVRelStats *vacrelstats);
+							  LVDeadTuples *dead_tuples, LVRelStats *vacrelstats);
 static void lazy_cleanup_index(Relation indrel,
 							   IndexBulkDeleteResult **stats,
-							   double reltuples, bool estimated_count, LVRelStats *vacrelstats);
+							   bool estimated_count, LVRelStats *vacrelstats);
 static int	lazy_vacuum_page(Relation onerel, BlockNumber blkno, Buffer buffer,
 							 int tupindex, LVRelStats *vacrelstats, Buffer *vmbuffer);
 static bool should_attempt_truncation(VacuumParams *params,
@@ -1801,7 +1801,7 @@ lazy_vacuum_all_indexes(Relation onerel, Relation *Irel,
 
 		for (idx = 0; idx < nindexes; idx++)
 			lazy_vacuum_index(Irel[idx], &stats[idx], vacrelstats->dead_tuples,
-							  vacrelstats->old_live_tuples, vacrelstats);
+							  vacrelstats);
 	}
 
 	/* Increase and report the number of index scans */
@@ -2301,11 +2301,10 @@ vacuum_one_index(Relation indrel, IndexBulkDeleteResult **stats,
 
 	/* Do vacuum or cleanup of the index */
 	if (lvshared->for_cleanup)
-		lazy_cleanup_index(indrel, stats, lvshared->reltuples,
-						   lvshared->estimated_count, vacrelstats);
+		lazy_cleanup_index(indrel, stats, lvshared->estimated_count, vacrelstats);
 	else
 		lazy_vacuum_index(indrel, stats, dead_tuples,
-						  lvshared->reltuples, vacrelstats);
+						  vacrelstats);
 
 	/*
 	 * Copy the index bulk-deletion result returned from ambulkdelete and
@@ -2379,7 +2378,6 @@ lazy_cleanup_all_indexes(Relation *Irel, IndexBulkDeleteResult **stats,
 	{
 		for (idx = 0; idx < nindexes; idx++)
 			lazy_cleanup_index(Irel[idx], &stats[idx],
-							   vacrelstats->new_rel_tuples,
 							   vacrelstats->tupcount_pages < vacrelstats->rel_pages,
 							   vacrelstats);
 	}
@@ -2396,7 +2394,7 @@ lazy_cleanup_all_indexes(Relation *Irel, IndexBulkDeleteResult **stats,
  */
 static void
 lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats,
-				  LVDeadTuples *dead_tuples, double reltuples, LVRelStats *vacrelstats)
+				  LVDeadTuples *dead_tuples, LVRelStats *vacrelstats)
 {
 	IndexVacuumInfo ivinfo;
 	const char *msg;
@@ -2410,7 +2408,7 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats,
 	ivinfo.report_progress = false;
 	ivinfo.estimated_count = true;
 	ivinfo.message_level = elevel;
-	ivinfo.num_heap_tuples = reltuples;
+	ivinfo.num_heap_tuples = vacrelstats->old_live_tuples;
 	ivinfo.strategy = vac_strategy;
 
 	/* Update error traceback information */
@@ -2451,7 +2449,7 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats,
 static void
 lazy_cleanup_index(Relation indrel,
 				   IndexBulkDeleteResult **stats,
-				   double reltuples, bool estimated_count, LVRelStats *vacrelstats)
+				   bool estimated_count, LVRelStats *vacrelstats)
 {
 	IndexVacuumInfo ivinfo;
 	const char *msg;
@@ -2466,7 +2464,7 @@ lazy_cleanup_index(Relation indrel,
 	ivinfo.estimated_count = estimated_count;
 	ivinfo.message_level = elevel;
 
-	ivinfo.num_heap_tuples = reltuples;
+	ivinfo.num_heap_tuples = vacrelstats->new_rel_tuples;
 	ivinfo.strategy = vac_strategy;
 
 	/* Update error traceback information */
@@ -3490,14 +3488,14 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
 	if (lvshared->maintenance_work_mem_worker > 0)
 		maintenance_work_mem = lvshared->maintenance_work_mem_worker;
 
-	/*
-	 * Initialize vacrelstats for use as error callback arg by parallel
-	 * worker.
-	 */
+	/* Initialize vacrelstats for use by parallel worker. */
 	vacrelstats.relnamespace = get_namespace_name(RelationGetNamespace(onerel));
 	vacrelstats.relname = pstrdup(RelationGetRelationName(onerel));
 	vacrelstats.indname = NULL;
 	vacrelstats.phase = VACUUM_ERRCB_PHASE_UNKNOWN; /* Not yet processing */
+	vacrelstats.old_live_tuples = lvshared->reltuples; /* Used for vacuum phase */
+	vacrelstats.new_rel_tuples = lvshared->reltuples; /* Used for cleanup phase */
+	vacrelstats.tupcount_pages = lvshared->estimated_count; /* Used for cleanup phase */
 
 	/* Setup error traceback support for ereport() */
 	errcallback.callback = vacuum_error_callback;
-- 
2.17.0


--TD8GDToEDw0WLGOL--