On Sun, 2 Aug 2020 at 13:24, Justin Pryzby <pry...@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