Re: error context for vacuum to include block number

Hi,

On 2020-01-19 23:41:59 -0600, Justin Pryzby wrote:
> /*
> + * Return whether skipping blocks or not.
> + * Except when aggressive is set, we want to skip pages that are
> + * all-visible according to the visibility map, but only when we can skip
> + * at least SKIP_PAGES_THRESHOLD consecutive pages. Since we're reading
> + * sequentially, the OS should be doing readahead for us, so there's no
> + * gain in skipping a page now and then; that's likely to disable
> + * readahead and so be counterproductive. Also, skipping even a single
> + * page means that we can't update relfrozenxid, so we only want to do it
> + * if we can skip a goodly number of pages.
> + *
> + * When aggressive is set, we can't skip pages just because they are
> + * all-visible, but we can still skip pages that are all-frozen, since
> + * such pages do not need freezing and do not affect the value that we can
> + * safely set for relfrozenxid or relminmxid.
> + *
> + * Before entering the main loop, establish the invariant that
> + * next_unskippable_block is the next block number >= blkno that we can't
> + * skip based on the visibility map, either all-visible for a regular scan
> + * or all-frozen for an aggressive scan. We set it to nblocks if there's
> + * no such block. We also set up the skipping_blocks flag correctly at
> + * this stage.
> + *
> + * Note: The value returned by visibilitymap_get_status could be slightly
> + * out-of-date, since we make this test before reading the corresponding
> + * heap page or locking the buffer. This is OK. If we mistakenly think
> + * that the page is all-visible or all-frozen when in fact the flag's just
> + * been cleared, we might fail to vacuum the page. It's easy to see that
> + * skipping a page when aggressive is not set is not a very big deal; we
> + * might leave some dead tuples lying around, but the next vacuum will
> + * find them. But even when aggressive *is* set, it's still OK if we miss
> + * a page whose all-frozen marking has just been cleared. Any new XIDs
> + * just added to that page are necessarily newer than the GlobalXmin we
> + * computed, so they'll have no effect on the value to which we can safely
> + * set relfrozenxid. A similar argument applies for MXIDs and relminmxid.
> + *
> + * We will scan the table's last page, at least to the extent of
> + * determining whether it has tuples or not, even if it should be skipped
> + * according to the above rules; except when we've already determined that
> + * it's not worth trying to truncate the table. This avoids having
> + * lazy_truncate_heap() take access-exclusive lock on the table to attempt
> + * a truncation that just fails immediately because there are tuples in
> + * the last page. This is worth avoiding mainly because such a lock must
> + * be replayed on any hot standby, where it can be disruptive.
> + */

FWIW, I think we should just flat out delete all this logic, and replace
it with a few explicit PrefetchBuffer() calls. Just by chance I
literally just now sped up a VACUUM by more than a factor of 10, by
manually prefetching buffers. At least the linux kernel readahead logic
doesn't deal well with reading and writing to different locations in the
same file, and that's what the ringbuffer pretty invariably leads to for
workloads that aren't cached.

Partially so I'll find it when I invariably search for this in the
future:
select pg_prewarm(relid, 'buffer', 'main', blocks_done, least(blocks_done+100000, blocks_total)) from pg_stat_progress_create_index where phase = 'building index: scanning table' and datid = (SELECT oid FROM pg_database WHERE datname = current_database());
\watch 0.5

> From 623c725c8add0670b28cdbfceca1824ba5b0647c Mon Sep 17 00:00:00 2001
> From: Justin Pryzby <pryzbyj(at)telsasoft(dot)com>
> Date: Thu, 12 Dec 2019 20:54:37 -0600
> Subject: [PATCH v9 2/3] 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 | 37 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
> diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
> index 9849685..c96abdf 100644
> --- a/src/backend/access/heap/vacuumlazy.c
> +++ b/src/backend/access/heap/vacuumlazy.c
> @@ -289,6 +289,12 @@ typedef struct LVRelStats
> bool lock_waiter_detected;
> } LVRelStats;
>
> +typedef struct
> +{
> + char *relname;
> + char *relnamespace;
> + BlockNumber blkno;
> +} vacuum_error_callback_arg;
>
> /* A few variables that don't seem worth passing around as parameters */
> static int elevel = -1;
> @@ -358,6 +364,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);
>
>
> /*
> @@ -803,6 +810,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);
>
> @@ -879,6 +888,15 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
> next_unskippable_block = 0;
> skipping_blocks = skip_blocks(onerel, params, &next_unskippable_block, nblocks, &vmbuffer, aggressive);
>
> + /* Setup error traceback support for ereport() */
> + errcbarg.relnamespace = get_namespace_name(RelationGetNamespace(onerel));
> + errcbarg.relname = relname;
> + errcbarg.blkno = InvalidBlockNumber; /* Not known yet */
> + 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;
> @@ -900,6 +918,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)
> @@ -966,8 +986,11 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
> }
>
> /* Work on all the indexes, then the heap */
> + /* Don't use the errcontext handler outside this function */
> + error_context_stack = errcallback.previous;
> lazy_vacuum_all_indexes(onerel, Irel, indstats,
> vacrelstats, lps, nindexes);
> + error_context_stack = &errcallback;
>
> /* Remove tuples from heap */
> lazy_vacuum_heap(onerel, vacrelstats);
> @@ -1575,6 +1598,9 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
> RecordPageWithFreeSpace(onerel, blkno, freespace);
> }

Alternatively we could push another context for each index inside
lazy_vacuum_all_indexes(). There's been plenty bugs in indexes
triggering problems, so that could be worthwhile.

> +/*
> + * Error context callback for errors occurring during vacuum.
> + */
> +static void
> +vacuum_error_callback(void *arg)
> +{
> + vacuum_error_callback_arg *cbarg = arg;
> + errcontext("while scanning block %u of relation \"%s.%s\"",
> + cbarg->blkno, cbarg->relnamespace, cbarg->relname);
> +}
> --
> 2.7.4
>

I think it might be useful to expand the message to explain which part
of vacuuming this is about. But I'd leave that for a later patch.

> From 27a0c085d8d965252ebb8eb2e47362f27fa4203e Mon Sep 17 00:00:00 2001
> From: Justin Pryzby <pryzbyj(at)telsasoft(dot)com>
> Date: Thu, 12 Dec 2019 20:34:03 -0600
> Subject: [PATCH v9 3/3] add errcontext callback in lazy_vacuum_heap, too
>
> ---
> src/backend/access/heap/vacuumlazy.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
> index c96abdf..f380437 100644
> --- a/src/backend/access/heap/vacuumlazy.c
> +++ b/src/backend/access/heap/vacuumlazy.c
> @@ -1639,6 +1639,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
>
> /* Remove tuples from heap */
> lazy_vacuum_heap(onerel, vacrelstats);
> + error_context_stack = errcallback.previous;
> }

This I do not get. I didn't yet fully wake up, so I might just be slow?

Greetings,

Andres Freund