Re: Should vacuum process config file reload more often - Mailing list pgsql-hackers
From | Melanie Plageman |
---|---|
Subject | Re: Should vacuum process config file reload more often |
Date | |
Msg-id | CAAKRu_Y0xdDu-6rKEJk8APJTupdOobjoNtGVKLsfyFYcQEMKZw@mail.gmail.com Whole thread Raw |
In response to | Re: Should vacuum process config file reload more often (Melanie Plageman <melanieplageman@gmail.com>) |
List | pgsql-hackers |
On Fri, Mar 10, 2023 at 6:11 PM Melanie Plageman <melanieplageman@gmail.com> wrote: > > Quotes below are combined from two of Sawada-san's emails. > > I've also attached a patch with my suggested current version. > > On Thu, Mar 9, 2023 at 10:27 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > On Fri, Mar 10, 2023 at 11:23 AM Melanie Plageman > > <melanieplageman@gmail.com> wrote: > > > > > > On Tue, Mar 7, 2023 at 12:10 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > > > > > On Mon, Mar 6, 2023 at 5:26 AM Melanie Plageman > > > > <melanieplageman@gmail.com> wrote: > > > > > > > > > > On Thu, Mar 2, 2023 at 6:37 PM Melanie Plageman > > > > > In this version I've removed wi_cost_delay from WorkerInfoData. There is > > > > > no synchronization of cost_delay amongst workers, so there is no reason > > > > > to keep it in shared memory. > > > > > > > > > > One consequence of not updating VacuumCostDelay from wi_cost_delay is > > > > > that we have to have a way to keep track of whether or not autovacuum > > > > > table options are in use. > > > > > > > > > > This patch does this in a cringeworthy way. I added two global > > > > > variables, one to track whether or not cost delay table options are in > > > > > use and the other to store the value of the table option cost delay. I > > > > > didn't want to use a single variable with a special value to indicate > > > > > that table option cost delay is in use because > > > > > autovacuum_vacuum_cost_delay already has special values that mean > > > > > certain things. My code needs a better solution. > > > > > > > > While it's true that wi_cost_delay doesn't need to be shared, it seems > > > > to make the logic somewhat complex. We need to handle cost_delay in a > > > > different way from other vacuum-related parameters and we need to make > > > > sure av[_use]_table_option_cost_delay are set properly. Removing > > > > wi_cost_delay from WorkerInfoData saves 8 bytes shared memory per > > > > autovacuum worker but it might be worth considering to keep > > > > wi_cost_delay for simplicity. > > > > > > Ah, it turns out we can't really remove wi_cost_delay from WorkerInfo > > > anyway because the launcher doesn't know anything about table options > > > and so the workers have to keep an updated wi_cost_delay that the > > > launcher or other autovac workers who are not vacuuming that table can > > > read from when calculating the new limit in autovac_balance_cost(). > > > > IIUC if any of the cost delay parameters has been set individually, > > the autovacuum worker is excluded from the balance algorithm. > > Ah, yes! That's right. So it is not a problem. Then I still think > removing wi_cost_delay from the worker info makes sense. wi_cost_delay > is a double and can't easily be accessed atomically the way > wi_cost_limit can be. > > Keeping the cost delay local to the backends also makes it clear that > cost delay is not something that should be written to by other backends > or that can differ from worker to worker. Without table options in the > picture, the cost delay should be the same for any worker who has > reloaded the config file. > > As for the cost limit safe access issue, maybe we can avoid a LWLock > acquisition for reading wi_cost_limit by using an atomic similar to what > you suggested here for "did_rebalance". > > > > I've added in a shared lock for reading from wi_cost_limit in this > > > patch. However, AutoVacuumUpdateLimit() is called unconditionally in > > > vacuum_delay_point(), which is called quite often (per block-ish), so I > > > was trying to think if there is a way we could avoid having to check > > > this shared memory variable on every call to vacuum_delay_point(). > > > Rebalances shouldn't happen very often (done by the launcher when a new > > > worker is launched and by workers between vacuuming tables). Maybe we > > > can read from it less frequently? > > > > Yeah, acquiring the lwlock for every call to vacuum_delay_point() > > seems to be harmful. One idea would be to have one sig_atomic_t > > variable in WorkerInfoData and autovac_balance_cost() set it to true > > after rebalancing the worker's cost-limit. The worker can check it > > without locking and update its delay parameters if the flag is true. > > Instead of having the atomic indicate whether or not someone (launcher > or another worker) did a rebalance, it would simply store the current > cost limit. Then the worker can normally access it with a simple read. > > My rationale is that if we used an atomic to indicate whether or not we > did a rebalance ("did_rebalance"), we would have the same cache > coherency guarantees as if we just used the atomic for the cost limit. > If we read from the "did_rebalance" variable and missed someone having > written to it on another core, we still wouldn't get around to checking > the wi_cost_limit variable in shared memory, so it doesn't matter that > we bothered to keep it in shared memory and use a lock to access it. > > I noticed we don't allow wi_cost_limit to ever be less than 0, so we > could store wi_cost_limit in an atomic uint32. > > I'm not sure if it is okay to do pg_atomic_read_u32() and > pg_atomic_unlocked_write_u32() or if we need pg_atomic_write_u32() in > most cases. > > I've implemented the atomic cost limit in the attached patch. Though, > I'm pretty unsure about how I initialized the atomics in > AutoVacuumShmemInit()... > > If the consensus is that it is simply too confusing to take > wi_cost_delay out of WorkerInfo, we might be able to afford using a > shared lock to access it because we won't call AutoVacuumUpdateDelay() > on every invocation of vacuum_delay_point() -- only when we've reloaded > the config file. One such implementation is attached. - Melanie
Attachment
pgsql-hackers by date: