Re: display offset along with block number in vacuum errors

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 ?

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

Thanks,
-- 
Justin