Date: Thu, 27 Feb 2020 21:09:42 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>,
	Andres Freund <andres@anarazel.de>,
	Michael Paquier <michael@paquier.xyz>, pgsql-hackers@postgresql.org
Subject: Re: error context for vacuum to include block number
Message-ID: <20200228000942.GA16156@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200227210813.GC29456@telsasoft.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On 2020-Feb-27, Justin Pryzby wrote:

> Originally, the patch only supported "scanning heap", and set the callback
> strictly, to avoid having callback installed when calling other functions (like
> vacuuming heap/indexes).
> 
> Then incrementally added callbacks in increasing number of places.  We only
> need one errcontext.  And possibly you're right that the callback could always
> be in place (?).  But what about things like vacuuming FSM ?  I think we'd need
> another "phase" for that (or else invent a PHASE_IGNORE to do nothing).  Would
> VACUUM_FSM be added to progress reporting, too?  We're also talking about new
> phase for TRUNCATE_PREFETCH and TRUNCATE_WAIT.

I think we should use a separate enum. It's simple enough, and there's
no reason to use the same enum for two different things if it seems to
complicate matters.

> Regarding the cbarg, at one point I took a suggestion from Andres to use the
> LVRelStats struct.  I got rid of that since I didn't like sharing "blkno"
> between heap scanning and heap vacuuming, and needs to be reset when switching
> back to scanning heap.  I experimented now going back to that now.  The only
> utility is in having an single allocation of relname/space.

I'm unsure about reusing that struct.  Not saying don't do it, just ...
unsure.  It possibly has other responsibilities.

I don't think there's a reason to keep 0002 separate.

Regarding this,

> +		case PROGRESS_VACUUM_PHASE_VACUUM_HEAP:
> +			if (BlockNumberIsValid(cbarg->blkno))
> +				errcontext("while vacuuming block %u of relation \"%s.%s\"",
> +						cbarg->blkno, cbarg->relnamespace, cbarg->relname);
> +			break;

I think you should still call errcontext() when blkno is invalid.  In
fact, just remove the "if" line altogether and let it show whatever
value is there.  It should work okay.  We don't expect the value to be
invalid anyway.

Maybe it would make sense to make the LVRelStats struct members be char
arrays rather than pointers.  Then you memcpy() or strlcpy() them
instead of palloc/free.

Please don't cuddle your braces.


-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services