On Mon, Jun 22, 2020 at 01:57:12PM -0700, Andres Freund wrote:
> On 2020-06-22 15:43:11 -0500, Justin Pryzby wrote:
> > On Mon, Jun 22, 2020 at 01:09:39PM -0700, Andres Freund wrote:
> > > I'm also uncomfortable with the approach of just copying all of
> > > LVRelStats in several places:
> > > 
> > > >  /*
> > > > @@ -1580,9 +1648,15 @@ lazy_vacuum_page(Relation onerel, BlockNumber 
> > > > blkno, Buffer buffer,
> > > >         int                     uncnt = 0;
> > > >         TransactionId visibility_cutoff_xid;
> > > >         bool            all_frozen;
> > > > +       LVRelStats      olderrinfo;
> > 
> > I guess the alternative is to write like
> > 
> > LVRelStats  olderrinfo = {
> >     .phase = vacrelstats.phase,
> >     .blkno = vacrelstats.blkno,
> >     .indname = vacrelstats.indname,
> > };
> 
> No, I don't think that's a solution. I think it's wrong to have
> something like olderrinfo in the first place. Using a struct with ~25
> members to store the current state of three variables just doesn't make
> sense.  Why isn't this just a LVSavedPosition struct or something like
> that?

I'd used LVRelStats on your suggestion:
https://www.postgresql.org/message-id/20191211165425.4ewww2s5k5cafi4l%40alap3.anarazel.de
https://www.postgresql.org/message-id/20200120191305.sxi44cedhtxwr3ag%40alap3.anarazel.de

I understood the goal to be avoiding the need to add a new struct, when most
functions are already passed LVRelStats *vacrelstats.

But maybe I misunderstood.  (Also, back in January, the callback was only used
for scan-heap phase, so it's increased in scope several times).

Anyway, I put together some patches for discussion purposes.

-- 
Justin
>From 82daf888eb6834400be482affe38c1f23851826b Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Mon, 22 Jun 2020 18:22:58 -0500
Subject: [PATCH 1/4] Rename from "errcbarg"..

..the name of which Alvaro didn't like.  I renamed this, but it was
accidentally re-introduced while trading patches with Amit.

We should *also* rename VACUUM_ERRCB* to ERRINFO, but I didn't do that here.
---
 src/backend/access/heap/vacuumlazy.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 3bef0e124b..a6a5d906c0 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -2459,7 +2459,7 @@ lazy_cleanup_index(Relation indrel,
 	IndexVacuumInfo ivinfo;
 	const char *msg;
 	PGRUsage	ru0;
-	LVRelStats	olderrcbarg;
+	LVRelStats	olderrinfo;
 
 	pg_rusage_init(&ru0);
 
@@ -2473,7 +2473,7 @@ lazy_cleanup_index(Relation indrel,
 	ivinfo.strategy = vac_strategy;
 
 	/* Update error traceback information */
-	olderrcbarg = *vacrelstats;
+	olderrinfo = *vacrelstats;
 	update_vacuum_error_info(vacrelstats,
 							 VACUUM_ERRCB_PHASE_INDEX_CLEANUP,
 							 InvalidBlockNumber,
@@ -2483,9 +2483,9 @@ lazy_cleanup_index(Relation indrel,
 
 	/* Revert back to the old phase information for error traceback */
 	update_vacuum_error_info(vacrelstats,
-							 olderrcbarg.phase,
-							 olderrcbarg.blkno,
-							 olderrcbarg.indname);
+							 olderrinfo.phase,
+							 olderrinfo.blkno,
+							 olderrinfo.indname);
 	if (!(*stats))
 		return;
 
-- 
2.17.0

>From 308faea3626e8c270eb75659bec3f7e15125a73a Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Mon, 22 Jun 2020 18:51:35 -0500
Subject: [PATCH 2/4] Add assert and document why indname is safe

---
 src/backend/access/heap/vacuumlazy.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index a6a5d906c0..40173480b8 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -3606,10 +3606,23 @@ update_vacuum_error_info(LVRelStats *errinfo, int phase, BlockNumber blkno,
 	errinfo->blkno = blkno;
 	errinfo->phase = phase;
 
-	/* Free index name from any previous phase */
 	if (errinfo->indname)
+	{
+		/*
+		 * indname is only ever saved during lazy_vacuum_index(), which
+		 * during which the phase information is not not further
+		 * manipulated, until it's restored before returning from
+		 * lazy_vacuum_index().
+		 */
+		Assert(indname == NULL);
+
 		pfree(errinfo->indname);
+		errinfo->indname = NULL;
+	}
 
-	/* For index phases, save the name of the current index for the callback */
-	errinfo->indname = indname ? pstrdup(indname) : NULL;
+	if (indname)
+	{
+		/* For index phases, save the name of the current index for the callback */
+		errinfo->indname = pstrdup(indname);
+	}
 }
-- 
2.17.0

>From 87ff6cc66510233b1b86a250e8ac48e52b7d54c2 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Mon, 22 Jun 2020 16:36:14 -0500
Subject: [PATCH 3/4] Move error information into a separate struct..

..to avoid copying large LVRelStats struct.

Andres things this is a problem but Tom thinks it's a micro-optimization.

See: b61d161c146328ae6ba9ed937862d66e5c8b035a

This also moves relname and relnamespace (see ef75140fe).

Discussion: https://www.postgresql.org/message-id/20200622200939.jkuniq3vtiumeszj%40alap3.anarazel.de
---
 src/backend/access/heap/vacuumlazy.c | 116 ++++++++++++++-------------
 1 file changed, 62 insertions(+), 54 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 40173480b8..6bd2867660 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -288,10 +288,17 @@ typedef struct LVParallelState
 	int			nindexes_parallel_condcleanup;
 } LVParallelState;
 
-typedef struct LVRelStats
+typedef struct
 {
 	char	   *relnamespace;
 	char	   *relname;
+	char	   *indname;
+	BlockNumber blkno;			/* used only for heap operations */
+	VacErrPhase phase;
+} LVSavedPosition;
+
+typedef struct LVRelStats
+{
 	/* useindex = true means two-pass strategy; false means one-pass */
 	bool		useindex;
 	/* Overall statistics about rel */
@@ -313,10 +320,7 @@ typedef struct LVRelStats
 	TransactionId latestRemovedXid;
 	bool		lock_waiter_detected;
 
-	/* Used for error callback */
-	char	   *indname;
-	BlockNumber blkno;			/* used only for heap operations */
-	VacErrPhase phase;
+	LVSavedPosition errinfo; /* Used for error callback */
 } LVRelStats;
 
 /* A few variables that don't seem worth passing around as parameters */
@@ -388,7 +392,7 @@ 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 update_vacuum_error_info(LVRelStats *errinfo, int phase,
+static void update_vacuum_error_info(LVSavedPosition *errinfo, int phase,
 									 BlockNumber blkno, char *indname);
 
 
@@ -475,10 +479,10 @@ heap_vacuum_rel(Relation onerel, VacuumParams *params,
 
 	vacrelstats = (LVRelStats *) palloc0(sizeof(LVRelStats));
 
-	vacrelstats->relnamespace = get_namespace_name(RelationGetNamespace(onerel));
-	vacrelstats->relname = pstrdup(RelationGetRelationName(onerel));
-	vacrelstats->indname = NULL;
-	vacrelstats->phase = VACUUM_ERRCB_PHASE_UNKNOWN;
+	vacrelstats->errinfo.relnamespace = get_namespace_name(RelationGetNamespace(onerel));
+	vacrelstats->errinfo.relname = pstrdup(RelationGetRelationName(onerel));
+	vacrelstats->errinfo.indname = NULL;
+	vacrelstats->errinfo.phase = VACUUM_ERRCB_PHASE_UNKNOWN;
 	vacrelstats->old_rel_pages = onerel->rd_rel->relpages;
 	vacrelstats->old_live_tuples = onerel->rd_rel->reltuples;
 	vacrelstats->num_index_scans = 0;
@@ -502,7 +506,7 @@ heap_vacuum_rel(Relation onerel, VacuumParams *params,
 	 * information is restored at the end of those phases.
 	 */
 	errcallback.callback = vacuum_error_callback;
-	errcallback.arg = vacrelstats;
+	errcallback.arg = &vacrelstats->errinfo;
 	errcallback.previous = error_context_stack;
 	error_context_stack = &errcallback;
 
@@ -538,7 +542,8 @@ heap_vacuum_rel(Relation onerel, VacuumParams *params,
 		 * which we add context information to errors, so we don't need to
 		 * revert to the previous phase.
 		 */
-		update_vacuum_error_info(vacrelstats, VACUUM_ERRCB_PHASE_TRUNCATE,
+		update_vacuum_error_info(&vacrelstats->errinfo,
+								 VACUUM_ERRCB_PHASE_TRUNCATE,
 								 vacrelstats->nonempty_pages, NULL);
 		lazy_truncate_heap(onerel, vacrelstats);
 	}
@@ -650,8 +655,8 @@ heap_vacuum_rel(Relation onerel, VacuumParams *params,
 			}
 			appendStringInfo(&buf, msgfmt,
 							 get_database_name(MyDatabaseId),
-							 vacrelstats->relnamespace,
-							 vacrelstats->relname,
+							 vacrelstats->errinfo.relnamespace,
+							 vacrelstats->errinfo.relname,
 							 vacrelstats->num_index_scans);
 			appendStringInfo(&buf, _("pages: %u removed, %u remain, %u skipped due to pins, %u skipped frozen\n"),
 							 vacrelstats->pages_removed,
@@ -786,13 +791,13 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
 	if (aggressive)
 		ereport(elevel,
 				(errmsg("aggressively vacuuming \"%s.%s\"",
-						vacrelstats->relnamespace,
-						vacrelstats->relname)));
+						vacrelstats->errinfo.relnamespace,
+						vacrelstats->errinfo.relname)));
 	else
 		ereport(elevel,
 				(errmsg("vacuuming \"%s.%s\"",
-						vacrelstats->relnamespace,
-						vacrelstats->relname)));
+						vacrelstats->errinfo.relnamespace,
+						vacrelstats->errinfo.relname)));
 
 	empty_pages = vacuumed_pages = 0;
 	next_fsm_block_to_vacuum = (BlockNumber) 0;
@@ -828,7 +833,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",
-								vacrelstats->relname)));
+								vacrelstats->errinfo.relname)));
 		}
 		else
 			lps = begin_parallel_vacuum(RelationGetRelid(onerel), Irel,
@@ -948,7 +953,8 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
 
 		pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno);
 
-		update_vacuum_error_info(vacrelstats, VACUUM_ERRCB_PHASE_SCAN_HEAP,
+		update_vacuum_error_info(&vacrelstats->errinfo,
+								 VACUUM_ERRCB_PHASE_SCAN_HEAP,
 								 blkno, NULL);
 
 		if (blkno == next_unskippable_block)
@@ -1582,7 +1588,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
 				 && VM_ALL_VISIBLE(onerel, blkno, &vmbuffer))
 		{
 			elog(WARNING, "page is not marked all-visible but visibility map bit is set in relation \"%s\" page %u",
-				 vacrelstats->relname, blkno);
+				 vacrelstats->errinfo.relname, blkno);
 			visibilitymap_clear(onerel, blkno, vmbuffer,
 								VISIBILITYMAP_VALID_BITS);
 		}
@@ -1603,7 +1609,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
 		else if (PageIsAllVisible(page) && has_dead_tuples)
 		{
 			elog(WARNING, "page containing dead tuples is marked as all-visible in relation \"%s\" page %u",
-				 vacrelstats->relname, blkno);
+				 vacrelstats->errinfo.relname, blkno);
 			PageClearAllVisible(page);
 			MarkBufferDirty(buf);
 			visibilitymap_clear(onerel, blkno, vmbuffer,
@@ -1713,7 +1719,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
 	if (vacuumed_pages)
 		ereport(elevel,
 				(errmsg("\"%s\": removed %.0f row versions in %u pages",
-						vacrelstats->relname,
+						vacrelstats->errinfo.relname,
 						tups_vacuumed, vacuumed_pages)));
 
 	/*
@@ -1742,7 +1748,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",
-					vacrelstats->relname,
+					vacrelstats->errinfo.relname,
 					tups_vacuumed, num_tuples,
 					vacrelstats->scanned_pages, nblocks),
 			 errdetail_internal("%s", buf.data)));
@@ -1820,15 +1826,16 @@ lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats)
 	int			npages;
 	PGRUsage	ru0;
 	Buffer		vmbuffer = InvalidBuffer;
-	LVRelStats	olderrinfo;
+	LVSavedPosition	olderrinfo;
 
 	/* Report that we are now vacuuming the heap */
 	pgstat_progress_update_param(PROGRESS_VACUUM_PHASE,
 								 PROGRESS_VACUUM_PHASE_VACUUM_HEAP);
 
 	/* Update error traceback information */
-	olderrinfo = *vacrelstats;
-	update_vacuum_error_info(vacrelstats, VACUUM_ERRCB_PHASE_VACUUM_HEAP,
+	olderrinfo = vacrelstats->errinfo;
+	update_vacuum_error_info(&vacrelstats->errinfo,
+							 VACUUM_ERRCB_PHASE_VACUUM_HEAP,
 							 InvalidBlockNumber, NULL);
 
 	pg_rusage_init(&ru0);
@@ -1845,7 +1852,7 @@ lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats)
 		vacuum_delay_point();
 
 		tblk = ItemPointerGetBlockNumber(&vacrelstats->dead_tuples->itemptrs[tupindex]);
-		vacrelstats->blkno = tblk;
+		vacrelstats->errinfo.blkno = tblk;
 		buf = ReadBufferExtended(onerel, MAIN_FORKNUM, tblk, RBM_NORMAL,
 								 vac_strategy);
 		if (!ConditionalLockBufferForCleanup(buf))
@@ -1874,12 +1881,12 @@ lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats)
 
 	ereport(elevel,
 			(errmsg("\"%s\": removed %d row versions in %d pages",
-					vacrelstats->relname,
+					vacrelstats->errinfo.relname,
 					tupindex, npages),
 			 errdetail_internal("%s", pg_rusage_show(&ru0))));
 
 	/* Revert to the previous phase information for error traceback */
-	update_vacuum_error_info(vacrelstats,
+	update_vacuum_error_info(&vacrelstats->errinfo,
 							 olderrinfo.phase,
 							 olderrinfo.blkno,
 							 olderrinfo.indname);
@@ -1905,13 +1912,14 @@ lazy_vacuum_page(Relation onerel, BlockNumber blkno, Buffer buffer,
 	int			uncnt = 0;
 	TransactionId visibility_cutoff_xid;
 	bool		all_frozen;
-	LVRelStats	olderrinfo;
+	LVSavedPosition	olderrinfo;
 
 	pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_VACUUMED, blkno);
 
 	/* Update error traceback information */
-	olderrinfo = *vacrelstats;
-	update_vacuum_error_info(vacrelstats, VACUUM_ERRCB_PHASE_VACUUM_HEAP,
+	olderrinfo = vacrelstats->errinfo;
+	update_vacuum_error_info(&vacrelstats->errinfo,
+							 VACUUM_ERRCB_PHASE_VACUUM_HEAP,
 							 blkno, NULL);
 
 	START_CRIT_SECTION();
@@ -1991,7 +1999,7 @@ lazy_vacuum_page(Relation onerel, BlockNumber blkno, Buffer buffer,
 	}
 
 	/* Revert to the previous phase information for error traceback */
-	update_vacuum_error_info(vacrelstats,
+	update_vacuum_error_info(&vacrelstats->errinfo,
 							 olderrinfo.phase,
 							 olderrinfo.blkno,
 							 olderrinfo.indname);
@@ -2404,7 +2412,7 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats,
 	IndexVacuumInfo ivinfo;
 	const char *msg;
 	PGRUsage	ru0;
-	LVRelStats	olderrinfo;
+	LVSavedPosition	olderrinfo;
 
 	pg_rusage_init(&ru0);
 
@@ -2417,8 +2425,8 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats,
 	ivinfo.strategy = vac_strategy;
 
 	/* Update error traceback information */
-	olderrinfo = *vacrelstats;
-	update_vacuum_error_info(vacrelstats,
+	olderrinfo = vacrelstats->errinfo;
+	update_vacuum_error_info(&vacrelstats->errinfo,
 							 VACUUM_ERRCB_PHASE_VACUUM_INDEX,
 							 InvalidBlockNumber,
 							 RelationGetRelationName(indrel));
@@ -2434,12 +2442,12 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats,
 
 	ereport(elevel,
 			(errmsg(msg,
-					vacrelstats->indname,
+					vacrelstats->errinfo.indname,
 					dead_tuples->num_tuples),
 			 errdetail_internal("%s", pg_rusage_show(&ru0))));
 
 	/* Revert to the previous phase information for error traceback */
-	update_vacuum_error_info(vacrelstats,
+	update_vacuum_error_info(&vacrelstats->errinfo,
 							 olderrinfo.phase,
 							 olderrinfo.blkno,
 							 olderrinfo.indname);
@@ -2459,7 +2467,7 @@ lazy_cleanup_index(Relation indrel,
 	IndexVacuumInfo ivinfo;
 	const char *msg;
 	PGRUsage	ru0;
-	LVRelStats	olderrinfo;
+	LVSavedPosition	olderrinfo;
 
 	pg_rusage_init(&ru0);
 
@@ -2473,8 +2481,8 @@ lazy_cleanup_index(Relation indrel,
 	ivinfo.strategy = vac_strategy;
 
 	/* Update error traceback information */
-	olderrinfo = *vacrelstats;
-	update_vacuum_error_info(vacrelstats,
+	olderrinfo = vacrelstats->errinfo;
+	update_vacuum_error_info(&vacrelstats->errinfo,
 							 VACUUM_ERRCB_PHASE_INDEX_CLEANUP,
 							 InvalidBlockNumber,
 							 RelationGetRelationName(indrel));
@@ -2482,7 +2490,7 @@ lazy_cleanup_index(Relation indrel,
 	*stats = index_vacuum_cleanup(&ivinfo, *stats);
 
 	/* Revert back to the old phase information for error traceback */
-	update_vacuum_error_info(vacrelstats,
+	update_vacuum_error_info(&vacrelstats->errinfo,
 							 olderrinfo.phase,
 							 olderrinfo.blkno,
 							 olderrinfo.indname);
@@ -2597,7 +2605,7 @@ lazy_truncate_heap(Relation onerel, LVRelStats *vacrelstats)
 				vacrelstats->lock_waiter_detected = true;
 				ereport(elevel,
 						(errmsg("\"%s\": stopping truncate due to conflicting lock request",
-								vacrelstats->relname)));
+								vacrelstats->errinfo.relname)));
 				return;
 			}
 
@@ -2630,7 +2638,7 @@ lazy_truncate_heap(Relation onerel, LVRelStats *vacrelstats)
 		 * were vacuuming.
 		 */
 		new_rel_pages = count_nondeletable_pages(onerel, vacrelstats);
-		vacrelstats->blkno = new_rel_pages;
+		vacrelstats->errinfo.blkno = new_rel_pages;
 
 		if (new_rel_pages >= old_rel_pages)
 		{
@@ -2663,7 +2671,7 @@ lazy_truncate_heap(Relation onerel, LVRelStats *vacrelstats)
 
 		ereport(elevel,
 				(errmsg("\"%s\": truncated %u to %u pages",
-						vacrelstats->relname,
+						vacrelstats->errinfo.relname,
 						old_rel_pages, new_rel_pages),
 				 errdetail_internal("%s",
 									pg_rusage_show(&ru0))));
@@ -2728,7 +2736,7 @@ count_nondeletable_pages(Relation onerel, LVRelStats *vacrelstats)
 				{
 					ereport(elevel,
 							(errmsg("\"%s\": suspending truncate due to conflicting lock request",
-									vacrelstats->relname)));
+									vacrelstats->errinfo.relname)));
 
 					vacrelstats->lock_waiter_detected = true;
 					return blkno;
@@ -3521,14 +3529,14 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
 	 * Initialize vacrelstats for use as error callback arg 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.errinfo.relnamespace = get_namespace_name(RelationGetNamespace(onerel));
+	vacrelstats.errinfo.relname = pstrdup(RelationGetRelationName(onerel));
+	vacrelstats.errinfo.indname = NULL;
+	vacrelstats.errinfo.phase = VACUUM_ERRCB_PHASE_UNKNOWN; /* Not yet processing */
 
 	/* Setup error traceback support for ereport() */
 	errcallback.callback = vacuum_error_callback;
-	errcallback.arg = &vacrelstats;
+	errcallback.arg = &vacrelstats.errinfo;
 	errcallback.previous = error_context_stack;
 	error_context_stack = &errcallback;
 
@@ -3559,7 +3567,7 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
 static void
 vacuum_error_callback(void *arg)
 {
-	LVRelStats *errinfo = arg;
+	LVSavedPosition *errinfo = arg;
 
 	switch (errinfo->phase)
 	{
@@ -3600,7 +3608,7 @@ vacuum_error_callback(void *arg)
 
 /* Update vacuum error callback for the current phase, block, and index. */
 static void
-update_vacuum_error_info(LVRelStats *errinfo, int phase, BlockNumber blkno,
+update_vacuum_error_info(LVSavedPosition *errinfo, int phase, BlockNumber blkno,
 						 char *indname)
 {
 	errinfo->blkno = blkno;
-- 
2.17.0

>From 2b98df134ed4b84ed22cb7b749dc43fa43e2f370 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Mon, 22 Jun 2020 17:13:39 -0500
Subject: [PATCH 4/4] Update functions to pass only errinfo struct..

..this is a more complete change, and probably a good idea since parallel
workers have an partially-populated LVRelStats, and it's better to avoid the
idea that it's usable (dead_tuples in particular is a bogus pointer).
---
 src/backend/access/heap/vacuumlazy.c | 58 ++++++++++++++--------------
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 6bd2867660..db3bd86338 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -344,10 +344,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, double reltuples, LVSavedPosition *errinfo);
 static void lazy_cleanup_index(Relation indrel,
 							   IndexBulkDeleteResult **stats,
-							   double reltuples, bool estimated_count, LVRelStats *vacrelstats);
+							   double reltuples, bool estimated_count, LVSavedPosition *errinfo);
 static int	lazy_vacuum_page(Relation onerel, BlockNumber blkno, Buffer buffer,
 							 int tupindex, LVRelStats *vacrelstats, Buffer *vmbuffer);
 static bool should_attempt_truncation(VacuumParams *params,
@@ -367,13 +367,13 @@ static void lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult *
 										 int nindexes);
 static void parallel_vacuum_index(Relation *Irel, IndexBulkDeleteResult **stats,
 								  LVShared *lvshared, LVDeadTuples *dead_tuples,
-								  int nindexes, LVRelStats *vacrelstats);
+								  int nindexes, LVSavedPosition *errinfo);
 static void vacuum_indexes_leader(Relation *Irel, IndexBulkDeleteResult **stats,
 								  LVRelStats *vacrelstats, LVParallelState *lps,
 								  int nindexes);
 static void vacuum_one_index(Relation indrel, IndexBulkDeleteResult **stats,
 							 LVShared *lvshared, LVSharedIndStats *shared_indstats,
-							 LVDeadTuples *dead_tuples, LVRelStats *vacrelstats);
+							 LVDeadTuples *dead_tuples, LVSavedPosition *errinfo);
 static void lazy_cleanup_all_indexes(Relation *Irel, IndexBulkDeleteResult **stats,
 									 LVRelStats *vacrelstats, LVParallelState *lps,
 									 int nindexes);
@@ -1798,7 +1798,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->old_live_tuples, &vacrelstats->errinfo);
 	}
 
 	/* Increase and report the number of index scans */
@@ -2163,7 +2163,7 @@ lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult **stats,
 	 * indexes in the case where no workers are launched.
 	 */
 	parallel_vacuum_index(Irel, stats, lps->lvshared,
-						  vacrelstats->dead_tuples, nindexes, vacrelstats);
+						  vacrelstats->dead_tuples, nindexes, &vacrelstats->errinfo);
 
 	/*
 	 * Next, accumulate buffer and WAL usage.  (This must wait for the workers
@@ -2198,7 +2198,7 @@ lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult **stats,
 static void
 parallel_vacuum_index(Relation *Irel, IndexBulkDeleteResult **stats,
 					  LVShared *lvshared, LVDeadTuples *dead_tuples,
-					  int nindexes, LVRelStats *vacrelstats)
+					  int nindexes, LVSavedPosition	*errinfo)
 {
 	/*
 	 * Increment the active worker count if we are able to launch any worker.
@@ -2232,7 +2232,7 @@ parallel_vacuum_index(Relation *Irel, IndexBulkDeleteResult **stats,
 
 		/* Do vacuum or cleanup of the index */
 		vacuum_one_index(Irel[idx], &(stats[idx]), lvshared, shared_indstats,
-						 dead_tuples, vacrelstats);
+						 dead_tuples, errinfo);
 	}
 
 	/*
@@ -2273,7 +2273,7 @@ vacuum_indexes_leader(Relation *Irel, IndexBulkDeleteResult **stats,
 			skip_parallel_vacuum_index(Irel[i], lps->lvshared))
 			vacuum_one_index(Irel[i], &(stats[i]), lps->lvshared,
 							 shared_indstats, vacrelstats->dead_tuples,
-							 vacrelstats);
+							 &vacrelstats->errinfo);
 	}
 
 	/*
@@ -2293,7 +2293,7 @@ vacuum_indexes_leader(Relation *Irel, IndexBulkDeleteResult **stats,
 static void
 vacuum_one_index(Relation indrel, IndexBulkDeleteResult **stats,
 				 LVShared *lvshared, LVSharedIndStats *shared_indstats,
-				 LVDeadTuples *dead_tuples, LVRelStats *vacrelstats)
+				 LVDeadTuples *dead_tuples, LVSavedPosition *errinfo)
 {
 	IndexBulkDeleteResult *bulkdelete_res = NULL;
 
@@ -2313,10 +2313,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);
+						   lvshared->estimated_count, errinfo);
 	else
 		lazy_vacuum_index(indrel, stats, dead_tuples,
-						  lvshared->reltuples, vacrelstats);
+						  lvshared->reltuples, errinfo);
 
 	/*
 	 * Copy the index bulk-deletion result returned from ambulkdelete and
@@ -2392,7 +2392,7 @@ lazy_cleanup_all_indexes(Relation *Irel, IndexBulkDeleteResult **stats,
 			lazy_cleanup_index(Irel[idx], &stats[idx],
 							   vacrelstats->new_rel_tuples,
 							   vacrelstats->tupcount_pages < vacrelstats->rel_pages,
-							   vacrelstats);
+							   &vacrelstats->errinfo);
 	}
 }
 
@@ -2407,7 +2407,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, double reltuples, LVSavedPosition *errinfo)
 {
 	IndexVacuumInfo ivinfo;
 	const char *msg;
@@ -2425,8 +2425,8 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats,
 	ivinfo.strategy = vac_strategy;
 
 	/* Update error traceback information */
-	olderrinfo = vacrelstats->errinfo;
-	update_vacuum_error_info(&vacrelstats->errinfo,
+	olderrinfo = *errinfo;
+	update_vacuum_error_info(errinfo,
 							 VACUUM_ERRCB_PHASE_VACUUM_INDEX,
 							 InvalidBlockNumber,
 							 RelationGetRelationName(indrel));
@@ -2442,12 +2442,12 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats,
 
 	ereport(elevel,
 			(errmsg(msg,
-					vacrelstats->errinfo.indname,
+					errinfo->indname,
 					dead_tuples->num_tuples),
 			 errdetail_internal("%s", pg_rusage_show(&ru0))));
 
 	/* Revert to the previous phase information for error traceback */
-	update_vacuum_error_info(&vacrelstats->errinfo,
+	update_vacuum_error_info(errinfo,
 							 olderrinfo.phase,
 							 olderrinfo.blkno,
 							 olderrinfo.indname);
@@ -2462,7 +2462,7 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats,
 static void
 lazy_cleanup_index(Relation indrel,
 				   IndexBulkDeleteResult **stats,
-				   double reltuples, bool estimated_count, LVRelStats *vacrelstats)
+				   double reltuples, bool estimated_count, LVSavedPosition *errinfo)
 {
 	IndexVacuumInfo ivinfo;
 	const char *msg;
@@ -2481,8 +2481,8 @@ lazy_cleanup_index(Relation indrel,
 	ivinfo.strategy = vac_strategy;
 
 	/* Update error traceback information */
-	olderrinfo = vacrelstats->errinfo;
-	update_vacuum_error_info(&vacrelstats->errinfo,
+	olderrinfo = *errinfo;
+	update_vacuum_error_info(errinfo,
 							 VACUUM_ERRCB_PHASE_INDEX_CLEANUP,
 							 InvalidBlockNumber,
 							 RelationGetRelationName(indrel));
@@ -2490,7 +2490,7 @@ lazy_cleanup_index(Relation indrel,
 	*stats = index_vacuum_cleanup(&ivinfo, *stats);
 
 	/* Revert back to the old phase information for error traceback */
-	update_vacuum_error_info(&vacrelstats->errinfo,
+	update_vacuum_error_info(errinfo,
 							 olderrinfo.phase,
 							 olderrinfo.blkno,
 							 olderrinfo.indname);
@@ -3474,7 +3474,7 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
 	int			nindexes;
 	char	   *sharedquery;
 	IndexBulkDeleteResult **stats;
-	LVRelStats	vacrelstats;
+	LVSavedPosition errinfo;
 	ErrorContextCallback errcallback;
 
 	lvshared = (LVShared *) shm_toc_lookup(toc, PARALLEL_VACUUM_KEY_SHARED,
@@ -3529,14 +3529,14 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
 	 * Initialize vacrelstats for use as error callback arg by parallel
 	 * worker.
 	 */
-	vacrelstats.errinfo.relnamespace = get_namespace_name(RelationGetNamespace(onerel));
-	vacrelstats.errinfo.relname = pstrdup(RelationGetRelationName(onerel));
-	vacrelstats.errinfo.indname = NULL;
-	vacrelstats.errinfo.phase = VACUUM_ERRCB_PHASE_UNKNOWN; /* Not yet processing */
+	errinfo.relnamespace = get_namespace_name(RelationGetNamespace(onerel));
+	errinfo.relname = pstrdup(RelationGetRelationName(onerel));
+	errinfo.indname = NULL;
+	errinfo.phase = VACUUM_ERRCB_PHASE_UNKNOWN; /* Not yet processing */
 
 	/* Setup error traceback support for ereport() */
 	errcallback.callback = vacuum_error_callback;
-	errcallback.arg = &vacrelstats.errinfo;
+	errcallback.arg = &errinfo;
 	errcallback.previous = error_context_stack;
 	error_context_stack = &errcallback;
 
@@ -3545,7 +3545,7 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
 
 	/* Process indexes to perform vacuum/cleanup */
 	parallel_vacuum_index(indrels, stats, lvshared, dead_tuples, nindexes,
-						  &vacrelstats);
+						  &errinfo);
 
 	/* Report buffer/WAL usage during parallel execution */
 	buffer_usage = shm_toc_lookup(toc, PARALLEL_VACUUM_KEY_BUFFER_USAGE, false);
-- 
2.17.0

Reply via email to