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
