Re: error context for vacuum to include block number

On Thu, Mar 19, 2020 at 9:38 AM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> On Thu, Mar 19, 2020 at 08:20:51AM +0530, Amit Kapila wrote:
>
> > > 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().
>

Right.

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

I think if we do it inside for loop, then we don't need to set it
conditionally at multiple places. I have changed like that in the
attached patch, see if that makes sense to you.

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

Right, but adding in callers will spread at multiple places.

I have made a few additional changes in the attached. (a) Removed
VACUUM_ERRCB_PHASE_VACUUM_FSM as I think we have to add it at many
places, you seem to have added for FreeSpaceMapVacuumRange() but not
for RecordPageWithFreeSpace(), (b) Reset the phase to
VACUUM_ERRCB_PHASE_UNKNOWN after finishing the work for a particular
phase, so that the new phase shouldn't continue in the callers.

I have another idea to make (b) better. How about if a call to
update_vacuum_error_cbarg returns information of old phase (blkno,
phase, and indname) along with what it is doing now and then once the
work for the current phase is over it can reset it back with old phase
information? This way the callee after finishing the new phase work
would be able to reset back to the old phase. This will work
something similar to our MemoryContextSwitchTo.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com