Re: error context for vacuum to include block number

On Thu, Mar 19, 2020 at 08:20:51AM +0530, Amit Kapila wrote:
> On Tue, Mar 17, 2020 at 11:51 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > On Tue, Mar 17, 2020 at 9:52 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > Another minor point, don't we need to remove the error stack by doing
> > > "error_context_stack = errcallback.previous;" in parallel_vacuum_main?

It's a good idea, thanks.

> > Few other comments:
> > 1. The error in lazy_vacuum_heap can either have phase
> > VACUUM_ERRCB_PHASE_INDEX_* or VACUUM_ERRCB_PHASE_VACUUM_HEAP depending
> > on when it occurs.  If it occurs the first time it enters that
> > function before a call to lazy_vacuum_page, it will use phase
> > VACUUM_ERRCB_PHASE_INDEX_*, otherwise, it would use
> > VACUUM_ERRCB_PHASE_VACUUM_HEAP.  The reason is lazy_vacuum_index or
> > lazy_cleanup_index won't reset the phase after leaving that function.

I think you mean that lazy_vacuum_heap() calls ReadBuffer itself, so needs to
be in phase VACUUM_HEAP even before it calls vacuum_page().

> > 2. Also once we set phase as VACUUM_ERRCB_PHASE_VACUUM_HEAP via
> > lazy_vacuum_page, it doesn't seem to be reset to
> > VACUUM_ERRCB_PHASE_SCAN_HEAP even when we do scanning of the heap.  I
> > think you need to set phase VACUUM_ERRCB_PHASE_SCAN_HEAP inside loop.

You're right.  PHASE_SCAN_HEAP was set, but only inside a conditional.

Both those issues are due to a change in the most recent patch.  In the
previous patch, the PHASE_VACUUM_HEAP was set only by lazy_vacuum_heap(), and I
moved it recently to vacuum_page.  But it needs to be copied, as you point out.

That's unfortunate due to a lack of symmetry: lazy_vacuum_page does its own
progress update, which suggests to me that it should also set its own error
callback.  It'd be nicer if EITHER the calling functions did that (scan_heap()
and vacuum_heap()) or if it was sufficient for the called function
(vacuum_page()) to do it.  

> > I think we need to be a bit more careful in setting/resetting the
> > phase information correctly so that it doesn't display the wrong info
> > in the context in an error message.
> 
> Justin, are you planning to work on the pending comments?  If you
> want, I can try to fix some of these.  We have less time left for this
> CF, so we need to do things a bit quicker.

Thanks for reminding.

-- 
Justin