Date: Wed, 4 Mar 2020 15:53:38 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Alvaro Herrera <alvherre@2ndquadrant.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: <20200304215338.GA31435@telsasoft.com>
References: <20200303193205.GG684@telsasoft.com>
 <20200303194900.GA17197@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200303194900.GA17197@alvherre.pgsql>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

On Tue, Mar 03, 2020 at 04:49:00PM -0300, Alvaro Herrera wrote:
> On 2020-Mar-03, Justin Pryzby wrote:
> > On Thu, Feb 27, 2020 at 09:09:42PM -0300, Alvaro Herrera wrote:
> > > > +		case PROGRESS_VACUUM_PHASE_VACUUM_HEAP:
> > > > +			if (BlockNumberIsValid(cbarg->blkno))
> > > > +				errcontext("while vacuuming block %u of relation \"%s.%s\"",
> > > 
> > > I think you should still call errcontext() when blkno is invalid.
> > 
> > In my experience while testing, the conditional avoids lots of CONTEXT noise
> > from interrupted autovacuum, at least.  I couldn't easily reproduce it with the
> > current patch, though, maybe due to less pushing and popping.
> 
> I think you're saying that the code had the bug that too many lines were
> reported because of excessive stack pushes, and you worked around it by
> making the errcontext() be conditional; and that now the bug is fixed by
> avoiding the push/pop games -- which explains why you can no longer
> reproduce it.  I don't see why you want to keep the no-longer-needed
> workaround.

No - the issue I observed from autovacuum ("while scanning block number
4294967295") was unrelated to showing multiple context lines (that issue I only
saw in the v22 patch, and was related to vacuum_one_index being used by both
leader and parallel workers).

The locations showing a block number first set vacrelstats->blkno to
InvalidBlockNumber, and then later update the vacrelstats->blkno from a loop
variable.  I think today's v24 patch makes it harder to hit the window where
it's set to InvalidBlockNumber, by moving VACUUM_HEAP context into
vacuum_page(), but I don't think we should rely on an absence of
CHECK_FOR_INTERRUPTS() to avoid misleading noise context.  

-- 
Justin