From: Justin Pryzby <pryzby@telsasoft.com>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers@postgresql.org
Subject: Re: error context for vacuum to include block number
Date: Wed, 4 Mar 2020 15:53:38 -0600
Message-ID: <20200304215338.GA31435@telsasoft.com> (raw)
In-Reply-To: <20200303194900.GA17197@alvherre.pgsql>
References: <20200303193205.GG684@telsasoft.com>
<20200303194900.GA17197@alvherre.pgsql>
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
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: pryzby@telsasoft.com, alvherre@2ndquadrant.com, masahiko.sawada@2ndquadrant.com, andres@anarazel.de, michael@paquier.xyz
Subject: Re: error context for vacuum to include block number
In-Reply-To: <20200304215338.GA31435@telsasoft.com>
* 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