Re: error context for vacuum to include block number

On Tue, Mar 17, 2020 at 9:21 AM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> 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".
>

Right, because that is what we do for progress updates.

> 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.
>

Right, that is an alternative, but not sure if it is worth adding
additional code. I am trying to see if we can get this functionality
without adding code at too many places primarily because the code in
this area is already complex, so adding more things can make it
difficult to understand.

Another minor point, don't we need to remove the error stack by doing
"error_context_stack = errcallback.previous;" in parallel_vacuum_main?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com