Date: Wed, 11 Dec 2019 12:33:53 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Michael Paquier <michael@paquier.xyz>, pgsql-hackers@postgresql.org,
	Andres Freund <andres@anarazel.de>
Subject: Re: error context for vacuum to include block number
Message-ID: <20191211153353.GA23008@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20191211143648.GK2082@telsasoft.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

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