Re: Should vacuum process config file reload more often

On Sat, Mar 18, 2023 at 6:47 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
> On Wed, Mar 15, 2023 at 1:14 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > On Sat, Mar 11, 2023 at 8:11 AM Melanie Plageman
> > <melanieplageman@gmail.com> wrote:
> > > > > Also not sure how the patch interacts with failsafe autovac and parallel
> > > > > vacuum.
> > > >
> > > > Good point.
> > > >
> > > > When entering the failsafe mode, we disable the vacuum delays (see
> > > > lazy_check_wraparound_failsafe()). We need to keep disabling the
> > > > vacuum delays even after reloading the config file. One idea is to
> > > > have another global variable indicating we're in the failsafe mode.
> > > > vacuum_delay_point() doesn't update VacuumCostActive if the flag is
> > > > true.
> > >
> > > I think we might not need to do this. Other than in
> > > lazy_check_wraparound_failsafe(), VacuumCostActive is only updated in
> > > two places:
> > >
> > > 1) in vacuum() which autovacuum will call per table. And failsafe is
> > > reset per table as well.
> > >
> > > 2) in vacuum_delay_point(), but, since VacuumCostActive will already be
> > > false when we enter vacuum_delay_point() the next time after
> > > lazy_check_wraparound_failsafe(), we won't set VacuumCostActive there.
> >
> > Indeed. But does it mean that there is no code path to turn
> > vacuum-delay on, even when vacuum_cost_delay is updated from 0 to
> > non-0?
>
> Ah yes! Good point. This is true.
> I'm not sure how to cheaply allow for re-enabling delays after disabling
> them in the middle of a table vacuum.
>
> I don't see a way around checking if we need to reload the config file
> on every call to vacuum_delay_point() (currently, we are only doing this
> when we have to wait anyway). It seems expensive to do this check every
> time. If we do do this, we would update VacuumCostActive when updating
> VacuumCostDelay, and we would need a global variable keeping the
> failsafe status, as you mentioned.
>
> It could be okay to say that you can only disable cost-based delays in
> the middle of vacuuming a table (i.e. you cannot enable them if they are
> already disabled until you start vacuuming the next table). Though maybe
> it is weird that you can increase the delay but not re-enable it...

So, I thought about it some more, and I think it is a bit odd that you
can increase the delay and limit but not re-enable them if they were
disabled. And, perhaps it would be okay to check ConfigReloadPending at
the top of vacuum_delay_point() instead of only after sleeping. It is
just one more branch. We can check if VacuumCostActive is false after
checking if we should reload and doing so if needed and return early.
I've implemented that in attached v6.

I added in the global we discussed for VacuumFailsafeActive. If we keep
it, we can probably remove the one in LVRelState -- as it seems
redundant. Let me know what you think.

- Melanie