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(dot)kapila16(at)gmail(dot)com> wrote:
> > On Tue, Mar 17, 2020 at 9:52 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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