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