On Thu, Aug 06, 2020 at 07:39:21PM +0530, Amit Kapila wrote: > On Wed, Jul 29, 2020 at 1:09 AM Justin Pryzby <pry...@telsasoft.com> wrote: > > > > On Wed, Jul 29, 2020 at 12:35:17AM +0530, Mahendra Singh Thalor wrote: > > > Apart from these, I fixed comments given by Sawada and Michael in the > > > latest patch. Attaching v2 patch for review. > > > > Thanks. > > > > lazy_check_needs_freeze iterates over blocks and this patch changes it to > > update vacrelstats. I think it should do what > > lazy_{vacuum/cleanup}_heap/page/index do and call > > update_vacuum_error_info() at > > its beginning (even though only the offset is changed), and then > > restore_vacuum_error_info() at its end (to "revert back" to the item number > > it > > started with). > > > > I see that Mahendra has changed patch as per this suggestion but I am > not convinced that it is a good idea to sprinkle > update_vacuum_error_info()/restore_vacuum_error_info() at places more > than required. I see that it might look a bit clean from the > perspective that if tomorrow we use the function > lazy_check_needs_freeze() for a different purpose then we don't need > to worry about the wrong phase information. If we are worried about > that then we should have an assert in that function to ensure that the > current phase is VACUUM_ERRCB_PHASE_SCAN_HEAP.
The motivation was to restore the offnum, which is set to Invalid at the start of lazy_scan_heap(), and then set valid within lazy_check_needs_freeze, but should be restored or re-set to Invalid when returns to lazy_scan_heap(). If you think it's important, we could just set vacrelstats->offnum = Invalid before returning, but that's what the restore function was built for. We do direct assignment in 2 places to avoid a function call within a loop. lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats, Relation *Irel, int nindexes, bool aggressive) ... for (blkno = 0; blkno < nblocks; blkno++) { ... update_vacuum_error_info(vacrelstats, NULL, VACUUM_ERRCB_PHASE_SCAN_HEAP, blkno, InvalidOffsetNumber); if (!ConditionalLockBufferForCleanup(buf)) { ... if (!lazy_check_needs_freeze(buf, &hastup, vacrelstats)) { ... for (offnum = FirstOffsetNumber; offnum <= maxoff; offnum = OffsetNumberNext(offnum)) -- Justin