On Thu, Aug 6, 2020 at 7:51 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Thu, Aug 06, 2020 at 07:39:21PM +0530, Amit Kapila wrote:
> > On Wed, Jul 29, 2020 at 1:09 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > >
> > >
> > > 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,
>
Yeah, I would prefer that and probably a comment to indicate why we
are doing that.
> but that's what the restore function was built for.
>
I think it would be better to call restore wherever we call update. I
see your point that there is some value doing it via update/restore
but I think we should try to avoid that at many places unless it is
required and we already update blockno information without
update/restore at few places.
--
With Regards,
Amit Kapila.
