Re: display offset along with block number in vacuum errors

On Sun, 2 Aug 2020 at 13:24, Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Sun, Aug 02, 2020 at 01:02:42PM +0900, Masahiko Sawada wrote:
> > Thank you for updating the patch!
> >
> > Here are my comments on v3 patch:
> >
> > @@ -2024,6 +2033,11 @@ lazy_check_needs_freeze(Buffer buf, bool *hastup)
> >     if (PageIsNew(page) || PageIsEmpty(page))
> >         return false;
> >
> > +   /* Update error traceback information */
> > +   update_vacuum_error_info(vacrelstats, &saved_err_info,
> > +           VACUUM_ERRCB_PHASE_SCAN_HEAP, vacrelstats->blkno,
> > +           InvalidOffsetNumber);
> > +
> >     maxoff = PageGetMaxOffsetNumber(page);
> >     for (offnum = FirstOffsetNumber;
> >          offnum <= maxoff;
> >
> > You update the error callback phase to VACUUM_ERRCB_PHASE_SCAN_HEAP
> > but I think we're already in that phase. I'm okay with explicitly
> > setting it but on the other hand, we don't set the phase in
> > heap_page_is_all_visible(). Is there any reason for that?
>
> That part was my suggestion, so I can answer that.  I added
> update_vacuum_error_info() to lazy_check_needs_freeze() to allow it to later
> call restore_vacuum_error_info().
>
> > Also, since we don't reset vacrelstats->offnum at the end of
> > heap_page_is_all_visible(), if an error occurs by the end of
> > lazy_vacuum_page(), the caller of  heap_page_is_all_visible(), we
> > report the error context with the last offset number in the page,
> > making the users confused.
>
> So it looks like heap_page_is_all_visible() should also call the update and
> restore functions.
>
> Do you agree with my suggestion that the VACUUM phase should never try to
> report an offset ?

Yeah, given the current heap vacuum implementation, I agree that
setting the offset number during VACUUM_HEAP phase doesn't help
anything. But setting the offset number during checking tuples'
visibility in heap_page_is_all_visible() might be useful, although it
might be unlikely to find a problem in heap_page_is_all_visible() as
the tuple visibility checking is already done in lazy_scan_heap(). I
wonder if we can set SCAN_HEAP phase and update the offset number in
heap_page_is_all_visible().

> How do you think we can phrase the message to avoid confusion due to 0-based
> block number and 1-based offset ?

I think that since the user who uses this errcontext information is
likely to know more or less the internal of PostgreSQL I think 0-based
block number and 1-based offset will not be a problem. However, I
expected these are documented but couldn't find. If not yet, I think
it’s a good time to document that.

Regards,

--
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services