Hi,
On 2020-06-23 00:14:40 -0400, Tom Lane wrote:
> Andres Freund <[email protected]> writes:
> > I am only suggesting that where you save the old location, as currently
> > done with LVRelStats olderrinfo, you instead use a more specific
> > type. Not that you should pass that anywhere (except for
> > update_vacuum_error_info).
>
> As things currently stand, I don't think we need another struct
> type at all. ISTM we should hard-wire the handling of indname
> as I suggested above. Then there are only two fields to be dealt
> with, and we could just as well save them in simple local variables.
That's fine with me too.
> If there's a clear future path to needing to save/restore more
> fields, then maybe another struct type would be useful ... but
> right now the struct type declaration itself would take more
> lines of code than it would save.
FWIW, I started to be annoyed by this code when I was addding
prefetching support to vacuum, and wanted to change what's tracked
where. The number of places that needed to be touched was
disproportional.
Here's a *draft* for how I thought this roughly could look like. I think
it's nicer to not specify the exact saved state in multiple places, and
I think it's much clearer to use a separate function to restore the
state than to set a "fresh" state.
I've only applied a hacky fix for the way the indname is tracked, I
thought that'd best be discussed separately.
Greetings,
Andres Freund
diff --git i/src/backend/access/heap/vacuumlazy.c w/src/backend/access/heap/vacuumlazy.c
index 3bef0e124ba..afd6128233b 100644
--- i/src/backend/access/heap/vacuumlazy.c
+++ w/src/backend/access/heap/vacuumlazy.c
@@ -319,6 +319,12 @@ typedef struct LVRelStats
VacErrPhase phase;
} LVRelStats;
+typedef struct LVSavedPos
+{
+ BlockNumber blkno;
+ VacErrPhase phase;
+} LVSavedPos;
+
/* A few variables that don't seem worth passing around as parameters */
static int elevel = -1;
@@ -388,9 +394,9 @@ 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,
- BlockNumber blkno, char *indname);
-
+static void update_vacuum_error_info(LVRelStats *errinfo, LVSavedPos *oldpos,
+ int phase, BlockNumber blkno, char *indname);
+static void restore_vacuum_error_info(LVRelStats *errinfo, const LVSavedPos *oldpos);
/*
* heap_vacuum_rel() -- perform VACUUM for one heap relation
@@ -538,7 +544,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, NULL,
+ VACUUM_ERRCB_PHASE_TRUNCATE,
vacrelstats->nonempty_pages, NULL);
lazy_truncate_heap(onerel, vacrelstats);
}
@@ -948,8 +955,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,
- blkno, NULL);
+ update_vacuum_error_info(vacrelstats, NULL,
+ VACUUM_ERRCB_PHASE_SCAN_HEAP, blkno, NULL);
if (blkno == next_unskippable_block)
{
@@ -1820,15 +1827,14 @@ lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats)
int npages;
PGRUsage ru0;
Buffer vmbuffer = InvalidBuffer;
- LVRelStats olderrinfo;
+ LVSavedPos oldpos;
/* 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,
+ update_vacuum_error_info(vacrelstats, &oldpos, VACUUM_ERRCB_PHASE_VACUUM_HEAP,
InvalidBlockNumber, NULL);
pg_rusage_init(&ru0);
@@ -1879,10 +1885,7 @@ lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats)
errdetail_internal("%s", pg_rusage_show(&ru0))));
/* Revert to the previous phase information for error traceback */
- update_vacuum_error_info(vacrelstats,
- olderrinfo.phase,
- olderrinfo.blkno,
- olderrinfo.indname);
+ restore_vacuum_error_info(vacrelstats, &oldpos);
}
/*
@@ -1905,14 +1908,13 @@ lazy_vacuum_page(Relation onerel, BlockNumber blkno, Buffer buffer,
int uncnt = 0;
TransactionId visibility_cutoff_xid;
bool all_frozen;
- LVRelStats olderrinfo;
+ LVSavedPos oldpos;
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,
- blkno, NULL);
+ update_vacuum_error_info(vacrelstats, &oldpos,
+ VACUUM_ERRCB_PHASE_VACUUM_HEAP, blkno, NULL);
START_CRIT_SECTION();
@@ -1991,10 +1993,7 @@ lazy_vacuum_page(Relation onerel, BlockNumber blkno, Buffer buffer,
}
/* Revert to the previous phase information for error traceback */
- update_vacuum_error_info(vacrelstats,
- olderrinfo.phase,
- olderrinfo.blkno,
- olderrinfo.indname);
+ restore_vacuum_error_info(vacrelstats, &oldpos);
return tupindex;
}
@@ -2404,7 +2403,7 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats,
IndexVacuumInfo ivinfo;
const char *msg;
PGRUsage ru0;
- LVRelStats olderrinfo;
+ LVSavedPos oldpos;
pg_rusage_init(&ru0);
@@ -2417,8 +2416,7 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats,
ivinfo.strategy = vac_strategy;
/* Update error traceback information */
- olderrinfo = *vacrelstats;
- update_vacuum_error_info(vacrelstats,
+ update_vacuum_error_info(vacrelstats, &oldpos,
VACUUM_ERRCB_PHASE_VACUUM_INDEX,
InvalidBlockNumber,
RelationGetRelationName(indrel));
@@ -2439,10 +2437,7 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats,
errdetail_internal("%s", pg_rusage_show(&ru0))));
/* Revert to the previous phase information for error traceback */
- update_vacuum_error_info(vacrelstats,
- olderrinfo.phase,
- olderrinfo.blkno,
- olderrinfo.indname);
+ restore_vacuum_error_info(vacrelstats, &oldpos);
}
/*
@@ -2459,7 +2454,7 @@ lazy_cleanup_index(Relation indrel,
IndexVacuumInfo ivinfo;
const char *msg;
PGRUsage ru0;
- LVRelStats olderrcbarg;
+ LVSavedPos oldpos;
pg_rusage_init(&ru0);
@@ -2473,8 +2468,7 @@ lazy_cleanup_index(Relation indrel,
ivinfo.strategy = vac_strategy;
/* Update error traceback information */
- olderrcbarg = *vacrelstats;
- update_vacuum_error_info(vacrelstats,
+ update_vacuum_error_info(vacrelstats, &oldpos,
VACUUM_ERRCB_PHASE_INDEX_CLEANUP,
InvalidBlockNumber,
RelationGetRelationName(indrel));
@@ -2482,10 +2476,8 @@ 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,
- olderrcbarg.phase,
- olderrcbarg.blkno,
- olderrcbarg.indname);
+ restore_vacuum_error_info(vacrelstats, &oldpos);
+
if (!(*stats))
return;
@@ -3600,9 +3592,15 @@ 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,
- char *indname)
+update_vacuum_error_info(LVRelStats *errinfo, LVSavedPos *oldpos,
+ int phase, BlockNumber blkno, char *indname)
{
+ if (oldpos)
+ {
+ oldpos->blkno = errinfo->blkno;
+ oldpos->phase = errinfo->phase;
+ }
+
errinfo->blkno = blkno;
errinfo->phase = phase;
@@ -3613,3 +3611,17 @@ update_vacuum_error_info(LVRelStats *errinfo, int phase, BlockNumber blkno,
/* For index phases, save the name of the current index for the callback */
errinfo->indname = indname ? pstrdup(indname) : NULL;
}
+
+static void
+restore_vacuum_error_info(LVRelStats *errinfo, const LVSavedPos *oldpos)
+{
+ errinfo->blkno = oldpos->blkno;
+ errinfo->phase = oldpos->phase;
+
+ /* FIXME: Imo pretty ugly that we don't restore the name properly */
+ if (errinfo->indname)
+ {
+ pfree(errinfo->indname);
+ errinfo->indname = NULL;
+ }
+}
