From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers@postgresql.org, Andres Freund <andres@anarazel.de>
Subject: Re: error context for vacuum to include block number
Date: Wed, 11 Dec 2019 12:33:53 -0300
Message-ID: <20191211153353.GA23008@alvherre.pgsql> (raw)
In-Reply-To: <20191211143648.GK2082@telsasoft.com>
On 2019-Dec-11, Justin Pryzby wrote:
> @@ -635,6 +644,15 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
> else
> skipping_blocks = false;
>
> + /* Setup error traceback support for ereport() */
> + errcallback.callback = vacuum_error_callback;
> + cbarg.relname = relname;
> + cbarg.relnamespace = get_namespace_name(RelationGetNamespace(onerel));
> + cbarg.blkno = 0; /* Not known yet */
Shouldn't you use InvalidBlockNumber for this initialization?
> @@ -658,6 +676,8 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
>
> pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno);
>
> + cbarg.blkno = blkno;
I would put this before pgstat_progress_update_param, just out of
paranoia.
> @@ -817,7 +837,6 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
>
> buf = ReadBufferExtended(onerel, MAIN_FORKNUM, blkno,
> RBM_NORMAL, vac_strategy);
> -
> /* We need buffer cleanup lock so that we can prune HOT chains. */
> if (!ConditionalLockBufferForCleanup(buf))
> {
Lose this hunk?
> @@ -2354,3 +2376,15 @@ heap_page_is_all_visible(Relation rel, Buffer buf,
>
> return all_visible;
> }
> +
> +/*
> + * 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);
> +}
I would put this function around line 1512 (just after lazy_scan_heap)
rather than at bottom of file. (And move its prototype accordingly, to
line 156.) Or do you intend that this is going to be used for
lazy_vacuum_heap too? Maybe it should.
Patch looks good to me otherwise.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@2ndquadrant.com, pryzby@telsasoft.com, michael@paquier.xyz, andres@anarazel.de
Subject: Re: error context for vacuum to include block number
In-Reply-To: <20191211153353.GA23008@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox