Date: Fri, 7 Feb 2020 19:01:07 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Andres Freund <andres@anarazel.de>,
	Michael Paquier <michael@paquier.xyz>,
	Alvaro Herrera <alvherre@2ndquadrant.com>,
	pgsql-hackers@postgresql.org
Subject: Re: error context for vacuum to include block number
Message-ID: <20200208010107.GT403@telsasoft.com>
References: <20200120054159.GT26045@telsasoft.com>
 <20200120191120.s2pyx4ib4yubavxe@alap3.anarazel.de>
 <20200120214929.GV26045@telsasoft.com>
 <20200126202938.dqtavh6vr3vgpw53@alap3.anarazel.de>
 <20200127053813.GS13621@telsasoft.com>
 <CA+fd4k71fHRYJAeGxigGza5U+_HDJcBAbMniiMK2xz5GHhsMmQ@mail.gmail.com>
 <20200127225018.GY13621@telsasoft.com>
 <CA+fd4k48wcdYpr-4UsLzchohOk6pwW3Aijmw_PR4hEXJBcs8Xw@mail.gmail.com>
 <20200202060259.GG13621@telsasoft.com>
 <CA+fd4k7U__9nWB5q0njLc66rBJfekKAUVJXOPJ0HEw-DYocyLQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="byLs0wutDcxFdwtm"
Content-Disposition: inline
In-Reply-To: 
 <CA+fd4k7U__9nWB5q0njLc66rBJfekKAUVJXOPJ0HEw-DYocyLQ@mail.gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


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

On Tue, Feb 04, 2020 at 01:58:20PM +0900, Masahiko Sawada wrote:
> Here is the comment for v16 patch:
> 
> 2.
> I think we can set the error context for heap scan again after
> freespace map vacuum finishing, maybe after reporting the new phase.
> Otherwise the user will get confused if an error occurs during
> freespace map vacuum. And I think the comment is unclear, how about
> "Set the error context fro heap scan again"?

Good point

> 3.
> +           if (cbarg->blkno!=InvalidBlockNumber)
> +               errcontext(_("while scanning block %u of relation \"%s.%s\""),
> +                       cbarg->blkno, cbarg->relnamespace, cbarg->relname);
> 
> We can use BlockNumberIsValid macro instead.

Thanks.  See attached, now squished together patches.

I added functions to initialize the callbacks, so error handling is out of the
way and minimally distract from the rest of vacuum.

--byLs0wutDcxFdwtm
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v17-0001-vacuum-errcontext-to-show-block-being-processed.patch"

From 95265412c56f3b308eed16531d7c83243e278f4f Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Thu, 12 Dec 2019 20:54:37 -0600
Subject: [PATCH v17 1/2] vacuum errcontext to show block being processed

As requested here.
https://www.postgresql.org/message-id/20190807235154.erbmr4o4bo6vgnjv%40alap3.anarazel.de
---
 src/backend/access/heap/vacuumlazy.c | 117 +++++++++++++++++++++++++++++++++++
 1 file changed, 117 insertions(+)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index a23cdef..9358ab4 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -292,6 +292,14 @@ typedef struct LVRelStats
 	bool		lock_waiter_detected;
 } LVRelStats;
 
+typedef struct
+{
+	char 		*relnamespace;
+	char		*relname;
+	char 		*indname; /* undefined while not processing index */
+	BlockNumber blkno;	/* undefined while not processing heap */
+	int			phase;	/* Reusing same enums as for progress reporting */
+} vacuum_error_callback_arg;
 
 /* A few variables that don't seem worth passing around as parameters */
 static int	elevel = -1;
@@ -361,6 +369,7 @@ static void end_parallel_vacuum(Relation *Irel, IndexBulkDeleteResult **stats,
 								LVParallelState *lps, int nindexes);
 static LVSharedIndStats *get_indstats(LVShared *lvshared, int n);
 static bool skip_parallel_vacuum_index(Relation indrel, LVShared *lvshared);
+static void vacuum_error_callback(void *arg);
 
 
 /*
@@ -724,6 +733,8 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
 		PROGRESS_VACUUM_MAX_DEAD_TUPLES
 	};
 	int64		initprog_val[3];
+	ErrorContextCallback errcallback;
+	vacuum_error_callback_arg errcbarg;
 
 	pg_rusage_init(&ru0);
 
@@ -870,6 +881,17 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
 	else
 		skipping_blocks = false;
 
+	/* Setup error traceback support for ereport() */
+	errcbarg.relnamespace = get_namespace_name(RelationGetNamespace(onerel));
+	errcbarg.relname = relname;
+	errcbarg.blkno = InvalidBlockNumber; /* Not known yet */
+	errcbarg.phase = PROGRESS_VACUUM_PHASE_SCAN_HEAP;
+
+	errcallback.callback = vacuum_error_callback;
+	errcallback.arg = (void *) &errcbarg;
+	errcallback.previous = error_context_stack;
+	error_context_stack = &errcallback;
+
 	for (blkno = 0; blkno < nblocks; blkno++)
 	{
 		Buffer		buf;
@@ -891,6 +913,8 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
 #define FORCE_CHECK_PAGE() \
 		(blkno == nblocks - 1 && should_attempt_truncation(params, vacrelstats))
 
+		errcbarg.blkno = blkno;
+
 		pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno);
 
 		if (blkno == next_unskippable_block)
@@ -987,6 +1011,9 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
 				vmbuffer = InvalidBuffer;
 			}
 
+			/* Pop the error context stack */
+			error_context_stack = errcallback.previous;
+
 			/* Work on all the indexes, then the heap */
 			lazy_vacuum_all_indexes(onerel, Irel, indstats,
 									vacrelstats, lps, nindexes);
@@ -1011,6 +1038,9 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
 			/* Report that we are once again scanning the heap */
 			pgstat_progress_update_param(PROGRESS_VACUUM_PHASE,
 										 PROGRESS_VACUUM_PHASE_SCAN_HEAP);
+
+			/* Set the error context while continuing heap scan */
+			error_context_stack = &errcallback;
 		}
 
 		/*
@@ -1597,6 +1627,9 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
 			RecordPageWithFreeSpace(onerel, blkno, freespace);
 	}
 
+	/* Pop the error context stack */
+	error_context_stack = errcallback.previous;
+
 	/* report that everything is scanned and vacuumed */
 	pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno);
 
@@ -1772,11 +1805,26 @@ lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats)
 	int			npages;
 	PGRUsage	ru0;
 	Buffer		vmbuffer = InvalidBuffer;
+	ErrorContextCallback errcallback;
+	vacuum_error_callback_arg errcbarg;
 
 	/* Report that we are now vacuuming the heap */
 	pgstat_progress_update_param(PROGRESS_VACUUM_PHASE,
 								 PROGRESS_VACUUM_PHASE_VACUUM_HEAP);
 
+	/*
+	 * Setup error traceback support for ereport()
+	 */
+	errcbarg.relnamespace = get_namespace_name(RelationGetNamespace(onerel));
+	errcbarg.relname = RelationGetRelationName(onerel);
+	errcbarg.blkno = InvalidBlockNumber; /* Not known yet */
+	errcbarg.phase = PROGRESS_VACUUM_PHASE_VACUUM_HEAP;
+
+	errcallback.callback = vacuum_error_callback;
+	errcallback.arg = (void *) &errcbarg;
+	errcallback.previous = error_context_stack;
+	error_context_stack = &errcallback;
+
 	pg_rusage_init(&ru0);
 	npages = 0;
 
@@ -1791,6 +1839,7 @@ lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats)
 		vacuum_delay_point();
 
 		tblk = ItemPointerGetBlockNumber(&vacrelstats->dead_tuples->itemptrs[tupindex]);
+		errcbarg.blkno = tblk;
 		buf = ReadBufferExtended(onerel, MAIN_FORKNUM, tblk, RBM_NORMAL,
 								 vac_strategy);
 		if (!ConditionalLockBufferForCleanup(buf))
@@ -1811,6 +1860,9 @@ lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats)
 		npages++;
 	}
 
+	/* Pop the error context stack */
+	error_context_stack = errcallback.previous;
+
 	if (BufferIsValid(vmbuffer))
 	{
 		ReleaseBuffer(vmbuffer);
@@ -2318,6 +2370,8 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats,
 	IndexVacuumInfo ivinfo;
 	const char *msg;
 	PGRUsage	ru0;
+	ErrorContextCallback errcallback;
+	vacuum_error_callback_arg errcbarg;
 
 	pg_rusage_init(&ru0);
 
@@ -2329,10 +2383,24 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats,
 	ivinfo.num_heap_tuples = reltuples;
 	ivinfo.strategy = vac_strategy;
 
+	/* Setup error traceback support for ereport() */
+	errcbarg.relnamespace = get_namespace_name(RelationGetNamespace(indrel));
+	errcbarg.indname = RelationGetRelationName(indrel);
+	errcbarg.relname = get_rel_name(indrel->rd_index->indexrelid);
+	errcbarg.phase = PROGRESS_VACUUM_PHASE_VACUUM_INDEX;
+
+	errcallback.callback = vacuum_error_callback;
+	errcallback.arg = (void *) &errcbarg;
+	errcallback.previous = error_context_stack;
+	error_context_stack = &errcallback;
+
 	/* Do bulk deletion */
 	*stats = index_bulk_delete(&ivinfo, *stats,
 							   lazy_tid_reaped, (void *) dead_tuples);
 
+	/* Pop the error context stack */
+	error_context_stack = errcallback.previous;
+
 	if (IsParallelWorker())
 		msg = gettext_noop("scanned index \"%s\" to remove %d row versions by parallel vacuum worker");
 	else
@@ -2359,6 +2427,8 @@ lazy_cleanup_index(Relation indrel,
 	IndexVacuumInfo ivinfo;
 	const char *msg;
 	PGRUsage	ru0;
+	vacuum_error_callback_arg errcbarg;
+	ErrorContextCallback errcallback;
 
 	pg_rusage_init(&ru0);
 
@@ -2371,8 +2441,22 @@ lazy_cleanup_index(Relation indrel,
 	ivinfo.num_heap_tuples = reltuples;
 	ivinfo.strategy = vac_strategy;
 
+	/* Setup error traceback support for ereport() */
+	errcbarg.relnamespace = get_namespace_name(RelationGetNamespace(indrel));
+	errcbarg.indname = RelationGetRelationName(indrel);
+	errcbarg.relname = get_rel_name(indrel->rd_index->indexrelid);
+	errcbarg.phase = PROGRESS_VACUUM_PHASE_INDEX_CLEANUP;
+
+	errcallback.previous = error_context_stack;
+	errcallback.callback = vacuum_error_callback;
+	errcallback.arg = (void *) &errcbarg;
+	error_context_stack = &errcallback;
+
 	*stats = index_vacuum_cleanup(&ivinfo, *stats);
 
+	/* Pop the error context stack */
+	error_context_stack = errcallback.previous;
+
 	if (!(*stats))
 		return;
 
@@ -3375,3 +3459,36 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
 	table_close(onerel, ShareUpdateExclusiveLock);
 	pfree(stats);
 }
+
+/*
+ * Error context callback for errors occurring during vacuum.
+ */
+static void
+vacuum_error_callback(void *arg)
+{
+	vacuum_error_callback_arg *cbarg = arg;
+
+	switch (cbarg->phase) {
+		case PROGRESS_VACUUM_PHASE_SCAN_HEAP:
+			if (BlockNumberIsValid(cbarg->blkno))
+				errcontext(_("while scanning block %u of relation \"%s.%s\""),
+						cbarg->blkno, cbarg->relnamespace, cbarg->relname);
+			break;
+
+		case PROGRESS_VACUUM_PHASE_VACUUM_HEAP:
+			if (BlockNumberIsValid(cbarg->blkno))
+				errcontext(_("while vacuuming block %u of relation \"%s.%s\""),
+						cbarg->blkno, cbarg->relnamespace, cbarg->relname);
+			break;
+
+		case PROGRESS_VACUUM_PHASE_VACUUM_INDEX:
+			errcontext(_("while vacuuming index \"%s.%s\" of relation \"%s\""),
+					cbarg->relnamespace, cbarg->indname, cbarg->relname);
+			break;
+
+		case PROGRESS_VACUUM_PHASE_INDEX_CLEANUP:
+			errcontext(_("while cleaning up index \"%s.%s\" of relation \"%s\""),
+					cbarg->relnamespace, cbarg->indname, cbarg->relname);
+			break;
+	}
+}
-- 
2.7.4


--byLs0wutDcxFdwtm
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v17-0002-Functions-to-initialize-errcontext.patch"

From 410fcd0106c5ba9a4718b79d2a758fa40c246afc Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Thu, 6 Feb 2020 10:27:05 -0600
Subject: [PATCH v17 2/2] Functions to initialize errcontext

---
 src/backend/access/heap/vacuumlazy.c | 68 +++++++++++++++++-------------------
 1 file changed, 32 insertions(+), 36 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 9358ab4..361e595 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -370,6 +370,8 @@ static void end_parallel_vacuum(Relation *Irel, IndexBulkDeleteResult **stats,
 static LVSharedIndStats *get_indstats(LVShared *lvshared, int n);
 static bool skip_parallel_vacuum_index(Relation indrel, LVShared *lvshared);
 static void vacuum_error_callback(void *arg);
+static void init_error_context_heap(ErrorContextCallback *errcallback, vacuum_error_callback_arg *errcbarg, Relation onerel, int phase);
+static void init_error_context_index(ErrorContextCallback *errcallback, vacuum_error_callback_arg *errcbarg, Relation indrel, int phase);
 
 
 /*
@@ -882,15 +884,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
 		skipping_blocks = false;
 
 	/* Setup error traceback support for ereport() */
-	errcbarg.relnamespace = get_namespace_name(RelationGetNamespace(onerel));
-	errcbarg.relname = relname;
-	errcbarg.blkno = InvalidBlockNumber; /* Not known yet */
-	errcbarg.phase = PROGRESS_VACUUM_PHASE_SCAN_HEAP;
-
-	errcallback.callback = vacuum_error_callback;
-	errcallback.arg = (void *) &errcbarg;
-	errcallback.previous = error_context_stack;
-	error_context_stack = &errcallback;
+	init_error_context_heap(&errcallback, &errcbarg, onerel, PROGRESS_VACUUM_PHASE_SCAN_HEAP);
 
 	for (blkno = 0; blkno < nblocks; blkno++)
 	{
@@ -1815,15 +1809,7 @@ lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats)
 	/*
 	 * Setup error traceback support for ereport()
 	 */
-	errcbarg.relnamespace = get_namespace_name(RelationGetNamespace(onerel));
-	errcbarg.relname = RelationGetRelationName(onerel);
-	errcbarg.blkno = InvalidBlockNumber; /* Not known yet */
-	errcbarg.phase = PROGRESS_VACUUM_PHASE_VACUUM_HEAP;
-
-	errcallback.callback = vacuum_error_callback;
-	errcallback.arg = (void *) &errcbarg;
-	errcallback.previous = error_context_stack;
-	error_context_stack = &errcallback;
+	init_error_context_heap(&errcallback, &errcbarg, onerel, PROGRESS_VACUUM_PHASE_VACUUM_HEAP);
 
 	pg_rusage_init(&ru0);
 	npages = 0;
@@ -2384,15 +2370,7 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats,
 	ivinfo.strategy = vac_strategy;
 
 	/* Setup error traceback support for ereport() */
-	errcbarg.relnamespace = get_namespace_name(RelationGetNamespace(indrel));
-	errcbarg.indname = RelationGetRelationName(indrel);
-	errcbarg.relname = get_rel_name(indrel->rd_index->indexrelid);
-	errcbarg.phase = PROGRESS_VACUUM_PHASE_VACUUM_INDEX;
-
-	errcallback.callback = vacuum_error_callback;
-	errcallback.arg = (void *) &errcbarg;
-	errcallback.previous = error_context_stack;
-	error_context_stack = &errcallback;
+	init_error_context_index(&errcallback, &errcbarg, indrel, PROGRESS_VACUUM_PHASE_VACUUM_INDEX);
 
 	/* Do bulk deletion */
 	*stats = index_bulk_delete(&ivinfo, *stats,
@@ -2442,15 +2420,7 @@ lazy_cleanup_index(Relation indrel,
 	ivinfo.strategy = vac_strategy;
 
 	/* Setup error traceback support for ereport() */
-	errcbarg.relnamespace = get_namespace_name(RelationGetNamespace(indrel));
-	errcbarg.indname = RelationGetRelationName(indrel);
-	errcbarg.relname = get_rel_name(indrel->rd_index->indexrelid);
-	errcbarg.phase = PROGRESS_VACUUM_PHASE_INDEX_CLEANUP;
-
-	errcallback.previous = error_context_stack;
-	errcallback.callback = vacuum_error_callback;
-	errcallback.arg = (void *) &errcbarg;
-	error_context_stack = &errcallback;
+	init_error_context_index(&errcallback, &errcbarg, indrel, PROGRESS_VACUUM_PHASE_INDEX_CLEANUP);
 
 	*stats = index_vacuum_cleanup(&ivinfo, *stats);
 
@@ -3492,3 +3462,29 @@ vacuum_error_callback(void *arg)
 			break;
 	}
 }
+
+/* Initialize error context for heap operations */
+static void
+init_error_context_heap(ErrorContextCallback *errcallback, vacuum_error_callback_arg *errcbarg, Relation onerel, int phase)
+{
+	errcbarg->relnamespace = get_namespace_name(RelationGetNamespace(onerel));
+	errcbarg->relname = RelationGetRelationName(onerel);
+	errcbarg->indname = NULL; /* Not used for heap */
+	errcbarg->blkno = InvalidBlockNumber; /* Not known yet */
+	errcbarg->phase = phase;
+
+	errcallback->callback = vacuum_error_callback;
+	errcallback->arg = errcbarg;
+	errcallback->previous = error_context_stack;
+	error_context_stack = errcallback;
+}
+
+/* Initialize error context for index operations */
+static void
+init_error_context_index(ErrorContextCallback *errcallback, vacuum_error_callback_arg *errcbarg, Relation indrel, int phase)
+{
+	/* Piggyback on the heap function but specially set relation names.  Note, blkno is not used for index. */
+	init_error_context_heap(errcallback, errcbarg, indrel, phase);
+	errcbarg->indname = RelationGetRelationName(indrel);
+	errcbarg->relname = get_rel_name(indrel->rd_index->indexrelid);
+}
-- 
2.7.4


--byLs0wutDcxFdwtm--