Date: Mon, 16 Mar 2020 22:51:19 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>,
	Alvaro Herrera <alvherre@2ndquadrant.com>,
	Amit Kapila <amit.kapila16@gmail.com>
Cc: 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: <20200317035119.GV26184@telsasoft.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAA4eK1JBU9ZtvwUL2VgGB+XBWZQEq+k+XJhCpnstjJ=v7744DA@mail.gmail.com>
 <20200303194900.GA17197@alvherre.pgsql>
 <CA+fd4k6h7bZSQ2nbZBqnmv7WWGtTsGPz4uynYYVAOO7vWyxzrQ@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Tue, Mar 03, 2020 at 10:05:42PM +0900, Masahiko Sawada wrote:
> I was concerned about fsm vacuum; vacuum error context might show heap
> scan while actually doing fsm vacuum. But perhaps we can update
> callback args for that. That would be helpful for user to distinguish
> that the problem seems to be either in heap vacuum or in fsm vacuum.

On Tue, Mar 03, 2020 at 04:49:00PM -0300, Alvaro Herrera wrote:
> Your use of the progress-report enum now has two warts -- the "-1"
> value, and this one,
> 
> > +#define PROGRESS_VACUUM_PHASE_VACUUM_FSM		7 /* For error reporting only */
> 
> I'd rather you define a new enum, in lazyvacuum.c.

On Mon, Mar 16, 2020 at 11:44:25AM +0530, Amit Kapila wrote:
> > On Wed, Mar 04, 2020 at 04:21:06PM +0900, Masahiko Sawada wrote:
> > > Thank you for updating the patch. But we have two more places where we
> > > do fsm vacuum.
> >
> > Oops, thanks.
...
> it is not clear whether it is a good idea to keep a phase like
> VACUUM_ERRCB_PHASE_VACUUM_FSM as it has added additional updates in
> multiple places in the code.

I think you're suggesting to rip out VACUUM_ERRCB_PHASE_VACUUM_FSM, and allow
reporting any errors there with an error context like "while scanning heap".

An alternative in the three places using VACUUM_ERRCB_PHASE_VACUUM_FSM is to
set:

|phase = VACUUM_ERRCB_PHASE_UNKNOWN;

to avoid reporting any error context until another phase is set.

-- 
Justin