public inbox for [email protected]help / color / mirror / Atom feed
Re: Should vacuum process config file reload more often 26+ messages / 6 participants [nested] [flat]
* Re: Should vacuum process config file reload more often @ 2023-04-05 19:43 Melanie Plageman <[email protected]> 0 siblings, 2 replies; 26+ messages in thread From: Melanie Plageman @ 2023-04-05 19:43 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Masahiko Sawada <[email protected]>; Kyotaro Horiguchi <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> On Wed, Apr 5, 2023 at 2:56 PM Robert Haas <[email protected]> wrote: > > + /* > + * Balance and update limit values for autovacuum workers. We must > + * always do this in case the autovacuum launcher or another > + * autovacuum worker has recalculated the number of workers across > + * which we must balance the limit. This is done by the launcher when > + * launching a new worker and by workers before vacuuming each table. > + */ > > I don't quite understand what's going on here. A big reason that I'm > worried about this whole issue in the first place is that sometimes > there's a vacuum going on a giant table and you can't get it to go > fast. You want it to absorb new settings, and to do so quickly. I > realize that this is about the number of workers, not the actual cost > limit, so that makes what I'm about to say less important. But ... is > this often enough? Like, the time before we move onto the next table > could be super long. The time before a new worker is launched should > be ~autovacuum_naptime/autovacuum_max_workers or ~20s with default > settings, so that's not horrible, but I'm kind of struggling to > understand the rationale for this particular choice. Maybe it's fine. VacuumUpdateCosts() also calls AutoVacuumUpdateCostLimit(), so this will happen if a config reload is pending the next time vacuum_delay_point() is called (which is pretty often -- roughly once per block vacuumed but definitely more than once per table). Relevant code is at the top of vacuum_delay_point(): if (ConfigReloadPending && IsAutoVacuumWorkerProcess()) { ConfigReloadPending = false; ProcessConfigFile(PGC_SIGHUP); VacuumUpdateCosts(); } - Melanie ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-04-05 19:58 Robert Haas <[email protected]> parent: Melanie Plageman <[email protected]> 1 sibling, 0 replies; 26+ messages in thread From: Robert Haas @ 2023-04-05 19:58 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Masahiko Sawada <[email protected]>; Kyotaro Horiguchi <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> On Wed, Apr 5, 2023 at 3:44 PM Melanie Plageman <[email protected]> wrote: > VacuumUpdateCosts() also calls AutoVacuumUpdateCostLimit(), so this will > happen if a config reload is pending the next time vacuum_delay_point() > is called (which is pretty often -- roughly once per block vacuumed but > definitely more than once per table). > > Relevant code is at the top of vacuum_delay_point(): > > if (ConfigReloadPending && IsAutoVacuumWorkerProcess()) > { > ConfigReloadPending = false; > ProcessConfigFile(PGC_SIGHUP); > VacuumUpdateCosts(); > } Yeah, that all makes sense, and I did see that logic, but I'm struggling to reconcile it with what that comment says. Maybe I'm just confused about what that comment is trying to explain. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-04-06 03:10 Melanie Plageman <[email protected]> parent: Melanie Plageman <[email protected]> 1 sibling, 1 reply; 26+ messages in thread From: Melanie Plageman @ 2023-04-06 03:10 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Masahiko Sawada <[email protected]>; Kyotaro Horiguchi <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> On Wed, Apr 5, 2023 at 3:43 PM Melanie Plageman <[email protected]> wrote: > > On Wed, Apr 5, 2023 at 2:56 PM Robert Haas <[email protected]> wrote: > > > > + /* > > + * Balance and update limit values for autovacuum workers. We must > > + * always do this in case the autovacuum launcher or another > > + * autovacuum worker has recalculated the number of workers across > > + * which we must balance the limit. This is done by the launcher when > > + * launching a new worker and by workers before vacuuming each table. > > + */ > > > > I don't quite understand what's going on here. A big reason that I'm > > worried about this whole issue in the first place is that sometimes > > there's a vacuum going on a giant table and you can't get it to go > > fast. You want it to absorb new settings, and to do so quickly. I > > realize that this is about the number of workers, not the actual cost > > limit, so that makes what I'm about to say less important. But ... is > > this often enough? Like, the time before we move onto the next table > > could be super long. The time before a new worker is launched should > > be ~autovacuum_naptime/autovacuum_max_workers or ~20s with default > > settings, so that's not horrible, but I'm kind of struggling to > > understand the rationale for this particular choice. Maybe it's fine. > > VacuumUpdateCosts() also calls AutoVacuumUpdateCostLimit(), so this will > happen if a config reload is pending the next time vacuum_delay_point() > is called (which is pretty often -- roughly once per block vacuumed but > definitely more than once per table). > > Relevant code is at the top of vacuum_delay_point(): > > if (ConfigReloadPending && IsAutoVacuumWorkerProcess()) > { > ConfigReloadPending = false; > ProcessConfigFile(PGC_SIGHUP); > VacuumUpdateCosts(); > } > Gah, I think I misunderstood you. You are saying that only calling AutoVacuumUpdateCostLimit() after napping while vacuuming a table may not be enough. The frequency at which the number of workers changes will likely be different. This is a good point. It's kind of weird to call AutoVacuumUpdateCostLimit() only after napping... Hmm. Well, I don't think we want to call AutoVacuumUpdateCostLimit() on every call to vacuum_delay_point(), though, do we? It includes two atomic operations. Maybe that pales in comparison to what we are doing on each page we are vacuuming. I haven't properly thought about it. Is there some other relevant condition we can use to determine whether or not to call AutoVacuumUpdateCostLimit() on a given invocation of vacuum_delay_point()? Maybe something with naptime/max workers? I'm not sure if there is a more reliable place than vacuum_delay_point() for us to do this. I poked around heap_vacuum_rel(), but I think we would want this cost limit update to happen table AM-agnostically. Thank you for bringing this up! - Melanie ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-04-06 15:52 Melanie Plageman <[email protected]> parent: Melanie Plageman <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Melanie Plageman @ 2023-04-06 15:52 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Masahiko Sawada <[email protected]>; Kyotaro Horiguchi <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> On Wed, Apr 5, 2023 at 11:10 PM Melanie Plageman <[email protected]> wrote: > On Wed, Apr 5, 2023 at 3:43 PM Melanie Plageman <[email protected]> wrote: > > On Wed, Apr 5, 2023 at 2:56 PM Robert Haas <[email protected]> wrote: > > > > > > + /* > > > + * Balance and update limit values for autovacuum workers. We must > > > + * always do this in case the autovacuum launcher or another > > > + * autovacuum worker has recalculated the number of workers across > > > + * which we must balance the limit. This is done by the launcher when > > > + * launching a new worker and by workers before vacuuming each table. > > > + */ > > > > > > I don't quite understand what's going on here. A big reason that I'm > > > worried about this whole issue in the first place is that sometimes > > > there's a vacuum going on a giant table and you can't get it to go > > > fast. You want it to absorb new settings, and to do so quickly. I > > > realize that this is about the number of workers, not the actual cost > > > limit, so that makes what I'm about to say less important. But ... is > > > this often enough? Like, the time before we move onto the next table > > > could be super long. The time before a new worker is launched should > > > be ~autovacuum_naptime/autovacuum_max_workers or ~20s with default > > > settings, so that's not horrible, but I'm kind of struggling to > > > understand the rationale for this particular choice. Maybe it's fine. > > > > VacuumUpdateCosts() also calls AutoVacuumUpdateCostLimit(), so this will > > happen if a config reload is pending the next time vacuum_delay_point() > > is called (which is pretty often -- roughly once per block vacuumed but > > definitely more than once per table). > > > > Relevant code is at the top of vacuum_delay_point(): > > > > if (ConfigReloadPending && IsAutoVacuumWorkerProcess()) > > { > > ConfigReloadPending = false; > > ProcessConfigFile(PGC_SIGHUP); > > VacuumUpdateCosts(); > > } > > > > Gah, I think I misunderstood you. You are saying that only calling > AutoVacuumUpdateCostLimit() after napping while vacuuming a table may > not be enough. The frequency at which the number of workers changes will > likely be different. This is a good point. > It's kind of weird to call AutoVacuumUpdateCostLimit() only after napping... A not fully baked idea for a solution: Why not keep the balanced limit in the atomic instead of the number of workers for balance. If we expect all of the workers to have the same value for cost limit, then why would we just count the workers and not also do the division and store that in the atomic variable. We are worried about the division not being done often enough, not the number of workers being out of date. This solves that, right? - Melanie ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-04-06 17:18 Robert Haas <[email protected]> parent: Melanie Plageman <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Robert Haas @ 2023-04-06 17:18 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Masahiko Sawada <[email protected]>; Kyotaro Horiguchi <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> On Thu, Apr 6, 2023 at 11:52 AM Melanie Plageman <[email protected]> wrote: > > Gah, I think I misunderstood you. You are saying that only calling > > AutoVacuumUpdateCostLimit() after napping while vacuuming a table may > > not be enough. The frequency at which the number of workers changes will > > likely be different. This is a good point. > > It's kind of weird to call AutoVacuumUpdateCostLimit() only after napping... > > A not fully baked idea for a solution: > > Why not keep the balanced limit in the atomic instead of the number of > workers for balance. If we expect all of the workers to have the same > value for cost limit, then why would we just count the workers and not > also do the division and store that in the atomic variable. We are > worried about the division not being done often enough, not the number > of workers being out of date. This solves that, right? A bird in the hand is worth two in the bush, though. We don't really have time to redesign the patch before feature freeze, and I can't convince myself that there's a big enough problem with what you already did that it would be worth putting off fixing this for another year. Reading your newer emails, I think that the answer to my original question is "we don't want to do it at every vacuum_delay_point because it might be too costly," which is reasonable. I don't particularly like this new idea, either, I think. While it may be true that we expect all the workers to come up with the same answer, they need not, because rereading the configuration file isn't synchronized. It would be pretty lame if a worker that had reread an updated value from the configuration file recomputed the value, and then another worker that still had an older value recalculated it again just afterward. Keeping only the number of workers in memory avoids the possibility of thrashing around in situations like that. I do kind of wonder if it would be possible to rejigger things so that we didn't have to keep recalculating av_nworkersForBalance, though. Perhaps now is not the time due to the impending freeze, but maybe we should explore maintaining that value in such a way that it is correct at every instant, instead of recalculating it at intervals. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-04-06 18:55 Daniel Gustafsson <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Daniel Gustafsson @ 2023-04-06 18:55 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Melanie Plageman <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Masahiko Sawada <[email protected]>; Kyotaro Horiguchi <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> > On 6 Apr 2023, at 19:18, Robert Haas <[email protected]> wrote: > > On Thu, Apr 6, 2023 at 11:52 AM Melanie Plageman > <[email protected]> wrote: >>> Gah, I think I misunderstood you. You are saying that only calling >>> AutoVacuumUpdateCostLimit() after napping while vacuuming a table may >>> not be enough. The frequency at which the number of workers changes will >>> likely be different. This is a good point. >>> It's kind of weird to call AutoVacuumUpdateCostLimit() only after napping... >> >> A not fully baked idea for a solution: >> >> Why not keep the balanced limit in the atomic instead of the number of >> workers for balance. If we expect all of the workers to have the same >> value for cost limit, then why would we just count the workers and not >> also do the division and store that in the atomic variable. We are >> worried about the division not being done often enough, not the number >> of workers being out of date. This solves that, right? > > A bird in the hand is worth two in the bush, though. We don't really > have time to redesign the patch before feature freeze, and I can't > convince myself that there's a big enough problem with what you > already did that it would be worth putting off fixing this for another > year. +1, I'd rather see we did a conservative version of the feature first and expand upon it in the 17 cycle. > Reading your newer emails, I think that the answer to my > original question is "we don't want to do it at every > vacuum_delay_point because it might be too costly," which is > reasonable. I think we kind of need to get to that granularity eventually, but it's not a showstopper for this feature, and can probably benefit from being done in the context of a larger av-worker re-think (the importance of which discussed downthread). -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-04-06 19:09 Melanie Plageman <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Melanie Plageman @ 2023-04-06 19:09 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Robert Haas <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Masahiko Sawada <[email protected]>; Kyotaro Horiguchi <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> v17 attached does not yet fix the logging problem or variable naming problem. I have not changed where AutoVacuumUpdateCostLimit() is called either. This is effectively just a round of cleanup. I hope I have managed to address all other code review feedback so far, though some may have slipped through the cracks. On Wed, Apr 5, 2023 at 2:56 PM Robert Haas <[email protected]> wrote: > On Wed, Apr 5, 2023 at 11:29 AM Melanie Plageman <[email protected]> wrote: > + /* > + * Balance and update limit values for autovacuum workers. We must > + * always do this in case the autovacuum launcher or another > + * autovacuum worker has recalculated the number of workers across > + * which we must balance the limit. This is done by the launcher when > + * launching a new worker and by workers before vacuuming each table. > + */ > > I don't quite understand what's going on here. A big reason that I'm > worried about this whole issue in the first place is that sometimes > there's a vacuum going on a giant table and you can't get it to go > fast. You want it to absorb new settings, and to do so quickly. I > realize that this is about the number of workers, not the actual cost > limit, so that makes what I'm about to say less important. But ... is > this often enough? Like, the time before we move onto the next table > could be super long. The time before a new worker is launched should > be ~autovacuum_naptime/autovacuum_max_workers or ~20s with default > settings, so that's not horrible, but I'm kind of struggling to > understand the rationale for this particular choice. Maybe it's fine. I've at least updated this comment to be more correct/less misleading. > > + if (autovacuum_vac_cost_limit > 0) > + VacuumCostLimit = autovacuum_vac_cost_limit; > + else > + VacuumCostLimit = vacuum_cost_limit; > + > + /* Only balance limit if no cost-related storage > parameters specified */ > + if (pg_atomic_unlocked_test_flag(&MyWorkerInfo->wi_dobalance)) > + return; > + Assert(VacuumCostLimit > 0); > + > + nworkers_for_balance = pg_atomic_read_u32( > + > &AutoVacuumShmem->av_nworkersForBalance); > + > + /* There is at least 1 autovac worker (this worker). */ > + if (nworkers_for_balance <= 0) > + elog(ERROR, "nworkers_for_balance must be > 0"); > + > + VacuumCostLimit = Max(VacuumCostLimit / > nworkers_for_balance, 1); > > I think it would be better stylistically to use a temporary variable > here and only assign the final value to VacuumCostLimit. I tried that and thought it adding confusing clutter. If it is a code cleanliness issue, I am willing to change it, though. On Wed, Apr 5, 2023 at 3:04 PM Daniel Gustafsson <[email protected]> wrote: > > > On 5 Apr 2023, at 20:55, Robert Haas <[email protected]> wrote: > > > Again, I don't think this is something we should try to > > address right now under time pressure, but in the future, I think we > > should consider ripping this behavior out. > > I would not be opposed to that, but I wholeheartedly agree that it's not the > job of this patch (or any patch at this point in the cycle). > > > + if (autovacuum_vac_cost_limit > 0) > > + VacuumCostLimit = autovacuum_vac_cost_limit; > > + else > > + VacuumCostLimit = vacuum_cost_limit; > > + > > + /* Only balance limit if no cost-related storage > > parameters specified */ > > + if (pg_atomic_unlocked_test_flag(&MyWorkerInfo->wi_dobalance)) > > + return; > > + Assert(VacuumCostLimit > 0); > > + > > + nworkers_for_balance = pg_atomic_read_u32( > > + > > &AutoVacuumShmem->av_nworkersForBalance); > > + > > + /* There is at least 1 autovac worker (this worker). */ > > + if (nworkers_for_balance <= 0) > > + elog(ERROR, "nworkers_for_balance must be > 0"); > > + > > + VacuumCostLimit = Max(VacuumCostLimit / > > nworkers_for_balance, 1); > > > > I think it would be better stylistically to use a temporary variable > > here and only assign the final value to VacuumCostLimit. > > I can agree with that. Another supertiny nitpick on the above is to not end a > single-line comment with a period. I have fixed this. On Thu, Apr 6, 2023 at 2:40 AM Masahiko Sawada <[email protected]> wrote: > > On Thu, Apr 6, 2023 at 12:29 AM Melanie Plageman > <[email protected]> wrote: > > > > Thanks all for the reviews. > > > > v16 attached. I put it together rather quickly, so there might be a few > > spurious whitespaces or similar. There is one rather annoying pgindent > > outlier that I have to figure out what to do about as well. > > > > The remaining functional TODOs that I know of are: > > > > - Resolve what to do about names of GUC and vacuum variables for cost > > limit and cost delay (since it may affect extensions) > > > > - Figure out what to do about the logging message which accesses dboid > > and tableoid (lock/no lock, where to put it, etc) > > > > - I see several places in docs which reference the balancing algorithm > > for autovac workers. I did not read them in great detail, but we may > > want to review them to see if any require updates. > > > > - Consider whether or not the initial two commits should just be > > squashed with the third commit > > > > - Anything else reviewers are still unhappy with > > > > > > On Wed, Apr 5, 2023 at 1:56 AM Masahiko Sawada <[email protected]> wrote: > > > > > > On Wed, Apr 5, 2023 at 5:05 AM Melanie Plageman > > > <[email protected]> wrote: > > > > > > > > On Tue, Apr 4, 2023 at 4:27 AM Masahiko Sawada <[email protected]> wrote: > > > > > --- > > > > > - if (worker->wi_proc != NULL) > > > > > - elog(DEBUG2, "autovac_balance_cost(pid=%d > > > > > db=%u, rel=%u, dobalance=%s cost_limit=%d, cost_limit_base=%d, > > > > > cost_delay=%g)", > > > > > - worker->wi_proc->pid, > > > > > worker->wi_dboid, worker->wi_tableoid, > > > > > - worker->wi_dobalance ? "yes" : "no", > > > > > - worker->wi_cost_limit, > > > > > worker->wi_cost_limit_base, > > > > > - worker->wi_cost_delay); > > > > > > > > > > I think it's better to keep this kind of log in some form for > > > > > debugging. For example, we can show these values of autovacuum workers > > > > > in VacuumUpdateCosts(). > > > > > > > > I added a message to do_autovacuum() after calling VacuumUpdateCosts() > > > > in the loop vacuuming each table. That means it will happen once per > > > > table. It's not ideal that I had to move the call to VacuumUpdateCosts() > > > > behind the shared lock in that loop so that we could access the pid and > > > > such in the logging message after updating the cost and delay, but it is > > > > probably okay. Though noone is going to be changing those at this > > > > point, it still seemed better to access them under the lock. > > > > > > > > This does mean we won't log anything when we do change the values of > > > > VacuumCostDelay and VacuumCostLimit while vacuuming a table. Is it worth > > > > adding some code to do that in VacuumUpdateCosts() (only when the value > > > > has changed not on every call to VacuumUpdateCosts())? Or perhaps we > > > > could add it in the config reload branch that is already in > > > > vacuum_delay_point()? > > > > > > Previously, we used to show the pid in the log since a worker/launcher > > > set other workers' delay costs. But now that the worker sets its delay > > > costs, we don't need to show the pid in the log. Also, I think it's > > > useful for debugging and investigating the system if we log it when > > > changing the values. The log I imagined to add was like: > > > > > > @@ -1801,6 +1801,13 @@ VacuumUpdateCosts(void) > > > VacuumCostDelay = vacuum_cost_delay; > > > > > > AutoVacuumUpdateLimit(); > > > + > > > + elog(DEBUG2, "autovacuum update costs (db=%u, rel=%u, > > > dobalance=%s, cost_limit=%d, cost_delay=%g active=%s failsafe=%s)", > > > + MyWorkerInfo->wi_dboid, MyWorkerInfo->wi_tableoid, > > > + pg_atomic_unlocked_test_flag(&MyWorkerInfo->wi_dobalance) > > > ? "no" : "yes", > > > + VacuumCostLimit, VacuumCostDelay, > > > + VacuumCostDelay > 0 ? "yes" : "no", > > > + VacuumFailsafeActive ? "yes" : "no"); > > > } > > > else > > > { > > > > Makes sense. I've updated the log message to roughly what you suggested. > > I also realized I think it does make sense to call it in > > VacuumUpdateCosts() -- only for autovacuum workers of course. I've done > > this. I haven't taken the lock though and can't decide if I must since > > they access dboid and tableoid -- those are not going to change at this > > point, but I still don't know if I can access them lock-free... > > Perhaps there is a way to condition it on the log level? > > > > If I have to take a lock, then I don't know if we should put these in > > VacuumUpdateCosts()... > > I think we don't need to acquire a lock there as both values are > updated only by workers reporting this message. I dunno. I just don't feel that comfortable saying, oh it's okay to access these without a lock probably. I propose we do one of the following: - Take a shared lock inside VacuumUpdateCosts() (it is not called on every call to vacuum_delay_point()) before reading from these variables. Pros: - We will log whenever there is a change to these parameters Cons: - This adds overhead in the common case when log level is < DEBUG2. Is there a way to check the log level before taking the lock? - Acquiring the lock inside the function is inconsistent with the pattern that some of the other autovacuum functions requiring locks use (they assume you have a lock if needed inside of the function). But, we could assert that the lock is not already held. - If we later decide we don't like this choice and want to move the logging elsewhere, it will necessarily log less frequently which seems like a harder change to make than logging more frequently. - Move this logging into the loop through relations in do_autovacuum() and the config reload code and take the shared lock before doing the logging. Pros: - Seems safe and not expensive - Covers most of the times we would want the logging Cons: - duplicates logging in two places > Some minor comments on 0003: > > +/* > + * autovac_recalculate_workers_for_balance > + * Recalculate the number of workers to consider, given > cost-related > + * storage parameters and the current number of active workers. > + * > + * Caller must hold the AutovacuumLock in at least shared mode to access > + * worker->wi_proc. > + */ > > Does it make sense to add Assert(LWLockHeldByMe(AutovacuumLock)) at > the beginning of this function? I've added this. It is called infrequently enough to be okay, I think. > /* rebalance in case the default cost parameters changed */ > - LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); > - autovac_balance_cost(); > + LWLockAcquire(AutovacuumLock, LW_SHARED); > + autovac_recalculate_workers_for_balance(); > LWLockRelease(AutovacuumLock); > > Do we really need to have the autovacuum launcher recalculate > av_nworkersForBalance after reloading the config file? Since the cost > parameters are not used inautovac_recalculate_workers_for_balance() > the comment also needs to be updated. Yep, almost certainly don't need this. I've removed this call to autovac_recalculate_workers_for_balance(). > + /* > + * Balance and update limit values for autovacuum > workers. We must > + * always do this in case the autovacuum launcher or another > + * autovacuum worker has recalculated the number of > workers across > + * which we must balance the limit. This is done by > the launcher when > + * launching a new worker and by workers before > vacuuming each table. > + */ > + AutoVacuumUpdateCostLimit(); > > I think the last sentence is not correct. IIUC recalculation of > av_nworkersForBalance is done by the launcher after a worker finished > and by workers before vacuuming each table. Yes, you are right. However, I think the comment was generally misleading and I have reworded it. > It's not a problem of this patch, but IIUC since we don't reset > wi_dobalance after vacuuming each table we use the last value of > wi_dobalance for performing autovacuum items. At end of the loop for > tables in do_autovacuum() we have the following code that explains why > we don't reset wi_dobalance: > > /* > * Remove my info from shared memory. We could, but intentionally > * don't, unset wi_dobalance on the assumption that we are more likely > * than not to vacuum a table with no cost-related storage parameters > * next, so we don't want to give up our share of I/O for a very short > * interval and thereby thrash the global balance. > */ > LWLockAcquire(AutovacuumScheduleLock, LW_EXCLUSIVE); > MyWorkerInfo->wi_tableoid = InvalidOid; > MyWorkerInfo->wi_sharedrel = false; > LWLockRelease(AutovacuumScheduleLock); > > Assuming we agree with that, probably we need to reset it to true > after vacuuming all tables? Ah, great point. I have done this. On Thu, Apr 6, 2023 at 8:29 AM Daniel Gustafsson <[email protected]> wrote: > > > On 6 Apr 2023, at 08:39, Masahiko Sawada <[email protected]> wrote: > > > Also I agree with > > where to put the log but I think the log message should start with > > lower cases: > > > > + elog(DEBUG2, > > + "Autovacuum VacuumUpdateCosts(db=%u, rel=%u, > > In principle I agree, but in this case Autovacuum is a name and should IMO in > userfacing messages start with capital A. I've left this unchanged while I agonize over what to do with the placement of the log message in general. But I am happy to keep it uppercase. > > +/* > > + * autovac_recalculate_workers_for_balance > > + * Recalculate the number of workers to consider, given > > cost-related > > + * storage parameters and the current number of active workers. > > + * > > + * Caller must hold the AutovacuumLock in at least shared mode to access > > + * worker->wi_proc. > > + */ > > > > Does it make sense to add Assert(LWLockHeldByMe(AutovacuumLock)) at > > the beginning of this function? > > That's probably not a bad idea. Done. > > --- > > /* rebalance in case the default cost parameters changed */ > > - LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); > > - autovac_balance_cost(); > > + LWLockAcquire(AutovacuumLock, LW_SHARED); > > + autovac_recalculate_workers_for_balance(); > > LWLockRelease(AutovacuumLock); > > > > Do we really need to have the autovacuum launcher recalculate > > av_nworkersForBalance after reloading the config file? Since the cost > > parameters are not used inautovac_recalculate_workers_for_balance() > > the comment also needs to be updated. > > If I understand this comment right; there was a discussion upthread that simply > doing it in both launcher and worker simplifies the code with little overhead. > A comment can reflect that choice though. Yes, but now that this function no longer deals with the cost limit and delay values itself, we can remove it. - Melanie Attachments: [text/x-patch] v17-0003-Autovacuum-refreshes-cost-based-delay-params-mor.patch (21.7K, ../../CAAKRu_YmQg_i=0DBfPzy1MMc2GjfuJFo=0rFv4fCzRQStLhjvw@mail.gmail.com/2-v17-0003-Autovacuum-refreshes-cost-based-delay-params-mor.patch) download | inline diff: From 684f710af4cb0bf7cd7ff70baa0a8a8fdc13d48c Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Sat, 25 Mar 2023 14:14:55 -0400 Subject: [PATCH v17 3/3] Autovacuum refreshes cost-based delay params more often Allow autovacuum to reload the config file more often so that cost-based delay parameters can take effect while VACUUMing a relation. Previously autovacuum workers only reloaded the config file once per relation vacuumed, so config changes could not take effect until beginning to vacuum the next table. Now, check if a reload is pending roughly once per block, when checking if we need to delay. In order for autovacuum workers to safely update their own cost delay and cost limit parameters without impacting performance, we had to rethink when and how these values were accessed. Previously, an autovacuum worker's wi_cost_limit was set only at the beginning of vacuuming a table, after reloading the config file. Therefore, at the time that autovac_balance_cost() is called, workers vacuuming tables with no cost-related storage parameters could still have different values for their wi_cost_limit_base and wi_cost_delay. Now that the cost parameters can be updated while vacuuming a table, workers will (within some margin of error) have no reason to have different values for cost limit and cost delay (in the absence of cost-related storage parameters). This removes the rationale for keeping cost limit and cost delay in shared memory. Balancing the cost limit requires only the number of active autovacuum workers vacuuming a table with no cost-based storage parameters. Reviewed-by: Masahiko Sawada <[email protected]> Reviewed-by: Daniel Gustafsson <[email protected]> Reviewed-by: Kyotaro Horiguchi <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7%2BDXhrjDA0gw%40mail.gmail.com --- src/backend/access/heap/vacuumlazy.c | 2 +- src/backend/commands/vacuum.c | 45 ++++- src/backend/commands/vacuumparallel.c | 1 - src/backend/postmaster/autovacuum.c | 276 +++++++++++++++----------- src/include/commands/vacuum.h | 1 + 5 files changed, 202 insertions(+), 123 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 2ba85bd3d6..0a9ebd22bd 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -389,7 +389,7 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, Assert(params->index_cleanup != VACOPTVALUE_UNSPECIFIED); Assert(params->truncate != VACOPTVALUE_UNSPECIFIED && params->truncate != VACOPTVALUE_AUTO); - VacuumFailsafeActive = false; + Assert(!VacuumFailsafeActive); vacrel->consider_bypass_optimization = true; vacrel->do_index_vacuuming = true; vacrel->do_index_cleanup = true; diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 5b6f8f5244..977e9c4c7e 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -48,6 +48,7 @@ #include "pgstat.h" #include "postmaster/autovacuum.h" #include "postmaster/bgworker_internals.h" +#include "postmaster/interrupt.h" #include "storage/bufmgr.h" #include "storage/lmgr.h" #include "storage/pmsignal.h" @@ -525,9 +526,9 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy, { ListCell *cur; - VacuumUpdateCosts(); in_vacuum = true; - VacuumCostActive = (VacuumCostDelay > 0); + VacuumFailsafeActive = false; + VacuumUpdateCosts(); VacuumCostBalance = 0; VacuumPageHit = 0; VacuumPageMiss = 0; @@ -581,12 +582,20 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy, CommandCounterIncrement(); } } + + /* + * Ensure VacuumFailsafeActive has been reset before vacuuming the + * next relation. + */ + VacuumFailsafeActive = false; } } PG_FINALLY(); { in_vacuum = false; VacuumCostActive = false; + VacuumFailsafeActive = false; + VacuumCostBalance = 0; } PG_END_TRY(); @@ -2247,7 +2256,27 @@ vacuum_delay_point(void) /* Always check for interrupts */ CHECK_FOR_INTERRUPTS(); - if (!VacuumCostActive || InterruptPending) + if (InterruptPending || + (!VacuumCostActive && !ConfigReloadPending)) + return; + + /* + * Reload the configuration file if requested. This allows changes to + * autovacuum_vacuum_cost_limit and autovacuum_vacuum_cost_delay to take + * effect while a table is being vacuumed or analyzed. + */ + if (ConfigReloadPending && IsAutoVacuumWorkerProcess()) + { + ConfigReloadPending = false; + ProcessConfigFile(PGC_SIGHUP); + VacuumUpdateCosts(); + } + + /* + * If we disabled cost-based delays after reloading the config file, + * return. + */ + if (!VacuumCostActive) return; /* @@ -2280,7 +2309,15 @@ vacuum_delay_point(void) VacuumCostBalance = 0; - VacuumUpdateCosts(); + /* + * Balance and update limit values for autovacuum workers. We must do + * this periodically as the number of workers across which we are + * balancing the limit may have changed. + * + * XXX: There may be better criteria for determining when to do this + * besides "check after napping". + */ + AutoVacuumUpdateCostLimit(); /* Might have gotten an interrupt while sleeping */ CHECK_FOR_INTERRUPTS(); diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c index 0b59c922e4..e200d5caf8 100644 --- a/src/backend/commands/vacuumparallel.c +++ b/src/backend/commands/vacuumparallel.c @@ -995,7 +995,6 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc) false); /* Set cost-based vacuum delay */ - VacuumCostActive = (VacuumCostDelay > 0); VacuumUpdateCosts(); VacuumCostBalance = 0; VacuumPageHit = 0; diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index 296b1851e3..f7237fa5ea 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -139,6 +139,18 @@ int Log_autovacuum_min_duration = 600000; static bool am_autovacuum_launcher = false; static bool am_autovacuum_worker = false; +/* + * Variables to save the cost-related storage parameters for the current + * relation being vacuumed by this autovacuum worker. Using these, we can + * ensure we don't overwrite the values of VacuumCostDelay and VacuumCostLimit + * after reloading the configuration file. They are initialized to "invalid" + * values to indicate no cost-related storage parameters were specified and + * will be set in do_autovacuum() after checking the storage parameters in + * table_recheck_autovac(). + */ +static double av_storage_param_cost_delay = -1; +static int av_storage_param_cost_limit = -1; + /* Flags set by signal handlers */ static volatile sig_atomic_t got_SIGUSR2 = false; @@ -189,8 +201,8 @@ typedef struct autovac_table { Oid at_relid; VacuumParams at_params; - double at_vacuum_cost_delay; - int at_vacuum_cost_limit; + double at_storage_param_vac_cost_delay; + int at_storage_param_vac_cost_limit; bool at_dobalance; bool at_sharedrel; char *at_relname; @@ -209,7 +221,7 @@ typedef struct autovac_table * wi_sharedrel flag indicating whether table is marked relisshared * wi_proc pointer to PGPROC of the running worker, NULL if not started * wi_launchtime Time at which this worker was launched - * wi_cost_* Vacuum cost-based delay parameters current in this worker + * wi_dobalance Whether this worker should be included in balance calculations * * All fields are protected by AutovacuumLock, except for wi_tableoid and * wi_sharedrel which are protected by AutovacuumScheduleLock (note these @@ -223,11 +235,8 @@ typedef struct WorkerInfoData Oid wi_tableoid; PGPROC *wi_proc; TimestampTz wi_launchtime; - bool wi_dobalance; + pg_atomic_flag wi_dobalance; bool wi_sharedrel; - double wi_cost_delay; - int wi_cost_limit; - int wi_cost_limit_base; } WorkerInfoData; typedef struct WorkerInfoData *WorkerInfo; @@ -273,6 +282,8 @@ typedef struct AutoVacuumWorkItem * av_startingWorker pointer to WorkerInfo currently being started (cleared by * the worker itself as soon as it's up and running) * av_workItems work item array + * av_nworkersForBalance the number of autovacuum workers to use when + * calculating the per worker cost limit * * This struct is protected by AutovacuumLock, except for av_signal and parts * of the worker list (see above). @@ -286,6 +297,7 @@ typedef struct dlist_head av_runningWorkers; WorkerInfo av_startingWorker; AutoVacuumWorkItem av_workItems[NUM_WORKITEMS]; + pg_atomic_uint32 av_nworkersForBalance; } AutoVacuumShmemStruct; static AutoVacuumShmemStruct *AutoVacuumShmem; @@ -319,7 +331,7 @@ static void launch_worker(TimestampTz now); static List *get_database_list(void); static void rebuild_database_list(Oid newdb); static int db_comparator(const void *a, const void *b); -static void autovac_balance_cost(void); +static void autovac_recalculate_workers_for_balance(void); static void do_autovacuum(void); static void FreeWorkerInfo(int code, Datum arg); @@ -669,7 +681,7 @@ AutoVacLauncherMain(int argc, char *argv[]) { LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); AutoVacuumShmem->av_signal[AutoVacRebalance] = false; - autovac_balance_cost(); + autovac_recalculate_workers_for_balance(); LWLockRelease(AutovacuumLock); } @@ -818,11 +830,6 @@ HandleAutoVacLauncherInterrupts(void) if (!AutoVacuumingActive()) AutoVacLauncherShutdown(); - /* rebalance in case the default cost parameters changed */ - LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); - autovac_balance_cost(); - LWLockRelease(AutovacuumLock); - /* rebuild the list in case the naptime changed */ rebuild_database_list(InvalidOid); } @@ -1754,10 +1761,7 @@ FreeWorkerInfo(int code, Datum arg) MyWorkerInfo->wi_sharedrel = false; MyWorkerInfo->wi_proc = NULL; MyWorkerInfo->wi_launchtime = 0; - MyWorkerInfo->wi_dobalance = false; - MyWorkerInfo->wi_cost_delay = 0; - MyWorkerInfo->wi_cost_limit = 0; - MyWorkerInfo->wi_cost_limit_base = 0; + pg_atomic_clear_flag(&MyWorkerInfo->wi_dobalance); dlist_push_head(&AutoVacuumShmem->av_freeWorkers, &MyWorkerInfo->wi_links); /* not mine anymore */ @@ -1783,97 +1787,128 @@ VacuumUpdateCosts(void) { if (MyWorkerInfo) { - VacuumCostDelay = MyWorkerInfo->wi_cost_delay; - VacuumCostLimit = MyWorkerInfo->wi_cost_limit; + if (av_storage_param_cost_delay >= 0) + VacuumCostDelay = av_storage_param_cost_delay; + else if (autovacuum_vac_cost_delay >= 0) + VacuumCostDelay = autovacuum_vac_cost_delay; + else + /* fall back to vacuum_cost_delay */ + VacuumCostDelay = vacuum_cost_delay; + + AutoVacuumUpdateCostLimit(); } else { /* Must be explicit VACUUM or ANALYZE */ - VacuumCostLimit = vacuum_cost_limit; VacuumCostDelay = vacuum_cost_delay; + VacuumCostLimit = vacuum_cost_limit; + } + + /* + * If configuration changes are allowed to impact VacuumCostActive, make + * sure it is updated. + */ + if (VacuumFailsafeActive) + Assert(!VacuumCostActive); + else if (VacuumCostDelay > 0) + VacuumCostActive = true; + else + { + VacuumCostActive = false; + VacuumCostBalance = 0; + } + + if (MyWorkerInfo) + { + elog(DEBUG2, + "Autovacuum VacuumUpdateCosts(db=%u, rel=%u, dobalance=%s, cost_limit=%d, cost_delay=%g active=%s failsafe=%s)", + MyWorkerInfo->wi_dboid, MyWorkerInfo->wi_tableoid, + pg_atomic_unlocked_test_flag(&MyWorkerInfo->wi_dobalance) ? "no" : "yes", + VacuumCostLimit, VacuumCostDelay, + VacuumCostDelay > 0 ? "yes" : "no", + VacuumFailsafeActive ? "yes" : "no"); } } /* - * autovac_balance_cost - * Recalculate the cost limit setting for each active worker. - * - * Caller must hold the AutovacuumLock in exclusive mode. + * Update VacuumCostLimit with the correct value for an autovacuum worker, given + * the value of other relevant cost limit parameters and the number of workers + * across which the limit must be balanced. Autovacuum workers must call this + * regularly in case av_nworkers_for_balance has been updated by another worker + * or by the autovacuum launcher. They must also call it after a config reload. */ -static void -autovac_balance_cost(void) +void +AutoVacuumUpdateCostLimit(void) { + if (!MyWorkerInfo) + return; + /* - * The idea here is that we ration out I/O equally. The amount of I/O - * that a worker can consume is determined by cost_limit/cost_delay, so we - * try to equalize those ratios rather than the raw limit settings. - * * note: in cost_limit, zero also means use value from elsewhere, because * zero is not a valid value. */ - int vac_cost_limit = (autovacuum_vac_cost_limit > 0 ? - autovacuum_vac_cost_limit : vacuum_cost_limit); - double vac_cost_delay = (autovacuum_vac_cost_delay >= 0 ? - autovacuum_vac_cost_delay : vacuum_cost_delay); - double cost_total; - double cost_avail; - dlist_iter iter; - /* not set? nothing to do */ - if (vac_cost_limit <= 0 || vac_cost_delay <= 0) - return; - - /* calculate the total base cost limit of participating active workers */ - cost_total = 0.0; - dlist_foreach(iter, &AutoVacuumShmem->av_runningWorkers) + if (av_storage_param_cost_limit > 0) + VacuumCostLimit = av_storage_param_cost_limit; + else { - WorkerInfo worker = dlist_container(WorkerInfoData, wi_links, iter.cur); + int nworkers_for_balance; + + if (autovacuum_vac_cost_limit > 0) + VacuumCostLimit = autovacuum_vac_cost_limit; + else + VacuumCostLimit = vacuum_cost_limit; + + /* Only balance limit if no cost-related storage parameters specified */ + if (pg_atomic_unlocked_test_flag(&MyWorkerInfo->wi_dobalance)) + return; - if (worker->wi_proc != NULL && - worker->wi_dobalance && - worker->wi_cost_limit_base > 0 && worker->wi_cost_delay > 0) - cost_total += - (double) worker->wi_cost_limit_base / worker->wi_cost_delay; + Assert(VacuumCostLimit > 0); + + nworkers_for_balance = pg_atomic_read_u32(&AutoVacuumShmem->av_nworkersForBalance); + + /* There is at least 1 autovac worker (this worker) */ + if (nworkers_for_balance <= 0) + elog(ERROR, "nworkers_for_balance must be > 0"); + + VacuumCostLimit = Max(VacuumCostLimit / nworkers_for_balance, 1); } +} - /* there are no cost limits -- nothing to do */ - if (cost_total <= 0) - return; +/* + * autovac_recalculate_workers_for_balance + * Recalculate the number of workers to consider, given cost-related + * storage parameters and the current number of active workers. + * + * Caller must hold the AutovacuumLock in at least shared mode to access + * worker->wi_proc. + */ +static void +autovac_recalculate_workers_for_balance(void) +{ + dlist_iter iter; + int orig_nworkers_for_balance; + int nworkers_for_balance = 0; + + Assert(LWLockHeldByMe(AutovacuumLock)); + + orig_nworkers_for_balance = + pg_atomic_read_u32(&AutoVacuumShmem->av_nworkersForBalance); - /* - * Adjust cost limit of each active worker to balance the total of cost - * limit to autovacuum_vacuum_cost_limit. - */ - cost_avail = (double) vac_cost_limit / vac_cost_delay; dlist_foreach(iter, &AutoVacuumShmem->av_runningWorkers) { WorkerInfo worker = dlist_container(WorkerInfoData, wi_links, iter.cur); - if (worker->wi_proc != NULL && - worker->wi_dobalance && - worker->wi_cost_limit_base > 0 && worker->wi_cost_delay > 0) - { - int limit = (int) - (cost_avail * worker->wi_cost_limit_base / cost_total); - - /* - * We put a lower bound of 1 on the cost_limit, to avoid division- - * by-zero in the vacuum code. Also, in case of roundoff trouble - * in these calculations, let's be sure we don't ever set - * cost_limit to more than the base value. - */ - worker->wi_cost_limit = Max(Min(limit, - worker->wi_cost_limit_base), - 1); - } + if (worker->wi_proc == NULL || + pg_atomic_unlocked_test_flag(&worker->wi_dobalance)) + continue; - if (worker->wi_proc != NULL) - elog(DEBUG2, "autovac_balance_cost(pid=%d db=%u, rel=%u, dobalance=%s cost_limit=%d, cost_limit_base=%d, cost_delay=%g)", - worker->wi_proc->pid, worker->wi_dboid, worker->wi_tableoid, - worker->wi_dobalance ? "yes" : "no", - worker->wi_cost_limit, worker->wi_cost_limit_base, - worker->wi_cost_delay); + nworkers_for_balance++; } + + if (nworkers_for_balance != orig_nworkers_for_balance) + pg_atomic_write_u32(&AutoVacuumShmem->av_nworkersForBalance, + nworkers_for_balance); } /* @@ -2421,23 +2456,34 @@ do_autovacuum(void) continue; } - /* Must hold AutovacuumLock while mucking with cost balance info */ - LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); + /* + * Save the cost-related storage parameter values in global variables + * for reference when updating VacuumCostLimit and VacuumCostDelay + * during vacuuming this table. + */ + av_storage_param_cost_limit = tab->at_storage_param_vac_cost_limit; + av_storage_param_cost_delay = tab->at_storage_param_vac_cost_delay; - /* advertise my cost delay parameters for the balancing algorithm */ - MyWorkerInfo->wi_dobalance = tab->at_dobalance; - MyWorkerInfo->wi_cost_delay = tab->at_vacuum_cost_delay; - MyWorkerInfo->wi_cost_limit = tab->at_vacuum_cost_limit; - MyWorkerInfo->wi_cost_limit_base = tab->at_vacuum_cost_limit; + /* + * We only expect this worker to ever set the flag, so don't bother + * checking the return value. We shouldn't have to retry. + */ + if (tab->at_dobalance) + pg_atomic_test_set_flag(&MyWorkerInfo->wi_dobalance); + else + pg_atomic_clear_flag(&MyWorkerInfo->wi_dobalance); - /* do a balance */ - autovac_balance_cost(); + LWLockAcquire(AutovacuumLock, LW_SHARED); + autovac_recalculate_workers_for_balance(); + LWLockRelease(AutovacuumLock); - /* set the active cost parameters from the result of that */ + /* + * We wait until this point to update cost delay and cost limit + * values, even though we reloaded the configuration file above, so + * that we can take into account the cost-related storage parameters. + */ VacuumUpdateCosts(); - /* done */ - LWLockRelease(AutovacuumLock); /* clean up memory before each iteration */ MemoryContextResetAndDeleteChildren(PortalContext); @@ -2521,16 +2567,17 @@ deleted: pfree(tab); /* - * Remove my info from shared memory. We could, but intentionally - * don't, clear wi_cost_limit and friends --- this is on the - * assumption that we probably have more to do with similar cost - * settings, so we don't want to give up our share of I/O for a very - * short interval and thereby thrash the global balance. + * Remove my info from shared memory. We set wi_dobalance on the + * assumption that we are more likely than not to vacuum a table with + * no cost-related storage parameters next, so we want to claim our + * share of I/O as soon as possible to avoid thrashing the global + * balance. */ LWLockAcquire(AutovacuumScheduleLock, LW_EXCLUSIVE); MyWorkerInfo->wi_tableoid = InvalidOid; MyWorkerInfo->wi_sharedrel = false; LWLockRelease(AutovacuumScheduleLock); + pg_atomic_test_set_flag(&MyWorkerInfo->wi_dobalance); } /* @@ -2562,6 +2609,7 @@ deleted: { ConfigReloadPending = false; ProcessConfigFile(PGC_SIGHUP); + VacuumUpdateCosts(); } LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); @@ -2797,8 +2845,6 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map, int freeze_table_age; int multixact_freeze_min_age; int multixact_freeze_table_age; - int vac_cost_limit; - double vac_cost_delay; int log_min_duration; /* @@ -2808,20 +2854,6 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map, * defaults, autovacuum's own first and plain vacuum second. */ - /* -1 in autovac setting means use plain vacuum_cost_delay */ - vac_cost_delay = (avopts && avopts->vacuum_cost_delay >= 0) - ? avopts->vacuum_cost_delay - : (autovacuum_vac_cost_delay >= 0) - ? autovacuum_vac_cost_delay - : vacuum_cost_delay; - - /* 0 or -1 in autovac setting means use plain vacuum_cost_limit */ - vac_cost_limit = (avopts && avopts->vacuum_cost_limit > 0) - ? avopts->vacuum_cost_limit - : (autovacuum_vac_cost_limit > 0) - ? autovacuum_vac_cost_limit - : vacuum_cost_limit; - /* -1 in autovac setting means use log_autovacuum_min_duration */ log_min_duration = (avopts && avopts->log_min_duration >= 0) ? avopts->log_min_duration @@ -2877,8 +2909,10 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map, tab->at_params.multixact_freeze_table_age = multixact_freeze_table_age; tab->at_params.is_wraparound = wraparound; tab->at_params.log_min_duration = log_min_duration; - tab->at_vacuum_cost_limit = vac_cost_limit; - tab->at_vacuum_cost_delay = vac_cost_delay; + tab->at_storage_param_vac_cost_limit = avopts ? + avopts->vacuum_cost_limit : 0; + tab->at_storage_param_vac_cost_delay = avopts ? + avopts->vacuum_cost_delay : -1; tab->at_relname = NULL; tab->at_nspname = NULL; tab->at_datname = NULL; @@ -3380,10 +3414,18 @@ AutoVacuumShmemInit(void) worker = (WorkerInfo) ((char *) AutoVacuumShmem + MAXALIGN(sizeof(AutoVacuumShmemStruct))); + /* initialize the WorkerInfo free list */ for (i = 0; i < autovacuum_max_workers; i++) + { dlist_push_head(&AutoVacuumShmem->av_freeWorkers, &worker[i].wi_links); + + pg_atomic_init_flag(&worker[i].wi_dobalance); + } + + pg_atomic_init_u32(&AutoVacuumShmem->av_nworkersForBalance, 0); + } else Assert(found); diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h index 440ddd2154..e9705ba51d 100644 --- a/src/include/commands/vacuum.h +++ b/src/include/commands/vacuum.h @@ -352,6 +352,7 @@ extern IndexBulkDeleteResult *vac_cleanup_one_index(IndexVacuumInfo *ivinfo, extern Size vac_max_items_to_alloc_size(int max_items); /* In postmaster/autovacuum.c */ +extern void AutoVacuumUpdateCostLimit(void); extern void VacuumUpdateCosts(void); /* in commands/vacuumparallel.c */ -- 2.37.2 [text/x-patch] v17-0001-Make-vacuum-s-failsafe_active-a-global.patch (5.3K, ../../CAAKRu_YmQg_i=0DBfPzy1MMc2GjfuJFo=0rFv4fCzRQStLhjvw@mail.gmail.com/3-v17-0001-Make-vacuum-s-failsafe_active-a-global.patch) download | inline diff: From 5b31ff0af5813d2f7747eaabbbf5e6f4c8284c4a Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Fri, 31 Mar 2023 10:38:39 -0400 Subject: [PATCH v17 1/3] Make vacuum's failsafe_active a global While vacuuming a table in failsafe mode, VacuumCostActive should not be re-enabled. This currently isn't a problem because vacuum cost parameters are only refreshed in between vacuuming tables and failsafe status is reset for every table. In preparation for allowing vacuum cost parameters to be updated more frequently, elevate LVRelState->failsafe_active to a global, VacuumFailsafeActive, which will be checked when determining whether or not to re-enable vacuum cost-related delays. Reviewed-by: Masahiko Sawada <[email protected]> Reviewed-by: Daniel Gustafsson <[email protected]> Reviewed-by: Kyotaro Horiguchi <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7%2BDXhrjDA0gw%40mail.gmail.com --- src/backend/access/heap/vacuumlazy.c | 16 +++++++--------- src/backend/commands/vacuum.c | 15 +++++++++++++++ src/include/commands/vacuum.h | 1 + 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 639179aa46..2ba85bd3d6 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -153,8 +153,6 @@ typedef struct LVRelState bool aggressive; /* Use visibility map to skip? (disabled by DISABLE_PAGE_SKIPPING) */ bool skipwithvm; - /* Wraparound failsafe has been triggered? */ - bool failsafe_active; /* Consider index vacuuming bypass optimization? */ bool consider_bypass_optimization; @@ -391,7 +389,7 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, Assert(params->index_cleanup != VACOPTVALUE_UNSPECIFIED); Assert(params->truncate != VACOPTVALUE_UNSPECIFIED && params->truncate != VACOPTVALUE_AUTO); - vacrel->failsafe_active = false; + VacuumFailsafeActive = false; vacrel->consider_bypass_optimization = true; vacrel->do_index_vacuuming = true; vacrel->do_index_cleanup = true; @@ -709,7 +707,7 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, } else { - if (!vacrel->failsafe_active) + if (!VacuumFailsafeActive) appendStringInfoString(&buf, _("index scan bypassed: ")); else appendStringInfoString(&buf, _("index scan bypassed by failsafe: ")); @@ -2293,7 +2291,7 @@ lazy_vacuum(LVRelState *vacrel) * vacuuming or heap vacuuming. This VACUUM operation won't end up * back here again. */ - Assert(vacrel->failsafe_active); + Assert(VacuumFailsafeActive); } /* @@ -2374,7 +2372,7 @@ lazy_vacuum_all_indexes(LVRelState *vacrel) */ Assert(vacrel->num_index_scans > 0 || vacrel->dead_items->num_items == vacrel->lpdead_items); - Assert(allindexes || vacrel->failsafe_active); + Assert(allindexes || VacuumFailsafeActive); /* * Increase and report the number of index scans. @@ -2616,12 +2614,12 @@ static bool lazy_check_wraparound_failsafe(LVRelState *vacrel) { /* Don't warn more than once per VACUUM */ - if (vacrel->failsafe_active) + if (VacuumFailsafeActive) return true; if (unlikely(vacuum_xid_failsafe_check(&vacrel->cutoffs))) { - vacrel->failsafe_active = true; + VacuumFailsafeActive = true; /* * Abandon use of a buffer access strategy to allow use of all of @@ -2820,7 +2818,7 @@ should_attempt_truncation(LVRelState *vacrel) { BlockNumber possibly_freeable; - if (!vacrel->do_rel_truncate || vacrel->failsafe_active || + if (!vacrel->do_rel_truncate || VacuumFailsafeActive || old_snapshot_threshold >= 0) return false; diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index ea1d8960f4..7fc5c19e37 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -72,6 +72,21 @@ int vacuum_multixact_freeze_table_age; int vacuum_failsafe_age; int vacuum_multixact_failsafe_age; +/* + * VacuumFailsafeActive is a defined as a global so that we can determine + * whether or not to re-enable cost-based vacuum delay when vacuuming a table. + * If failsafe mode has been engaged, we will not re-enable cost-based delay + * for the table until after vacuuming has completed, regardless of other + * settings. + * + * Only VACUUM code should inspect this variable and only table access methods + * should set it to true. In Table AM-agnostic VACUUM code, this variable is + * inspected to determine whether or not to allow cost-based delays. Table AMs + * are free to set it if they desire this behavior, but it is false by default + * and reset to false in between vacuuming each relation. + */ +bool VacuumFailsafeActive = false; + /* * Variables for cost-based parallel vacuum. See comments atop * compute_parallel_delay to understand how it works. diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h index 19ca818dc2..1223d15e0d 100644 --- a/src/include/commands/vacuum.h +++ b/src/include/commands/vacuum.h @@ -306,6 +306,7 @@ extern PGDLLIMPORT pg_atomic_uint32 *VacuumSharedCostBalance; extern PGDLLIMPORT pg_atomic_uint32 *VacuumActiveNWorkers; extern PGDLLIMPORT int VacuumCostBalanceLocal; +extern PGDLLIMPORT bool VacuumFailsafeActive; /* in commands/vacuum.c */ extern void ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel); -- 2.37.2 [text/x-patch] v17-0002-Separate-vacuum-cost-variables-from-gucs.patch (10.4K, ../../CAAKRu_YmQg_i=0DBfPzy1MMc2GjfuJFo=0rFv4fCzRQStLhjvw@mail.gmail.com/4-v17-0002-Separate-vacuum-cost-variables-from-gucs.patch) download | inline diff: From c456e0d6949e7422168a3eb5b2dc268b7a243833 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Mon, 3 Apr 2023 11:22:18 -0400 Subject: [PATCH v17 2/3] Separate vacuum cost variables from gucs Vacuum code run both by autovacuum workers and a backend doing VACUUM/ANALYZE previously used VacuumCostLimit and VacuumCostDelay which were the global variables for the gucs vacuum_cost_limit and vacuum_cost_delay. Autovacuum workers needed to override these variables with their own values, derived from autovacuum_vacuum_cost_limit and autovacuum_vacuum_cost_delay and worker cost limit balancing logic. This led to confusing code which, in some cases, both derived and set a new value of VacuumCostLimit from VacuumCostLimit. In preparation for refreshing these guc values more often, separate these variables from the gucs themselves and add a function to update the global variables using the gucs and existing logic. Per suggestion by Kyotaro Horiguchi Reviewed-by: Masahiko Sawada <[email protected]> Reviewed-by: Daniel Gustafsson <[email protected]> Reviewed-by: Kyotaro Horiguchi <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7%2BDXhrjDA0gw%40mail.gmail.com --- src/backend/commands/vacuum.c | 15 +++++++++-- src/backend/commands/vacuumparallel.c | 1 + src/backend/postmaster/autovacuum.c | 38 +++++++++++---------------- src/backend/utils/init/globals.c | 2 -- src/backend/utils/misc/guc_tables.c | 4 +-- src/include/commands/vacuum.h | 7 +++++ src/include/miscadmin.h | 2 -- src/include/postmaster/autovacuum.h | 3 --- 8 files changed, 39 insertions(+), 33 deletions(-) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 7fc5c19e37..5b6f8f5244 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -71,6 +71,17 @@ int vacuum_multixact_freeze_min_age; int vacuum_multixact_freeze_table_age; int vacuum_failsafe_age; int vacuum_multixact_failsafe_age; +double vacuum_cost_delay; +int vacuum_cost_limit; + +/* + * Variables for cost-based vacuum delay. The defaults differ between + * autovacuum and vacuum. These should be overridden with the appropriate GUC + * value in vacuum code. These are initialized here to the defaults for client + * backends executing VACUUM or ANALYZE. + */ +int VacuumCostLimit = 200; +double VacuumCostDelay = 0; /* * VacuumFailsafeActive is a defined as a global so that we can determine @@ -514,6 +525,7 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy, { ListCell *cur; + VacuumUpdateCosts(); in_vacuum = true; VacuumCostActive = (VacuumCostDelay > 0); VacuumCostBalance = 0; @@ -2268,8 +2280,7 @@ vacuum_delay_point(void) VacuumCostBalance = 0; - /* update balance values for workers */ - AutoVacuumUpdateDelay(); + VacuumUpdateCosts(); /* Might have gotten an interrupt while sleeping */ CHECK_FOR_INTERRUPTS(); diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c index 563117a8f6..0b59c922e4 100644 --- a/src/backend/commands/vacuumparallel.c +++ b/src/backend/commands/vacuumparallel.c @@ -996,6 +996,7 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc) /* Set cost-based vacuum delay */ VacuumCostActive = (VacuumCostDelay > 0); + VacuumUpdateCosts(); VacuumCostBalance = 0; VacuumPageHit = 0; VacuumPageMiss = 0; diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index c1e911b1b3..296b1851e3 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -1773,17 +1773,25 @@ FreeWorkerInfo(int code, Datum arg) } /* - * Update the cost-based delay parameters, so that multiple workers consume - * each a fraction of the total available I/O. + * Update vacuum cost-based delay-related parameters for autovacuum workers and + * backends executing VACUUM or ANALYZE using the value of relevant gucs and + * global state. This must be called during setup for vacuum and after every + * config reload to ensure up-to-date values. */ void -AutoVacuumUpdateDelay(void) +VacuumUpdateCosts(void) { if (MyWorkerInfo) { VacuumCostDelay = MyWorkerInfo->wi_cost_delay; VacuumCostLimit = MyWorkerInfo->wi_cost_limit; } + else + { + /* Must be explicit VACUUM or ANALYZE */ + VacuumCostLimit = vacuum_cost_limit; + VacuumCostDelay = vacuum_cost_delay; + } } /* @@ -1804,9 +1812,9 @@ autovac_balance_cost(void) * zero is not a valid value. */ int vac_cost_limit = (autovacuum_vac_cost_limit > 0 ? - autovacuum_vac_cost_limit : VacuumCostLimit); + autovacuum_vac_cost_limit : vacuum_cost_limit); double vac_cost_delay = (autovacuum_vac_cost_delay >= 0 ? - autovacuum_vac_cost_delay : VacuumCostDelay); + autovacuum_vac_cost_delay : vacuum_cost_delay); double cost_total; double cost_avail; dlist_iter iter; @@ -2311,8 +2319,6 @@ do_autovacuum(void) autovac_table *tab; bool isshared; bool skipit; - double stdVacuumCostDelay; - int stdVacuumCostLimit; dlist_iter iter; CHECK_FOR_INTERRUPTS(); @@ -2415,14 +2421,6 @@ do_autovacuum(void) continue; } - /* - * Remember the prevailing values of the vacuum cost GUCs. We have to - * restore these at the bottom of the loop, else we'll compute wrong - * values in the next iteration of autovac_balance_cost(). - */ - stdVacuumCostDelay = VacuumCostDelay; - stdVacuumCostLimit = VacuumCostLimit; - /* Must hold AutovacuumLock while mucking with cost balance info */ LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); @@ -2436,7 +2434,7 @@ do_autovacuum(void) autovac_balance_cost(); /* set the active cost parameters from the result of that */ - AutoVacuumUpdateDelay(); + VacuumUpdateCosts(); /* done */ LWLockRelease(AutovacuumLock); @@ -2533,10 +2531,6 @@ deleted: MyWorkerInfo->wi_tableoid = InvalidOid; MyWorkerInfo->wi_sharedrel = false; LWLockRelease(AutovacuumScheduleLock); - - /* restore vacuum cost GUCs for the next iteration */ - VacuumCostDelay = stdVacuumCostDelay; - VacuumCostLimit = stdVacuumCostLimit; } /* @@ -2819,14 +2813,14 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map, ? avopts->vacuum_cost_delay : (autovacuum_vac_cost_delay >= 0) ? autovacuum_vac_cost_delay - : VacuumCostDelay; + : vacuum_cost_delay; /* 0 or -1 in autovac setting means use plain vacuum_cost_limit */ vac_cost_limit = (avopts && avopts->vacuum_cost_limit > 0) ? avopts->vacuum_cost_limit : (autovacuum_vac_cost_limit > 0) ? autovacuum_vac_cost_limit - : VacuumCostLimit; + : vacuum_cost_limit; /* -1 in autovac setting means use log_autovacuum_min_duration */ log_min_duration = (avopts && avopts->log_min_duration >= 0) diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c index 1b1d814254..8e5b065e8f 100644 --- a/src/backend/utils/init/globals.c +++ b/src/backend/utils/init/globals.c @@ -142,8 +142,6 @@ int MaxBackends = 0; int VacuumCostPageHit = 1; /* GUC parameters for vacuum */ int VacuumCostPageMiss = 2; int VacuumCostPageDirty = 20; -int VacuumCostLimit = 200; -double VacuumCostDelay = 0; int64 VacuumPageHit = 0; int64 VacuumPageMiss = 0; diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index 8062589efd..77db1a146c 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -2409,7 +2409,7 @@ struct config_int ConfigureNamesInt[] = gettext_noop("Vacuum cost amount available before napping."), NULL }, - &VacuumCostLimit, + &vacuum_cost_limit, 200, 1, 10000, NULL, NULL, NULL }, @@ -3701,7 +3701,7 @@ struct config_real ConfigureNamesReal[] = NULL, GUC_UNIT_MS }, - &VacuumCostDelay, + &vacuum_cost_delay, 0, 0, 100, NULL, NULL, NULL }, diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h index 1223d15e0d..440ddd2154 100644 --- a/src/include/commands/vacuum.h +++ b/src/include/commands/vacuum.h @@ -300,6 +300,8 @@ extern PGDLLIMPORT int vacuum_multixact_freeze_min_age; extern PGDLLIMPORT int vacuum_multixact_freeze_table_age; extern PGDLLIMPORT int vacuum_failsafe_age; extern PGDLLIMPORT int vacuum_multixact_failsafe_age; +extern PGDLLIMPORT double vacuum_cost_delay; +extern PGDLLIMPORT int vacuum_cost_limit; /* Variables for cost-based parallel vacuum */ extern PGDLLIMPORT pg_atomic_uint32 *VacuumSharedCostBalance; @@ -307,6 +309,8 @@ extern PGDLLIMPORT pg_atomic_uint32 *VacuumActiveNWorkers; extern PGDLLIMPORT int VacuumCostBalanceLocal; extern PGDLLIMPORT bool VacuumFailsafeActive; +extern PGDLLIMPORT int VacuumCostLimit; +extern PGDLLIMPORT double VacuumCostDelay; /* in commands/vacuum.c */ extern void ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel); @@ -347,6 +351,9 @@ extern IndexBulkDeleteResult *vac_cleanup_one_index(IndexVacuumInfo *ivinfo, IndexBulkDeleteResult *istat); extern Size vac_max_items_to_alloc_size(int max_items); +/* In postmaster/autovacuum.c */ +extern void VacuumUpdateCosts(void); + /* in commands/vacuumparallel.c */ extern ParallelVacuumState *parallel_vacuum_init(Relation rel, Relation *indrels, int nindexes, int nrequested_workers, diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 06a86f9ac1..66db1b2c69 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -266,8 +266,6 @@ extern PGDLLIMPORT int max_parallel_maintenance_workers; extern PGDLLIMPORT int VacuumCostPageHit; extern PGDLLIMPORT int VacuumCostPageMiss; extern PGDLLIMPORT int VacuumCostPageDirty; -extern PGDLLIMPORT int VacuumCostLimit; -extern PGDLLIMPORT double VacuumCostDelay; extern PGDLLIMPORT int64 VacuumPageHit; extern PGDLLIMPORT int64 VacuumPageMiss; diff --git a/src/include/postmaster/autovacuum.h b/src/include/postmaster/autovacuum.h index c140371b51..65afd1ea1e 100644 --- a/src/include/postmaster/autovacuum.h +++ b/src/include/postmaster/autovacuum.h @@ -63,9 +63,6 @@ extern int StartAutoVacWorker(void); /* called from postmaster when a worker could not be forked */ extern void AutoVacWorkerFailed(void); -/* autovacuum cost-delay balancer */ -extern void AutoVacuumUpdateDelay(void); - #ifdef EXEC_BACKEND extern void AutoVacLauncherMain(int argc, char *argv[]) pg_attribute_noreturn(); extern void AutoVacWorkerMain(int argc, char *argv[]) pg_attribute_noreturn(); -- 2.37.2 ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-04-06 21:06 Melanie Plageman <[email protected]> parent: Melanie Plageman <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Melanie Plageman @ 2023-04-06 21:06 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Robert Haas <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Masahiko Sawada <[email protected]>; Kyotaro Horiguchi <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> I think attached v18 addresses all outstanding issues except a run through the docs making sure all mentions of the balancing algorithm are still correct. On Wed, Apr 5, 2023 at 9:10 AM Daniel Gustafsson <[email protected]> wrote: > > On 4 Apr 2023, at 22:04, Melanie Plageman <[email protected]> wrote: > >> +extern int VacuumCostLimit; > >> +extern double VacuumCostDelay; > >> ... > >> -extern PGDLLIMPORT int VacuumCostLimit; > >> -extern PGDLLIMPORT double VacuumCostDelay; > >> > >> Same with these, I don't think this is according to our default visibility. > >> Moreover, I'm not sure it's a good idea to perform this rename. This will keep > >> VacuumCostLimit and VacuumCostDelay exported, but change their meaning. Any > >> external code referring to these thinking they are backing the GUCs will still > >> compile, but may be broken in subtle ways. Is there a reason for not keeping > >> the current GUC variables and instead add net new ones? > > > > When VacuumCostLimit was the same variable in the code and for the GUC > > vacuum_cost_limit, everytime we reload the config file, VacuumCostLimit > > is overwritten. Autovacuum workers have to overwrite this value with the > > appropriate one for themselves given the balancing logic and the value > > of autovacuum_vacuum_cost_limit. However, the problem is, because you > > can specify -1 for autovacuum_vacuum_cost_limit to indicate it should > > fall back to vacuum_cost_limit, we have to reference the value of > > VacuumCostLimit when calculating the new autovacuum worker's cost limit > > after a config reload. > > > > But, you have to be sure you *only* do this after a config reload when > > the value of VacuumCostLimit is fresh and unmodified or you risk > > dividing the value of VacuumCostLimit over and over. That means it is > > unsafe to call functions updating the cost limit more than once. > > > > This orchestration wasn't as difficult when we only reloaded the config > > file once every table. We were careful about it and also kept the > > original "base" cost limit around from table_recheck_autovac(). However, > > once we started reloading the config file more often, this no longer > > works. > > > > By separating the variables modified when the gucs are set and the ones > > used the code, we can make sure we always have the original value the > > guc was set to in vacuum_cost_limit and autovacuum_vacuum_cost_limit, > > whenever we need to reference it. > > > > That being said, perhaps we should document what extensions should do? > > Do you think they will want to use the variables backing the gucs or to > > be able to overwrite the variables being used in the code? > > I think I wasn't clear in my comment, sorry. I don't have a problem with > introducing a new variable to split the balanced value from the GUC value. > What I don't think we should do is repurpose an exported symbol into doing a > new thing. In the case at hand I think VacuumCostLimit and VacuumCostDelay > should remain the backing variables for the GUCs, with vacuum_cost_limit and > vacuum_cost_delay carrying the balanced values. So the inverse of what is in > the patch now. > > The risk of these symbols being used in extensions might be very low but on > principle it seems unwise to alter a symbol and risk subtle breakage. In attached v18, I have flipped them. Existing (in master) GUCs which were exported for VacuumCostLimit and VacuumCostDelay retain their names and new globals vacuum_cost_limit and vacuum_cost_delay have been introduced for use in the code. Flipping these kind of melted my mind, so I could definitely use another set of eyes double checking that the correct ones are being used in the correct places throughout 0002 and 0003. On Thu, Apr 6, 2023 at 3:09 PM Melanie Plageman <[email protected]> wrote: > > v17 attached does not yet fix the logging problem or variable naming > problem. > > I have not changed where AutoVacuumUpdateCostLimit() is called either. > > This is effectively just a round of cleanup. I hope I have managed to > address all other code review feedback so far, though some may have > slipped through the cracks. > > On Wed, Apr 5, 2023 at 2:56 PM Robert Haas <[email protected]> wrote: > > On Wed, Apr 5, 2023 at 11:29 AM Melanie Plageman <[email protected]> wrote: > > + /* > > + * Balance and update limit values for autovacuum workers. We must > > + * always do this in case the autovacuum launcher or another > > + * autovacuum worker has recalculated the number of workers across > > + * which we must balance the limit. This is done by the launcher when > > + * launching a new worker and by workers before vacuuming each table. > > + */ > > > > I don't quite understand what's going on here. A big reason that I'm > > worried about this whole issue in the first place is that sometimes > > there's a vacuum going on a giant table and you can't get it to go > > fast. You want it to absorb new settings, and to do so quickly. I > > realize that this is about the number of workers, not the actual cost > > limit, so that makes what I'm about to say less important. But ... is > > this often enough? Like, the time before we move onto the next table > > could be super long. The time before a new worker is launched should > > be ~autovacuum_naptime/autovacuum_max_workers or ~20s with default > > settings, so that's not horrible, but I'm kind of struggling to > > understand the rationale for this particular choice. Maybe it's fine. > > I've at least updated this comment to be more correct/less misleading. > > > > > + if (autovacuum_vac_cost_limit > 0) > > + VacuumCostLimit = autovacuum_vac_cost_limit; > > + else > > + VacuumCostLimit = vacuum_cost_limit; > > + > > + /* Only balance limit if no cost-related storage > > parameters specified */ > > + if (pg_atomic_unlocked_test_flag(&MyWorkerInfo->wi_dobalance)) > > + return; > > + Assert(VacuumCostLimit > 0); > > + > > + nworkers_for_balance = pg_atomic_read_u32( > > + > > &AutoVacuumShmem->av_nworkersForBalance); > > + > > + /* There is at least 1 autovac worker (this worker). */ > > + if (nworkers_for_balance <= 0) > > + elog(ERROR, "nworkers_for_balance must be > 0"); > > + > > + VacuumCostLimit = Max(VacuumCostLimit / > > nworkers_for_balance, 1); > > > > I think it would be better stylistically to use a temporary variable > > here and only assign the final value to VacuumCostLimit. > > I tried that and thought it adding confusing clutter. If it is a code > cleanliness issue, I am willing to change it, though. > > On Wed, Apr 5, 2023 at 3:04 PM Daniel Gustafsson <[email protected]> wrote: > > > > > On 5 Apr 2023, at 20:55, Robert Haas <[email protected]> wrote: > > > > > Again, I don't think this is something we should try to > > > address right now under time pressure, but in the future, I think we > > > should consider ripping this behavior out. > > > > I would not be opposed to that, but I wholeheartedly agree that it's not the > > job of this patch (or any patch at this point in the cycle). > > > > > + if (autovacuum_vac_cost_limit > 0) > > > + VacuumCostLimit = autovacuum_vac_cost_limit; > > > + else > > > + VacuumCostLimit = vacuum_cost_limit; > > > + > > > + /* Only balance limit if no cost-related storage > > > parameters specified */ > > > + if (pg_atomic_unlocked_test_flag(&MyWorkerInfo->wi_dobalance)) > > > + return; > > > + Assert(VacuumCostLimit > 0); > > > + > > > + nworkers_for_balance = pg_atomic_read_u32( > > > + > > > &AutoVacuumShmem->av_nworkersForBalance); > > > + > > > + /* There is at least 1 autovac worker (this worker). */ > > > + if (nworkers_for_balance <= 0) > > > + elog(ERROR, "nworkers_for_balance must be > 0"); > > > + > > > + VacuumCostLimit = Max(VacuumCostLimit / > > > nworkers_for_balance, 1); > > > > > > I think it would be better stylistically to use a temporary variable > > > here and only assign the final value to VacuumCostLimit. > > > > I can agree with that. Another supertiny nitpick on the above is to not end a > > single-line comment with a period. > > I have fixed this. > > On Thu, Apr 6, 2023 at 2:40 AM Masahiko Sawada <[email protected]> wrote: > > > > On Thu, Apr 6, 2023 at 12:29 AM Melanie Plageman > > <[email protected]> wrote: > > > > > > Thanks all for the reviews. > > > > > > v16 attached. I put it together rather quickly, so there might be a few > > > spurious whitespaces or similar. There is one rather annoying pgindent > > > outlier that I have to figure out what to do about as well. > > > > > > The remaining functional TODOs that I know of are: > > > > > > - Resolve what to do about names of GUC and vacuum variables for cost > > > limit and cost delay (since it may affect extensions) > > > > > > - Figure out what to do about the logging message which accesses dboid > > > and tableoid (lock/no lock, where to put it, etc) > > > > > > - I see several places in docs which reference the balancing algorithm > > > for autovac workers. I did not read them in great detail, but we may > > > want to review them to see if any require updates. > > > > > > - Consider whether or not the initial two commits should just be > > > squashed with the third commit > > > > > > - Anything else reviewers are still unhappy with > > > > > > > > > On Wed, Apr 5, 2023 at 1:56 AM Masahiko Sawada <[email protected]> wrote: > > > > > > > > On Wed, Apr 5, 2023 at 5:05 AM Melanie Plageman > > > > <[email protected]> wrote: > > > > > > > > > > On Tue, Apr 4, 2023 at 4:27 AM Masahiko Sawada <[email protected]> wrote: > > > > > > --- > > > > > > - if (worker->wi_proc != NULL) > > > > > > - elog(DEBUG2, "autovac_balance_cost(pid=%d > > > > > > db=%u, rel=%u, dobalance=%s cost_limit=%d, cost_limit_base=%d, > > > > > > cost_delay=%g)", > > > > > > - worker->wi_proc->pid, > > > > > > worker->wi_dboid, worker->wi_tableoid, > > > > > > - worker->wi_dobalance ? "yes" : "no", > > > > > > - worker->wi_cost_limit, > > > > > > worker->wi_cost_limit_base, > > > > > > - worker->wi_cost_delay); > > > > > > > > > > > > I think it's better to keep this kind of log in some form for > > > > > > debugging. For example, we can show these values of autovacuum workers > > > > > > in VacuumUpdateCosts(). > > > > > > > > > > I added a message to do_autovacuum() after calling VacuumUpdateCosts() > > > > > in the loop vacuuming each table. That means it will happen once per > > > > > table. It's not ideal that I had to move the call to VacuumUpdateCosts() > > > > > behind the shared lock in that loop so that we could access the pid and > > > > > such in the logging message after updating the cost and delay, but it is > > > > > probably okay. Though noone is going to be changing those at this > > > > > point, it still seemed better to access them under the lock. > > > > > > > > > > This does mean we won't log anything when we do change the values of > > > > > VacuumCostDelay and VacuumCostLimit while vacuuming a table. Is it worth > > > > > adding some code to do that in VacuumUpdateCosts() (only when the value > > > > > has changed not on every call to VacuumUpdateCosts())? Or perhaps we > > > > > could add it in the config reload branch that is already in > > > > > vacuum_delay_point()? > > > > > > > > Previously, we used to show the pid in the log since a worker/launcher > > > > set other workers' delay costs. But now that the worker sets its delay > > > > costs, we don't need to show the pid in the log. Also, I think it's > > > > useful for debugging and investigating the system if we log it when > > > > changing the values. The log I imagined to add was like: > > > > > > > > @@ -1801,6 +1801,13 @@ VacuumUpdateCosts(void) > > > > VacuumCostDelay = vacuum_cost_delay; > > > > > > > > AutoVacuumUpdateLimit(); > > > > + > > > > + elog(DEBUG2, "autovacuum update costs (db=%u, rel=%u, > > > > dobalance=%s, cost_limit=%d, cost_delay=%g active=%s failsafe=%s)", > > > > + MyWorkerInfo->wi_dboid, MyWorkerInfo->wi_tableoid, > > > > + pg_atomic_unlocked_test_flag(&MyWorkerInfo->wi_dobalance) > > > > ? "no" : "yes", > > > > + VacuumCostLimit, VacuumCostDelay, > > > > + VacuumCostDelay > 0 ? "yes" : "no", > > > > + VacuumFailsafeActive ? "yes" : "no"); > > > > } > > > > else > > > > { > > > > > > Makes sense. I've updated the log message to roughly what you suggested. > > > I also realized I think it does make sense to call it in > > > VacuumUpdateCosts() -- only for autovacuum workers of course. I've done > > > this. I haven't taken the lock though and can't decide if I must since > > > they access dboid and tableoid -- those are not going to change at this > > > point, but I still don't know if I can access them lock-free... > > > Perhaps there is a way to condition it on the log level? > > > > > > If I have to take a lock, then I don't know if we should put these in > > > VacuumUpdateCosts()... > > > > I think we don't need to acquire a lock there as both values are > > updated only by workers reporting this message. > > I dunno. I just don't feel that comfortable saying, oh it's okay to > access these without a lock probably. I propose we do one of the > following: > > - Take a shared lock inside VacuumUpdateCosts() (it is not called on every > call to vacuum_delay_point()) before reading from these variables. > > Pros: > - We will log whenever there is a change to these parameters > Cons: > - This adds overhead in the common case when log level is < DEBUG2. > Is there a way to check the log level before taking the lock? > - Acquiring the lock inside the function is inconsistent with the > pattern that some of the other autovacuum functions requiring > locks use (they assume you have a lock if needed inside of the > function). But, we could assert that the lock is not already held. > - If we later decide we don't like this choice and want to move the > logging elsewhere, it will necessarily log less frequently which > seems like a harder change to make than logging more frequently. > > - Move this logging into the loop through relations in do_autovacuum() > and the config reload code and take the shared lock before doing the > logging. > > Pros: > - Seems safe and not expensive > - Covers most of the times we would want the logging > Cons: > - duplicates logging in two places Okay, in an attempt to wrap up this saga, I have made the following change: Autovacuum workers, at the end of VacuumUpdateCosts(), check if cost limit or cost delay have been changed. If they have, they assert that they don't already hold the AutovacuumLock, take it in shared mode, and do the logging. - Melanie Attachments: [text/x-patch] v18-0003-Autovacuum-refreshes-cost-based-delay-params-mor.patch (22.2K, ../../CAAKRu_beQECOV4WfU6nG0hhEVobCnH88Y8MQi1LrTwMfRyxuCw@mail.gmail.com/2-v18-0003-Autovacuum-refreshes-cost-based-delay-params-mor.patch) download | inline diff: From 902ef92089d8b073be0d664fa5d5cc23624aa313 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Sat, 25 Mar 2023 14:14:55 -0400 Subject: [PATCH v18 3/3] Autovacuum refreshes cost-based delay params more often Allow autovacuum to reload the config file more often so that cost-based delay parameters can take effect while VACUUMing a relation. Previously, autovacuum workers only reloaded the config file once per relation vacuumed, so config changes could not take effect until beginning to vacuum the next table. Now, check if a reload is pending roughly once per block, when checking if we need to delay. In order for autovacuum workers to safely update their own cost delay and cost limit parameters without impacting performance, we had to rethink when and how these values were accessed. Previously, an autovacuum worker's wi_cost_limit was set only at the beginning of vacuuming a table, after reloading the config file. Therefore, at the time that autovac_balance_cost() was called, workers vacuuming tables with no cost-related storage parameters could still have different values for their wi_cost_limit_base and wi_cost_delay. Now that the cost parameters can be updated while vacuuming a table, workers will (within some margin of error) have no reason to have different values for cost limit and cost delay (in the absence of cost-related storage parameters). This removes the rationale for keeping cost limit and cost delay in shared memory. Balancing the cost limit requires only the number of active autovacuum workers vacuuming a table with no cost-based storage parameters. Reviewed-by: Masahiko Sawada <[email protected]> Reviewed-by: Daniel Gustafsson <[email protected]> Reviewed-by: Kyotaro Horiguchi <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7%2BDXhrjDA0gw%40mail.gmail.com --- src/backend/access/heap/vacuumlazy.c | 2 +- src/backend/commands/vacuum.c | 46 +++- src/backend/commands/vacuumparallel.c | 1 - src/backend/postmaster/autovacuum.c | 289 +++++++++++++++----------- src/include/commands/vacuum.h | 1 + 5 files changed, 217 insertions(+), 122 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 2ba85bd3d6..0a9ebd22bd 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -389,7 +389,7 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, Assert(params->index_cleanup != VACOPTVALUE_UNSPECIFIED); Assert(params->truncate != VACOPTVALUE_UNSPECIFIED && params->truncate != VACOPTVALUE_AUTO); - VacuumFailsafeActive = false; + Assert(!VacuumFailsafeActive); vacrel->consider_bypass_optimization = true; vacrel->do_index_vacuuming = true; vacrel->do_index_cleanup = true; diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index f2be74cdb5..ca347e0a6d 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -48,6 +48,7 @@ #include "pgstat.h" #include "postmaster/autovacuum.h" #include "postmaster/bgworker_internals.h" +#include "postmaster/interrupt.h" #include "storage/bufmgr.h" #include "storage/lmgr.h" #include "storage/pmsignal.h" @@ -523,9 +524,9 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy, { ListCell *cur; - VacuumUpdateCosts(); in_vacuum = true; - VacuumCostActive = (vacuum_cost_delay > 0); + VacuumFailsafeActive = false; + VacuumUpdateCosts(); VacuumCostBalance = 0; VacuumPageHit = 0; VacuumPageMiss = 0; @@ -579,12 +580,20 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy, CommandCounterIncrement(); } } + + /* + * Ensure VacuumFailsafeActive has been reset before vacuuming the + * next relation. + */ + VacuumFailsafeActive = false; } } PG_FINALLY(); { in_vacuum = false; VacuumCostActive = false; + VacuumFailsafeActive = false; + VacuumCostBalance = 0; } PG_END_TRY(); @@ -2245,7 +2254,28 @@ vacuum_delay_point(void) /* Always check for interrupts */ CHECK_FOR_INTERRUPTS(); - if (!VacuumCostActive || InterruptPending) + if (InterruptPending || + (!VacuumCostActive && !ConfigReloadPending)) + return; + + /* + * Autovacuum workers should reload the configuration file if requested. + * This allows changes to [autovacuum_]vacuum_cost_limit and + * [autovacuum_]vacuum_cost_delay to take effect while a table is being + * vacuumed or analyzed. + */ + if (ConfigReloadPending && IsAutoVacuumWorkerProcess()) + { + ConfigReloadPending = false; + ProcessConfigFile(PGC_SIGHUP); + VacuumUpdateCosts(); + } + + /* + * If we disabled cost-based delays after reloading the config file, + * return. + */ + if (!VacuumCostActive) return; /* @@ -2278,7 +2308,15 @@ vacuum_delay_point(void) VacuumCostBalance = 0; - VacuumUpdateCosts(); + /* + * Balance and update limit values for autovacuum workers. We must do + * this periodically, as the number of workers across which we are + * balancing the limit may have changed. + * + * XXX: There may be better criteria for determining when to do this + * besides "check after napping". + */ + AutoVacuumUpdateCostLimit(); /* Might have gotten an interrupt while sleeping */ CHECK_FOR_INTERRUPTS(); diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c index cc0aff7904..e200d5caf8 100644 --- a/src/backend/commands/vacuumparallel.c +++ b/src/backend/commands/vacuumparallel.c @@ -995,7 +995,6 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc) false); /* Set cost-based vacuum delay */ - VacuumCostActive = (vacuum_cost_delay > 0); VacuumUpdateCosts(); VacuumCostBalance = 0; VacuumPageHit = 0; diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index 3644b86443..8529324ebd 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -139,6 +139,18 @@ int Log_autovacuum_min_duration = 600000; static bool am_autovacuum_launcher = false; static bool am_autovacuum_worker = false; +/* + * Variables to save the cost-related storage parameters for the current + * relation being vacuumed by this autovacuum worker. Using these, we can + * ensure we don't overwrite the values of vacuum_cost_delay and + * vacuum_cost_limit after reloading the configuration file. They are + * initialized to "invalid" values to indicate that no cost-related storage + * parameters were specified and will be set in do_autovacuum() after checking + * the storage parameters in table_recheck_autovac(). + */ +static double av_storage_param_cost_delay = -1; +static int av_storage_param_cost_limit = -1; + /* Flags set by signal handlers */ static volatile sig_atomic_t got_SIGUSR2 = false; @@ -189,8 +201,8 @@ typedef struct autovac_table { Oid at_relid; VacuumParams at_params; - double at_vacuum_cost_delay; - int at_vacuum_cost_limit; + double at_storage_param_vac_cost_delay; + int at_storage_param_vac_cost_limit; bool at_dobalance; bool at_sharedrel; char *at_relname; @@ -209,7 +221,7 @@ typedef struct autovac_table * wi_sharedrel flag indicating whether table is marked relisshared * wi_proc pointer to PGPROC of the running worker, NULL if not started * wi_launchtime Time at which this worker was launched - * wi_cost_* Vacuum cost-based delay parameters current in this worker + * wi_dobalance Whether this worker should be included in balance calculations * * All fields are protected by AutovacuumLock, except for wi_tableoid and * wi_sharedrel which are protected by AutovacuumScheduleLock (note these @@ -223,11 +235,8 @@ typedef struct WorkerInfoData Oid wi_tableoid; PGPROC *wi_proc; TimestampTz wi_launchtime; - bool wi_dobalance; + pg_atomic_flag wi_dobalance; bool wi_sharedrel; - double wi_cost_delay; - int wi_cost_limit; - int wi_cost_limit_base; } WorkerInfoData; typedef struct WorkerInfoData *WorkerInfo; @@ -273,6 +282,8 @@ typedef struct AutoVacuumWorkItem * av_startingWorker pointer to WorkerInfo currently being started (cleared by * the worker itself as soon as it's up and running) * av_workItems work item array + * av_nworkersForBalance the number of autovacuum workers to use when + * calculating the per worker cost limit * * This struct is protected by AutovacuumLock, except for av_signal and parts * of the worker list (see above). @@ -286,6 +297,7 @@ typedef struct dlist_head av_runningWorkers; WorkerInfo av_startingWorker; AutoVacuumWorkItem av_workItems[NUM_WORKITEMS]; + pg_atomic_uint32 av_nworkersForBalance; } AutoVacuumShmemStruct; static AutoVacuumShmemStruct *AutoVacuumShmem; @@ -319,7 +331,7 @@ static void launch_worker(TimestampTz now); static List *get_database_list(void); static void rebuild_database_list(Oid newdb); static int db_comparator(const void *a, const void *b); -static void autovac_balance_cost(void); +static void autovac_recalculate_workers_for_balance(void); static void do_autovacuum(void); static void FreeWorkerInfo(int code, Datum arg); @@ -669,7 +681,7 @@ AutoVacLauncherMain(int argc, char *argv[]) { LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); AutoVacuumShmem->av_signal[AutoVacRebalance] = false; - autovac_balance_cost(); + autovac_recalculate_workers_for_balance(); LWLockRelease(AutovacuumLock); } @@ -818,11 +830,6 @@ HandleAutoVacLauncherInterrupts(void) if (!AutoVacuumingActive()) AutoVacLauncherShutdown(); - /* rebalance in case the default cost parameters changed */ - LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); - autovac_balance_cost(); - LWLockRelease(AutovacuumLock); - /* rebuild the list in case the naptime changed */ rebuild_database_list(InvalidOid); } @@ -1754,10 +1761,7 @@ FreeWorkerInfo(int code, Datum arg) MyWorkerInfo->wi_sharedrel = false; MyWorkerInfo->wi_proc = NULL; MyWorkerInfo->wi_launchtime = 0; - MyWorkerInfo->wi_dobalance = false; - MyWorkerInfo->wi_cost_delay = 0; - MyWorkerInfo->wi_cost_limit = 0; - MyWorkerInfo->wi_cost_limit_base = 0; + pg_atomic_clear_flag(&MyWorkerInfo->wi_dobalance); dlist_push_head(&AutoVacuumShmem->av_freeWorkers, &MyWorkerInfo->wi_links); /* not mine anymore */ @@ -1781,10 +1785,20 @@ FreeWorkerInfo(int code, Datum arg) void VacuumUpdateCosts(void) { + double original_cost_delay = vacuum_cost_delay; + int original_cost_limit = vacuum_cost_limit; + if (MyWorkerInfo) { - vacuum_cost_delay = MyWorkerInfo->wi_cost_delay; - vacuum_cost_limit = MyWorkerInfo->wi_cost_limit; + if (av_storage_param_cost_delay >= 0) + vacuum_cost_delay = av_storage_param_cost_delay; + else if (autovacuum_vac_cost_delay >= 0) + vacuum_cost_delay = autovacuum_vac_cost_delay; + else + /* fall back to VacuumCostDelay */ + vacuum_cost_delay = VacuumCostDelay; + + AutoVacuumUpdateCostLimit(); } else { @@ -1792,88 +1806,124 @@ VacuumUpdateCosts(void) vacuum_cost_delay = VacuumCostDelay; vacuum_cost_limit = VacuumCostLimit; } + + /* + * If configuration changes are allowed to impact VacuumCostActive, make + * sure it is updated. + */ + if (VacuumFailsafeActive) + Assert(!VacuumCostActive); + else if (vacuum_cost_delay > 0) + VacuumCostActive = true; + else + { + VacuumCostActive = false; + VacuumCostBalance = 0; + } + + if (MyWorkerInfo) + { + /* Only log updates to cost-related variables */ + if (vacuum_cost_delay == original_cost_delay && + vacuum_cost_limit == original_cost_limit) + return; + + Assert(!LWLockHeldByMe(AutovacuumLock)); + + LWLockAcquire(AutovacuumLock, LW_SHARED); + + elog(DEBUG2, + "Autovacuum VacuumUpdateCosts(db=%u, rel=%u, dobalance=%s, cost_limit=%d, cost_delay=%g active=%s failsafe=%s)", + MyWorkerInfo->wi_dboid, MyWorkerInfo->wi_tableoid, + pg_atomic_unlocked_test_flag(&MyWorkerInfo->wi_dobalance) ? "no" : "yes", + vacuum_cost_limit, vacuum_cost_delay, + vacuum_cost_delay > 0 ? "yes" : "no", + VacuumFailsafeActive ? "yes" : "no"); + + LWLockRelease(AutovacuumLock); + } } /* - * autovac_balance_cost - * Recalculate the cost limit setting for each active worker. - * - * Caller must hold the AutovacuumLock in exclusive mode. + * Update vacuum_cost_limit with the correct value for an autovacuum worker, + * given the value of other relevant cost limit parameters and the number of + * workers across which the limit must be balanced. Autovacuum workers must + * call this regularly in case av_nworkersForBalance has been updated by + * another worker or by the autovacuum launcher. They must also call it after a + * config reload. */ -static void -autovac_balance_cost(void) +void +AutoVacuumUpdateCostLimit(void) { + if (!MyWorkerInfo) + return; + /* - * The idea here is that we ration out I/O equally. The amount of I/O - * that a worker can consume is determined by cost_limit/cost_delay, so we - * try to equalize those ratios rather than the raw limit settings. - * * note: in cost_limit, zero also means use value from elsewhere, because * zero is not a valid value. */ - int vac_cost_limit = (autovacuum_vac_cost_limit > 0 ? - autovacuum_vac_cost_limit : VacuumCostLimit); - double vac_cost_delay = (autovacuum_vac_cost_delay >= 0 ? - autovacuum_vac_cost_delay : VacuumCostDelay); - double cost_total; - double cost_avail; - dlist_iter iter; - - /* not set? nothing to do */ - if (vac_cost_limit <= 0 || vac_cost_delay <= 0) - return; - /* calculate the total base cost limit of participating active workers */ - cost_total = 0.0; - dlist_foreach(iter, &AutoVacuumShmem->av_runningWorkers) + if (av_storage_param_cost_limit > 0) + vacuum_cost_limit = av_storage_param_cost_limit; + else { - WorkerInfo worker = dlist_container(WorkerInfoData, wi_links, iter.cur); + int nworkers_for_balance; + + if (autovacuum_vac_cost_limit > 0) + vacuum_cost_limit = autovacuum_vac_cost_limit; + else + vacuum_cost_limit = VacuumCostLimit; + + /* Only balance limit if no cost-related storage parameters specified */ + if (pg_atomic_unlocked_test_flag(&MyWorkerInfo->wi_dobalance)) + return; + + Assert(vacuum_cost_limit > 0); - if (worker->wi_proc != NULL && - worker->wi_dobalance && - worker->wi_cost_limit_base > 0 && worker->wi_cost_delay > 0) - cost_total += - (double) worker->wi_cost_limit_base / worker->wi_cost_delay; + nworkers_for_balance = pg_atomic_read_u32(&AutoVacuumShmem->av_nworkersForBalance); + + /* There is at least 1 autovac worker (this worker) */ + if (nworkers_for_balance <= 0) + elog(ERROR, "nworkers_for_balance must be > 0"); + + vacuum_cost_limit = Max(vacuum_cost_limit / nworkers_for_balance, 1); } +} - /* there are no cost limits -- nothing to do */ - if (cost_total <= 0) - return; +/* + * autovac_recalculate_workers_for_balance + * Recalculate the number of workers to consider, given cost-related + * storage parameters and the current number of active workers. + * + * Caller must hold the AutovacuumLock in at least shared mode to access + * worker->wi_proc. + */ +static void +autovac_recalculate_workers_for_balance(void) +{ + dlist_iter iter; + int orig_nworkers_for_balance; + int nworkers_for_balance = 0; + + Assert(LWLockHeldByMe(AutovacuumLock)); + + orig_nworkers_for_balance = + pg_atomic_read_u32(&AutoVacuumShmem->av_nworkersForBalance); - /* - * Adjust cost limit of each active worker to balance the total of cost - * limit to autovacuum_vacuum_cost_limit. - */ - cost_avail = (double) vac_cost_limit / vac_cost_delay; dlist_foreach(iter, &AutoVacuumShmem->av_runningWorkers) { WorkerInfo worker = dlist_container(WorkerInfoData, wi_links, iter.cur); - if (worker->wi_proc != NULL && - worker->wi_dobalance && - worker->wi_cost_limit_base > 0 && worker->wi_cost_delay > 0) - { - int limit = (int) - (cost_avail * worker->wi_cost_limit_base / cost_total); - - /* - * We put a lower bound of 1 on the cost_limit, to avoid division- - * by-zero in the vacuum code. Also, in case of roundoff trouble - * in these calculations, let's be sure we don't ever set - * cost_limit to more than the base value. - */ - worker->wi_cost_limit = Max(Min(limit, - worker->wi_cost_limit_base), - 1); - } + if (worker->wi_proc == NULL || + pg_atomic_unlocked_test_flag(&worker->wi_dobalance)) + continue; - if (worker->wi_proc != NULL) - elog(DEBUG2, "autovac_balance_cost(pid=%d db=%u, rel=%u, dobalance=%s cost_limit=%d, cost_limit_base=%d, cost_delay=%g)", - worker->wi_proc->pid, worker->wi_dboid, worker->wi_tableoid, - worker->wi_dobalance ? "yes" : "no", - worker->wi_cost_limit, worker->wi_cost_limit_base, - worker->wi_cost_delay); + nworkers_for_balance++; } + + if (nworkers_for_balance != orig_nworkers_for_balance) + pg_atomic_write_u32(&AutoVacuumShmem->av_nworkersForBalance, + nworkers_for_balance); } /* @@ -2421,23 +2471,34 @@ do_autovacuum(void) continue; } - /* Must hold AutovacuumLock while mucking with cost balance info */ - LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); + /* + * Save the cost-related storage parameter values in global variables + * for reference when updating vacuum_cost_delay and vacuum_cost_limit + * during vacuuming this table. + */ + av_storage_param_cost_delay = tab->at_storage_param_vac_cost_delay; + av_storage_param_cost_limit = tab->at_storage_param_vac_cost_limit; - /* advertise my cost delay parameters for the balancing algorithm */ - MyWorkerInfo->wi_dobalance = tab->at_dobalance; - MyWorkerInfo->wi_cost_delay = tab->at_vacuum_cost_delay; - MyWorkerInfo->wi_cost_limit = tab->at_vacuum_cost_limit; - MyWorkerInfo->wi_cost_limit_base = tab->at_vacuum_cost_limit; + /* + * We only expect this worker to ever set the flag, so don't bother + * checking the return value. We shouldn't have to retry. + */ + if (tab->at_dobalance) + pg_atomic_test_set_flag(&MyWorkerInfo->wi_dobalance); + else + pg_atomic_clear_flag(&MyWorkerInfo->wi_dobalance); - /* do a balance */ - autovac_balance_cost(); + LWLockAcquire(AutovacuumLock, LW_SHARED); + autovac_recalculate_workers_for_balance(); + LWLockRelease(AutovacuumLock); - /* set the active cost parameters from the result of that */ + /* + * We wait until this point to update cost delay and cost limit + * values, even though we reloaded the configuration file above, so + * that we can take into account the cost-related storage parameters. + */ VacuumUpdateCosts(); - /* done */ - LWLockRelease(AutovacuumLock); /* clean up memory before each iteration */ MemoryContextResetAndDeleteChildren(PortalContext); @@ -2521,16 +2582,17 @@ deleted: pfree(tab); /* - * Remove my info from shared memory. We could, but intentionally - * don't, clear wi_cost_limit and friends --- this is on the - * assumption that we probably have more to do with similar cost - * settings, so we don't want to give up our share of I/O for a very - * short interval and thereby thrash the global balance. + * Remove my info from shared memory. We set wi_dobalance on the + * assumption that we are more likely than not to vacuum a table with + * no cost-related storage parameters next, so we want to claim our + * share of I/O as soon as possible to avoid thrashing the global + * balance. */ LWLockAcquire(AutovacuumScheduleLock, LW_EXCLUSIVE); MyWorkerInfo->wi_tableoid = InvalidOid; MyWorkerInfo->wi_sharedrel = false; LWLockRelease(AutovacuumScheduleLock); + pg_atomic_test_set_flag(&MyWorkerInfo->wi_dobalance); } /* @@ -2562,6 +2624,7 @@ deleted: { ConfigReloadPending = false; ProcessConfigFile(PGC_SIGHUP); + VacuumUpdateCosts(); } LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); @@ -2797,8 +2860,6 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map, int freeze_table_age; int multixact_freeze_min_age; int multixact_freeze_table_age; - int vac_cost_limit; - double vac_cost_delay; int log_min_duration; /* @@ -2808,20 +2869,6 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map, * defaults, autovacuum's own first and plain vacuum second. */ - /* -1 in autovac setting means use plain vacuum_cost_delay */ - vac_cost_delay = (avopts && avopts->vacuum_cost_delay >= 0) - ? avopts->vacuum_cost_delay - : (autovacuum_vac_cost_delay >= 0) - ? autovacuum_vac_cost_delay - : VacuumCostDelay; - - /* 0 or -1 in autovac setting means use plain vacuum_cost_limit */ - vac_cost_limit = (avopts && avopts->vacuum_cost_limit > 0) - ? avopts->vacuum_cost_limit - : (autovacuum_vac_cost_limit > 0) - ? autovacuum_vac_cost_limit - : VacuumCostLimit; - /* -1 in autovac setting means use log_autovacuum_min_duration */ log_min_duration = (avopts && avopts->log_min_duration >= 0) ? avopts->log_min_duration @@ -2877,8 +2924,10 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map, tab->at_params.multixact_freeze_table_age = multixact_freeze_table_age; tab->at_params.is_wraparound = wraparound; tab->at_params.log_min_duration = log_min_duration; - tab->at_vacuum_cost_limit = vac_cost_limit; - tab->at_vacuum_cost_delay = vac_cost_delay; + tab->at_storage_param_vac_cost_limit = avopts ? + avopts->vacuum_cost_limit : 0; + tab->at_storage_param_vac_cost_delay = avopts ? + avopts->vacuum_cost_delay : -1; tab->at_relname = NULL; tab->at_nspname = NULL; tab->at_datname = NULL; @@ -3380,10 +3429,18 @@ AutoVacuumShmemInit(void) worker = (WorkerInfo) ((char *) AutoVacuumShmem + MAXALIGN(sizeof(AutoVacuumShmemStruct))); + /* initialize the WorkerInfo free list */ for (i = 0; i < autovacuum_max_workers; i++) + { dlist_push_head(&AutoVacuumShmem->av_freeWorkers, &worker[i].wi_links); + + pg_atomic_init_flag(&worker[i].wi_dobalance); + } + + pg_atomic_init_u32(&AutoVacuumShmem->av_nworkersForBalance, 0); + } else Assert(found); diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h index 50caf1315d..2a856b0e5e 100644 --- a/src/include/commands/vacuum.h +++ b/src/include/commands/vacuum.h @@ -350,6 +350,7 @@ extern IndexBulkDeleteResult *vac_cleanup_one_index(IndexVacuumInfo *ivinfo, extern Size vac_max_items_to_alloc_size(int max_items); /* In postmaster/autovacuum.c */ +extern void AutoVacuumUpdateCostLimit(void); extern void VacuumUpdateCosts(void); /* in commands/vacuumparallel.c */ -- 2.37.2 [text/x-patch] v18-0002-Separate-vacuum-cost-variables-from-GUCs.patch (8.7K, ../../CAAKRu_beQECOV4WfU6nG0hhEVobCnH88Y8MQi1LrTwMfRyxuCw@mail.gmail.com/3-v18-0002-Separate-vacuum-cost-variables-from-GUCs.patch) download | inline diff: From 92fc87969a496c1a1f5fd612d3c7c32251dec6e1 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Thu, 6 Apr 2023 16:02:12 -0400 Subject: [PATCH v18 2/3] Separate vacuum cost variables from GUCs Vacuum code run both by autovacuum workers and a backend doing VACUUM/ANALYZE previously inspected VacuumCostLimit and VacuumCostDelay, which are the global variables backing the GUCs vacuum_cost_limit and vacuum_cost_delay. Autovacuum workers needed to override these variables with their own values, derived from autovacuum_vacuum_cost_limit and autovacuum_vacuum_cost_delay and worker cost limit balancing logic. This led to confusing code which, in some cases, both derived and set a new value of VacuumCostLimit from VacuumCostLimit. In preparation for refreshing these GUC values more often, introduce new, independent global variables and add a function to update them using the GUCs and existing logic. Per suggestion by Kyotaro Horiguchi Reviewed-by: Masahiko Sawada <[email protected]> Reviewed-by: Daniel Gustafsson <[email protected]> Reviewed-by: Kyotaro Horiguchi <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7%2BDXhrjDA0gw%40mail.gmail.com --- src/backend/commands/vacuum.c | 29 +++++++++++++++-------- src/backend/commands/vacuumparallel.c | 3 ++- src/backend/postmaster/autovacuum.c | 34 +++++++++++---------------- src/include/commands/vacuum.h | 5 ++++ src/include/postmaster/autovacuum.h | 3 --- 5 files changed, 40 insertions(+), 34 deletions(-) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 7fc5c19e37..f2be74cdb5 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -72,6 +72,15 @@ int vacuum_multixact_freeze_table_age; int vacuum_failsafe_age; int vacuum_multixact_failsafe_age; +/* + * Variables for cost-based vacuum delay. The defaults differ between + * autovacuum and vacuum. They should be set with the appropriate GUC value in + * vacuum code. They are initialized here to the defaults for client backends + * executing VACUUM or ANALYZE. + */ +double vacuum_cost_delay = 0; +int vacuum_cost_limit = 200; + /* * VacuumFailsafeActive is a defined as a global so that we can determine * whether or not to re-enable cost-based vacuum delay when vacuuming a table. @@ -514,8 +523,9 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy, { ListCell *cur; + VacuumUpdateCosts(); in_vacuum = true; - VacuumCostActive = (VacuumCostDelay > 0); + VacuumCostActive = (vacuum_cost_delay > 0); VacuumCostBalance = 0; VacuumPageHit = 0; VacuumPageMiss = 0; @@ -2244,14 +2254,14 @@ vacuum_delay_point(void) */ if (VacuumSharedCostBalance != NULL) msec = compute_parallel_delay(); - else if (VacuumCostBalance >= VacuumCostLimit) - msec = VacuumCostDelay * VacuumCostBalance / VacuumCostLimit; + else if (VacuumCostBalance >= vacuum_cost_limit) + msec = vacuum_cost_delay * VacuumCostBalance / vacuum_cost_limit; /* Nap if appropriate */ if (msec > 0) { - if (msec > VacuumCostDelay * 4) - msec = VacuumCostDelay * 4; + if (msec > vacuum_cost_delay * 4) + msec = vacuum_cost_delay * 4; pgstat_report_wait_start(WAIT_EVENT_VACUUM_DELAY); pg_usleep(msec * 1000); @@ -2268,8 +2278,7 @@ vacuum_delay_point(void) VacuumCostBalance = 0; - /* update balance values for workers */ - AutoVacuumUpdateDelay(); + VacuumUpdateCosts(); /* Might have gotten an interrupt while sleeping */ CHECK_FOR_INTERRUPTS(); @@ -2319,11 +2328,11 @@ compute_parallel_delay(void) /* Compute the total local balance for the current worker */ VacuumCostBalanceLocal += VacuumCostBalance; - if ((shared_balance >= VacuumCostLimit) && - (VacuumCostBalanceLocal > 0.5 * ((double) VacuumCostLimit / nworkers))) + if ((shared_balance >= vacuum_cost_limit) && + (VacuumCostBalanceLocal > 0.5 * ((double) vacuum_cost_limit / nworkers))) { /* Compute sleep time based on the local cost balance */ - msec = VacuumCostDelay * VacuumCostBalanceLocal / VacuumCostLimit; + msec = vacuum_cost_delay * VacuumCostBalanceLocal / vacuum_cost_limit; pg_atomic_sub_fetch_u32(VacuumSharedCostBalance, VacuumCostBalanceLocal); VacuumCostBalanceLocal = 0; } diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c index 563117a8f6..cc0aff7904 100644 --- a/src/backend/commands/vacuumparallel.c +++ b/src/backend/commands/vacuumparallel.c @@ -995,7 +995,8 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc) false); /* Set cost-based vacuum delay */ - VacuumCostActive = (VacuumCostDelay > 0); + VacuumCostActive = (vacuum_cost_delay > 0); + VacuumUpdateCosts(); VacuumCostBalance = 0; VacuumPageHit = 0; VacuumPageMiss = 0; diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index c1e911b1b3..3644b86443 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -1773,16 +1773,24 @@ FreeWorkerInfo(int code, Datum arg) } /* - * Update the cost-based delay parameters, so that multiple workers consume - * each a fraction of the total available I/O. + * Update vacuum cost-based delay-related parameters for autovacuum workers and + * backends executing VACUUM or ANALYZE using the value of relevant gucs and + * global state. This must be called during setup for vacuum and after every + * config reload to ensure up-to-date values. */ void -AutoVacuumUpdateDelay(void) +VacuumUpdateCosts(void) { if (MyWorkerInfo) { - VacuumCostDelay = MyWorkerInfo->wi_cost_delay; - VacuumCostLimit = MyWorkerInfo->wi_cost_limit; + vacuum_cost_delay = MyWorkerInfo->wi_cost_delay; + vacuum_cost_limit = MyWorkerInfo->wi_cost_limit; + } + else + { + /* Must be explicit VACUUM or ANALYZE */ + vacuum_cost_delay = VacuumCostDelay; + vacuum_cost_limit = VacuumCostLimit; } } @@ -2311,8 +2319,6 @@ do_autovacuum(void) autovac_table *tab; bool isshared; bool skipit; - double stdVacuumCostDelay; - int stdVacuumCostLimit; dlist_iter iter; CHECK_FOR_INTERRUPTS(); @@ -2415,14 +2421,6 @@ do_autovacuum(void) continue; } - /* - * Remember the prevailing values of the vacuum cost GUCs. We have to - * restore these at the bottom of the loop, else we'll compute wrong - * values in the next iteration of autovac_balance_cost(). - */ - stdVacuumCostDelay = VacuumCostDelay; - stdVacuumCostLimit = VacuumCostLimit; - /* Must hold AutovacuumLock while mucking with cost balance info */ LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); @@ -2436,7 +2434,7 @@ do_autovacuum(void) autovac_balance_cost(); /* set the active cost parameters from the result of that */ - AutoVacuumUpdateDelay(); + VacuumUpdateCosts(); /* done */ LWLockRelease(AutovacuumLock); @@ -2533,10 +2531,6 @@ deleted: MyWorkerInfo->wi_tableoid = InvalidOid; MyWorkerInfo->wi_sharedrel = false; LWLockRelease(AutovacuumScheduleLock); - - /* restore vacuum cost GUCs for the next iteration */ - VacuumCostDelay = stdVacuumCostDelay; - VacuumCostLimit = stdVacuumCostLimit; } /* diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h index 1223d15e0d..50caf1315d 100644 --- a/src/include/commands/vacuum.h +++ b/src/include/commands/vacuum.h @@ -307,6 +307,8 @@ extern PGDLLIMPORT pg_atomic_uint32 *VacuumActiveNWorkers; extern PGDLLIMPORT int VacuumCostBalanceLocal; extern PGDLLIMPORT bool VacuumFailsafeActive; +extern PGDLLIMPORT double vacuum_cost_delay; +extern PGDLLIMPORT int vacuum_cost_limit; /* in commands/vacuum.c */ extern void ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel); @@ -347,6 +349,9 @@ extern IndexBulkDeleteResult *vac_cleanup_one_index(IndexVacuumInfo *ivinfo, IndexBulkDeleteResult *istat); extern Size vac_max_items_to_alloc_size(int max_items); +/* In postmaster/autovacuum.c */ +extern void VacuumUpdateCosts(void); + /* in commands/vacuumparallel.c */ extern ParallelVacuumState *parallel_vacuum_init(Relation rel, Relation *indrels, int nindexes, int nrequested_workers, diff --git a/src/include/postmaster/autovacuum.h b/src/include/postmaster/autovacuum.h index c140371b51..65afd1ea1e 100644 --- a/src/include/postmaster/autovacuum.h +++ b/src/include/postmaster/autovacuum.h @@ -63,9 +63,6 @@ extern int StartAutoVacWorker(void); /* called from postmaster when a worker could not be forked */ extern void AutoVacWorkerFailed(void); -/* autovacuum cost-delay balancer */ -extern void AutoVacuumUpdateDelay(void); - #ifdef EXEC_BACKEND extern void AutoVacLauncherMain(int argc, char *argv[]) pg_attribute_noreturn(); extern void AutoVacWorkerMain(int argc, char *argv[]) pg_attribute_noreturn(); -- 2.37.2 [text/x-patch] v18-0001-Make-vacuum-failsafe_active-global.patch (5.3K, ../../CAAKRu_beQECOV4WfU6nG0hhEVobCnH88Y8MQi1LrTwMfRyxuCw@mail.gmail.com/4-v18-0001-Make-vacuum-failsafe_active-global.patch) download | inline diff: From 0042067ce72a474fe4087245b978847c0b835b72 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Fri, 31 Mar 2023 10:38:39 -0400 Subject: [PATCH v18 1/3] Make vacuum failsafe_active global While vacuuming a table in failsafe mode, VacuumCostActive should not be re-enabled. This currently isn't a problem because vacuum cost parameters are only refreshed in between vacuuming tables and failsafe status is reset for every table. In preparation for allowing vacuum cost parameters to be updated more frequently, elevate LVRelState->failsafe_active to a global, VacuumFailsafeActive, which will be checked when determining whether or not to re-enable vacuum cost-related delays. Reviewed-by: Masahiko Sawada <[email protected]> Reviewed-by: Daniel Gustafsson <[email protected]> Reviewed-by: Kyotaro Horiguchi <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7%2BDXhrjDA0gw%40mail.gmail.com --- src/backend/access/heap/vacuumlazy.c | 16 +++++++--------- src/backend/commands/vacuum.c | 15 +++++++++++++++ src/include/commands/vacuum.h | 1 + 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 639179aa46..2ba85bd3d6 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -153,8 +153,6 @@ typedef struct LVRelState bool aggressive; /* Use visibility map to skip? (disabled by DISABLE_PAGE_SKIPPING) */ bool skipwithvm; - /* Wraparound failsafe has been triggered? */ - bool failsafe_active; /* Consider index vacuuming bypass optimization? */ bool consider_bypass_optimization; @@ -391,7 +389,7 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, Assert(params->index_cleanup != VACOPTVALUE_UNSPECIFIED); Assert(params->truncate != VACOPTVALUE_UNSPECIFIED && params->truncate != VACOPTVALUE_AUTO); - vacrel->failsafe_active = false; + VacuumFailsafeActive = false; vacrel->consider_bypass_optimization = true; vacrel->do_index_vacuuming = true; vacrel->do_index_cleanup = true; @@ -709,7 +707,7 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, } else { - if (!vacrel->failsafe_active) + if (!VacuumFailsafeActive) appendStringInfoString(&buf, _("index scan bypassed: ")); else appendStringInfoString(&buf, _("index scan bypassed by failsafe: ")); @@ -2293,7 +2291,7 @@ lazy_vacuum(LVRelState *vacrel) * vacuuming or heap vacuuming. This VACUUM operation won't end up * back here again. */ - Assert(vacrel->failsafe_active); + Assert(VacuumFailsafeActive); } /* @@ -2374,7 +2372,7 @@ lazy_vacuum_all_indexes(LVRelState *vacrel) */ Assert(vacrel->num_index_scans > 0 || vacrel->dead_items->num_items == vacrel->lpdead_items); - Assert(allindexes || vacrel->failsafe_active); + Assert(allindexes || VacuumFailsafeActive); /* * Increase and report the number of index scans. @@ -2616,12 +2614,12 @@ static bool lazy_check_wraparound_failsafe(LVRelState *vacrel) { /* Don't warn more than once per VACUUM */ - if (vacrel->failsafe_active) + if (VacuumFailsafeActive) return true; if (unlikely(vacuum_xid_failsafe_check(&vacrel->cutoffs))) { - vacrel->failsafe_active = true; + VacuumFailsafeActive = true; /* * Abandon use of a buffer access strategy to allow use of all of @@ -2820,7 +2818,7 @@ should_attempt_truncation(LVRelState *vacrel) { BlockNumber possibly_freeable; - if (!vacrel->do_rel_truncate || vacrel->failsafe_active || + if (!vacrel->do_rel_truncate || VacuumFailsafeActive || old_snapshot_threshold >= 0) return false; diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index ea1d8960f4..7fc5c19e37 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -72,6 +72,21 @@ int vacuum_multixact_freeze_table_age; int vacuum_failsafe_age; int vacuum_multixact_failsafe_age; +/* + * VacuumFailsafeActive is a defined as a global so that we can determine + * whether or not to re-enable cost-based vacuum delay when vacuuming a table. + * If failsafe mode has been engaged, we will not re-enable cost-based delay + * for the table until after vacuuming has completed, regardless of other + * settings. + * + * Only VACUUM code should inspect this variable and only table access methods + * should set it to true. In Table AM-agnostic VACUUM code, this variable is + * inspected to determine whether or not to allow cost-based delays. Table AMs + * are free to set it if they desire this behavior, but it is false by default + * and reset to false in between vacuuming each relation. + */ +bool VacuumFailsafeActive = false; + /* * Variables for cost-based parallel vacuum. See comments atop * compute_parallel_delay to understand how it works. diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h index 19ca818dc2..1223d15e0d 100644 --- a/src/include/commands/vacuum.h +++ b/src/include/commands/vacuum.h @@ -306,6 +306,7 @@ extern PGDLLIMPORT pg_atomic_uint32 *VacuumSharedCostBalance; extern PGDLLIMPORT pg_atomic_uint32 *VacuumActiveNWorkers; extern PGDLLIMPORT int VacuumCostBalanceLocal; +extern PGDLLIMPORT bool VacuumFailsafeActive; /* in commands/vacuum.c */ extern void ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel); -- 2.37.2 ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-04-06 21:45 Daniel Gustafsson <[email protected]> parent: Melanie Plageman <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Daniel Gustafsson @ 2023-04-06 21:45 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Robert Haas <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Masahiko Sawada <[email protected]>; Kyotaro Horiguchi <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> > On 6 Apr 2023, at 23:06, Melanie Plageman <[email protected]> wrote: > Autovacuum workers, at the end of VacuumUpdateCosts(), check if cost > limit or cost delay have been changed. If they have, they assert that > they don't already hold the AutovacuumLock, take it in shared mode, and > do the logging. Another idea would be to copy the values to local temp variables while holding the lock, and release the lock before calling elog() to avoid holding the lock over potential IO. -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-04-06 22:12 Melanie Plageman <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Melanie Plageman @ 2023-04-06 22:12 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Robert Haas <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Masahiko Sawada <[email protected]>; Kyotaro Horiguchi <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> On Thu, Apr 6, 2023 at 5:45 PM Daniel Gustafsson <[email protected]> wrote: > > > On 6 Apr 2023, at 23:06, Melanie Plageman <[email protected]> wrote: > > > Autovacuum workers, at the end of VacuumUpdateCosts(), check if cost > > limit or cost delay have been changed. If they have, they assert that > > they don't already hold the AutovacuumLock, take it in shared mode, and > > do the logging. > > Another idea would be to copy the values to local temp variables while holding > the lock, and release the lock before calling elog() to avoid holding the lock > over potential IO. Good idea. I've done this in attached v19. Also I looked through the docs and everything still looks correct for balancing algo. - Melanie Attachments: [text/x-patch] v19-0001-Make-vacuum-failsafe_active-global.patch (5.3K, ../../CAAKRu_Znk6WCCQqEw8hv3+X1Fm8gG0x4KKRAkKmO2JOcA-77-g@mail.gmail.com/2-v19-0001-Make-vacuum-failsafe_active-global.patch) download | inline diff: From 0042067ce72a474fe4087245b978847c0b835b72 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Fri, 31 Mar 2023 10:38:39 -0400 Subject: [PATCH v19 1/3] Make vacuum failsafe_active global While vacuuming a table in failsafe mode, VacuumCostActive should not be re-enabled. This currently isn't a problem because vacuum cost parameters are only refreshed in between vacuuming tables and failsafe status is reset for every table. In preparation for allowing vacuum cost parameters to be updated more frequently, elevate LVRelState->failsafe_active to a global, VacuumFailsafeActive, which will be checked when determining whether or not to re-enable vacuum cost-related delays. Reviewed-by: Masahiko Sawada <[email protected]> Reviewed-by: Daniel Gustafsson <[email protected]> Reviewed-by: Kyotaro Horiguchi <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7%2BDXhrjDA0gw%40mail.gmail.com --- src/backend/access/heap/vacuumlazy.c | 16 +++++++--------- src/backend/commands/vacuum.c | 15 +++++++++++++++ src/include/commands/vacuum.h | 1 + 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 639179aa46..2ba85bd3d6 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -153,8 +153,6 @@ typedef struct LVRelState bool aggressive; /* Use visibility map to skip? (disabled by DISABLE_PAGE_SKIPPING) */ bool skipwithvm; - /* Wraparound failsafe has been triggered? */ - bool failsafe_active; /* Consider index vacuuming bypass optimization? */ bool consider_bypass_optimization; @@ -391,7 +389,7 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, Assert(params->index_cleanup != VACOPTVALUE_UNSPECIFIED); Assert(params->truncate != VACOPTVALUE_UNSPECIFIED && params->truncate != VACOPTVALUE_AUTO); - vacrel->failsafe_active = false; + VacuumFailsafeActive = false; vacrel->consider_bypass_optimization = true; vacrel->do_index_vacuuming = true; vacrel->do_index_cleanup = true; @@ -709,7 +707,7 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, } else { - if (!vacrel->failsafe_active) + if (!VacuumFailsafeActive) appendStringInfoString(&buf, _("index scan bypassed: ")); else appendStringInfoString(&buf, _("index scan bypassed by failsafe: ")); @@ -2293,7 +2291,7 @@ lazy_vacuum(LVRelState *vacrel) * vacuuming or heap vacuuming. This VACUUM operation won't end up * back here again. */ - Assert(vacrel->failsafe_active); + Assert(VacuumFailsafeActive); } /* @@ -2374,7 +2372,7 @@ lazy_vacuum_all_indexes(LVRelState *vacrel) */ Assert(vacrel->num_index_scans > 0 || vacrel->dead_items->num_items == vacrel->lpdead_items); - Assert(allindexes || vacrel->failsafe_active); + Assert(allindexes || VacuumFailsafeActive); /* * Increase and report the number of index scans. @@ -2616,12 +2614,12 @@ static bool lazy_check_wraparound_failsafe(LVRelState *vacrel) { /* Don't warn more than once per VACUUM */ - if (vacrel->failsafe_active) + if (VacuumFailsafeActive) return true; if (unlikely(vacuum_xid_failsafe_check(&vacrel->cutoffs))) { - vacrel->failsafe_active = true; + VacuumFailsafeActive = true; /* * Abandon use of a buffer access strategy to allow use of all of @@ -2820,7 +2818,7 @@ should_attempt_truncation(LVRelState *vacrel) { BlockNumber possibly_freeable; - if (!vacrel->do_rel_truncate || vacrel->failsafe_active || + if (!vacrel->do_rel_truncate || VacuumFailsafeActive || old_snapshot_threshold >= 0) return false; diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index ea1d8960f4..7fc5c19e37 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -72,6 +72,21 @@ int vacuum_multixact_freeze_table_age; int vacuum_failsafe_age; int vacuum_multixact_failsafe_age; +/* + * VacuumFailsafeActive is a defined as a global so that we can determine + * whether or not to re-enable cost-based vacuum delay when vacuuming a table. + * If failsafe mode has been engaged, we will not re-enable cost-based delay + * for the table until after vacuuming has completed, regardless of other + * settings. + * + * Only VACUUM code should inspect this variable and only table access methods + * should set it to true. In Table AM-agnostic VACUUM code, this variable is + * inspected to determine whether or not to allow cost-based delays. Table AMs + * are free to set it if they desire this behavior, but it is false by default + * and reset to false in between vacuuming each relation. + */ +bool VacuumFailsafeActive = false; + /* * Variables for cost-based parallel vacuum. See comments atop * compute_parallel_delay to understand how it works. diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h index 19ca818dc2..1223d15e0d 100644 --- a/src/include/commands/vacuum.h +++ b/src/include/commands/vacuum.h @@ -306,6 +306,7 @@ extern PGDLLIMPORT pg_atomic_uint32 *VacuumSharedCostBalance; extern PGDLLIMPORT pg_atomic_uint32 *VacuumActiveNWorkers; extern PGDLLIMPORT int VacuumCostBalanceLocal; +extern PGDLLIMPORT bool VacuumFailsafeActive; /* in commands/vacuum.c */ extern void ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel); -- 2.37.2 [text/x-patch] v19-0003-Autovacuum-refreshes-cost-based-delay-params-mor.patch (22.3K, ../../CAAKRu_Znk6WCCQqEw8hv3+X1Fm8gG0x4KKRAkKmO2JOcA-77-g@mail.gmail.com/3-v19-0003-Autovacuum-refreshes-cost-based-delay-params-mor.patch) download | inline diff: From 3db6b900042f193892857ba7b2301a478ad0ef9a Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Sat, 25 Mar 2023 14:14:55 -0400 Subject: [PATCH v19 3/3] Autovacuum refreshes cost-based delay params more often Allow autovacuum to reload the config file more often so that cost-based delay parameters can take effect while VACUUMing a relation. Previously, autovacuum workers only reloaded the config file once per relation vacuumed, so config changes could not take effect until beginning to vacuum the next table. Now, check if a reload is pending roughly once per block, when checking if we need to delay. In order for autovacuum workers to safely update their own cost delay and cost limit parameters without impacting performance, we had to rethink when and how these values were accessed. Previously, an autovacuum worker's wi_cost_limit was set only at the beginning of vacuuming a table, after reloading the config file. Therefore, at the time that autovac_balance_cost() was called, workers vacuuming tables with no cost-related storage parameters could still have different values for their wi_cost_limit_base and wi_cost_delay. Now that the cost parameters can be updated while vacuuming a table, workers will (within some margin of error) have no reason to have different values for cost limit and cost delay (in the absence of cost-related storage parameters). This removes the rationale for keeping cost limit and cost delay in shared memory. Balancing the cost limit requires only the number of active autovacuum workers vacuuming a table with no cost-based storage parameters. Reviewed-by: Masahiko Sawada <[email protected]> Reviewed-by: Daniel Gustafsson <[email protected]> Reviewed-by: Kyotaro Horiguchi <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7%2BDXhrjDA0gw%40mail.gmail.com --- src/backend/access/heap/vacuumlazy.c | 2 +- src/backend/commands/vacuum.c | 46 +++- src/backend/commands/vacuumparallel.c | 1 - src/backend/postmaster/autovacuum.c | 293 ++++++++++++++++---------- src/include/commands/vacuum.h | 1 + 5 files changed, 221 insertions(+), 122 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 2ba85bd3d6..0a9ebd22bd 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -389,7 +389,7 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, Assert(params->index_cleanup != VACOPTVALUE_UNSPECIFIED); Assert(params->truncate != VACOPTVALUE_UNSPECIFIED && params->truncate != VACOPTVALUE_AUTO); - VacuumFailsafeActive = false; + Assert(!VacuumFailsafeActive); vacrel->consider_bypass_optimization = true; vacrel->do_index_vacuuming = true; vacrel->do_index_cleanup = true; diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index f2be74cdb5..ca347e0a6d 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -48,6 +48,7 @@ #include "pgstat.h" #include "postmaster/autovacuum.h" #include "postmaster/bgworker_internals.h" +#include "postmaster/interrupt.h" #include "storage/bufmgr.h" #include "storage/lmgr.h" #include "storage/pmsignal.h" @@ -523,9 +524,9 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy, { ListCell *cur; - VacuumUpdateCosts(); in_vacuum = true; - VacuumCostActive = (vacuum_cost_delay > 0); + VacuumFailsafeActive = false; + VacuumUpdateCosts(); VacuumCostBalance = 0; VacuumPageHit = 0; VacuumPageMiss = 0; @@ -579,12 +580,20 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy, CommandCounterIncrement(); } } + + /* + * Ensure VacuumFailsafeActive has been reset before vacuuming the + * next relation. + */ + VacuumFailsafeActive = false; } } PG_FINALLY(); { in_vacuum = false; VacuumCostActive = false; + VacuumFailsafeActive = false; + VacuumCostBalance = 0; } PG_END_TRY(); @@ -2245,7 +2254,28 @@ vacuum_delay_point(void) /* Always check for interrupts */ CHECK_FOR_INTERRUPTS(); - if (!VacuumCostActive || InterruptPending) + if (InterruptPending || + (!VacuumCostActive && !ConfigReloadPending)) + return; + + /* + * Autovacuum workers should reload the configuration file if requested. + * This allows changes to [autovacuum_]vacuum_cost_limit and + * [autovacuum_]vacuum_cost_delay to take effect while a table is being + * vacuumed or analyzed. + */ + if (ConfigReloadPending && IsAutoVacuumWorkerProcess()) + { + ConfigReloadPending = false; + ProcessConfigFile(PGC_SIGHUP); + VacuumUpdateCosts(); + } + + /* + * If we disabled cost-based delays after reloading the config file, + * return. + */ + if (!VacuumCostActive) return; /* @@ -2278,7 +2308,15 @@ vacuum_delay_point(void) VacuumCostBalance = 0; - VacuumUpdateCosts(); + /* + * Balance and update limit values for autovacuum workers. We must do + * this periodically, as the number of workers across which we are + * balancing the limit may have changed. + * + * XXX: There may be better criteria for determining when to do this + * besides "check after napping". + */ + AutoVacuumUpdateCostLimit(); /* Might have gotten an interrupt while sleeping */ CHECK_FOR_INTERRUPTS(); diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c index cc0aff7904..e200d5caf8 100644 --- a/src/backend/commands/vacuumparallel.c +++ b/src/backend/commands/vacuumparallel.c @@ -995,7 +995,6 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc) false); /* Set cost-based vacuum delay */ - VacuumCostActive = (vacuum_cost_delay > 0); VacuumUpdateCosts(); VacuumCostBalance = 0; VacuumPageHit = 0; diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index 3644b86443..17177c44c6 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -139,6 +139,18 @@ int Log_autovacuum_min_duration = 600000; static bool am_autovacuum_launcher = false; static bool am_autovacuum_worker = false; +/* + * Variables to save the cost-related storage parameters for the current + * relation being vacuumed by this autovacuum worker. Using these, we can + * ensure we don't overwrite the values of vacuum_cost_delay and + * vacuum_cost_limit after reloading the configuration file. They are + * initialized to "invalid" values to indicate that no cost-related storage + * parameters were specified and will be set in do_autovacuum() after checking + * the storage parameters in table_recheck_autovac(). + */ +static double av_storage_param_cost_delay = -1; +static int av_storage_param_cost_limit = -1; + /* Flags set by signal handlers */ static volatile sig_atomic_t got_SIGUSR2 = false; @@ -189,8 +201,8 @@ typedef struct autovac_table { Oid at_relid; VacuumParams at_params; - double at_vacuum_cost_delay; - int at_vacuum_cost_limit; + double at_storage_param_vac_cost_delay; + int at_storage_param_vac_cost_limit; bool at_dobalance; bool at_sharedrel; char *at_relname; @@ -209,7 +221,7 @@ typedef struct autovac_table * wi_sharedrel flag indicating whether table is marked relisshared * wi_proc pointer to PGPROC of the running worker, NULL if not started * wi_launchtime Time at which this worker was launched - * wi_cost_* Vacuum cost-based delay parameters current in this worker + * wi_dobalance Whether this worker should be included in balance calculations * * All fields are protected by AutovacuumLock, except for wi_tableoid and * wi_sharedrel which are protected by AutovacuumScheduleLock (note these @@ -223,11 +235,8 @@ typedef struct WorkerInfoData Oid wi_tableoid; PGPROC *wi_proc; TimestampTz wi_launchtime; - bool wi_dobalance; + pg_atomic_flag wi_dobalance; bool wi_sharedrel; - double wi_cost_delay; - int wi_cost_limit; - int wi_cost_limit_base; } WorkerInfoData; typedef struct WorkerInfoData *WorkerInfo; @@ -273,6 +282,8 @@ typedef struct AutoVacuumWorkItem * av_startingWorker pointer to WorkerInfo currently being started (cleared by * the worker itself as soon as it's up and running) * av_workItems work item array + * av_nworkersForBalance the number of autovacuum workers to use when + * calculating the per worker cost limit * * This struct is protected by AutovacuumLock, except for av_signal and parts * of the worker list (see above). @@ -286,6 +297,7 @@ typedef struct dlist_head av_runningWorkers; WorkerInfo av_startingWorker; AutoVacuumWorkItem av_workItems[NUM_WORKITEMS]; + pg_atomic_uint32 av_nworkersForBalance; } AutoVacuumShmemStruct; static AutoVacuumShmemStruct *AutoVacuumShmem; @@ -319,7 +331,7 @@ static void launch_worker(TimestampTz now); static List *get_database_list(void); static void rebuild_database_list(Oid newdb); static int db_comparator(const void *a, const void *b); -static void autovac_balance_cost(void); +static void autovac_recalculate_workers_for_balance(void); static void do_autovacuum(void); static void FreeWorkerInfo(int code, Datum arg); @@ -669,7 +681,7 @@ AutoVacLauncherMain(int argc, char *argv[]) { LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); AutoVacuumShmem->av_signal[AutoVacRebalance] = false; - autovac_balance_cost(); + autovac_recalculate_workers_for_balance(); LWLockRelease(AutovacuumLock); } @@ -818,11 +830,6 @@ HandleAutoVacLauncherInterrupts(void) if (!AutoVacuumingActive()) AutoVacLauncherShutdown(); - /* rebalance in case the default cost parameters changed */ - LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); - autovac_balance_cost(); - LWLockRelease(AutovacuumLock); - /* rebuild the list in case the naptime changed */ rebuild_database_list(InvalidOid); } @@ -1754,10 +1761,7 @@ FreeWorkerInfo(int code, Datum arg) MyWorkerInfo->wi_sharedrel = false; MyWorkerInfo->wi_proc = NULL; MyWorkerInfo->wi_launchtime = 0; - MyWorkerInfo->wi_dobalance = false; - MyWorkerInfo->wi_cost_delay = 0; - MyWorkerInfo->wi_cost_limit = 0; - MyWorkerInfo->wi_cost_limit_base = 0; + pg_atomic_clear_flag(&MyWorkerInfo->wi_dobalance); dlist_push_head(&AutoVacuumShmem->av_freeWorkers, &MyWorkerInfo->wi_links); /* not mine anymore */ @@ -1781,10 +1785,20 @@ FreeWorkerInfo(int code, Datum arg) void VacuumUpdateCosts(void) { + double original_cost_delay = vacuum_cost_delay; + int original_cost_limit = vacuum_cost_limit; + if (MyWorkerInfo) { - vacuum_cost_delay = MyWorkerInfo->wi_cost_delay; - vacuum_cost_limit = MyWorkerInfo->wi_cost_limit; + if (av_storage_param_cost_delay >= 0) + vacuum_cost_delay = av_storage_param_cost_delay; + else if (autovacuum_vac_cost_delay >= 0) + vacuum_cost_delay = autovacuum_vac_cost_delay; + else + /* fall back to VacuumCostDelay */ + vacuum_cost_delay = VacuumCostDelay; + + AutoVacuumUpdateCostLimit(); } else { @@ -1792,88 +1806,128 @@ VacuumUpdateCosts(void) vacuum_cost_delay = VacuumCostDelay; vacuum_cost_limit = VacuumCostLimit; } + + /* + * If configuration changes are allowed to impact VacuumCostActive, make + * sure it is updated. + */ + if (VacuumFailsafeActive) + Assert(!VacuumCostActive); + else if (vacuum_cost_delay > 0) + VacuumCostActive = true; + else + { + VacuumCostActive = false; + VacuumCostBalance = 0; + } + + if (MyWorkerInfo) + { + Oid dboid, + tableoid; + + /* Only log updates to cost-related variables */ + if (vacuum_cost_delay == original_cost_delay && + vacuum_cost_limit == original_cost_limit) + return; + + Assert(!LWLockHeldByMe(AutovacuumLock)); + + LWLockAcquire(AutovacuumLock, LW_SHARED); + dboid = MyWorkerInfo->wi_dboid; + tableoid = MyWorkerInfo->wi_tableoid; + LWLockRelease(AutovacuumLock); + + elog(DEBUG2, + "Autovacuum VacuumUpdateCosts(db=%u, rel=%u, dobalance=%s, cost_limit=%d, cost_delay=%g active=%s failsafe=%s)", + dboid, tableoid, pg_atomic_unlocked_test_flag(&MyWorkerInfo->wi_dobalance) ? "no" : "yes", + vacuum_cost_limit, vacuum_cost_delay, + vacuum_cost_delay > 0 ? "yes" : "no", + VacuumFailsafeActive ? "yes" : "no"); + + } } /* - * autovac_balance_cost - * Recalculate the cost limit setting for each active worker. - * - * Caller must hold the AutovacuumLock in exclusive mode. + * Update vacuum_cost_limit with the correct value for an autovacuum worker, + * given the value of other relevant cost limit parameters and the number of + * workers across which the limit must be balanced. Autovacuum workers must + * call this regularly in case av_nworkersForBalance has been updated by + * another worker or by the autovacuum launcher. They must also call it after a + * config reload. */ -static void -autovac_balance_cost(void) +void +AutoVacuumUpdateCostLimit(void) { + if (!MyWorkerInfo) + return; + /* - * The idea here is that we ration out I/O equally. The amount of I/O - * that a worker can consume is determined by cost_limit/cost_delay, so we - * try to equalize those ratios rather than the raw limit settings. - * * note: in cost_limit, zero also means use value from elsewhere, because * zero is not a valid value. */ - int vac_cost_limit = (autovacuum_vac_cost_limit > 0 ? - autovacuum_vac_cost_limit : VacuumCostLimit); - double vac_cost_delay = (autovacuum_vac_cost_delay >= 0 ? - autovacuum_vac_cost_delay : VacuumCostDelay); - double cost_total; - double cost_avail; - dlist_iter iter; - - /* not set? nothing to do */ - if (vac_cost_limit <= 0 || vac_cost_delay <= 0) - return; - /* calculate the total base cost limit of participating active workers */ - cost_total = 0.0; - dlist_foreach(iter, &AutoVacuumShmem->av_runningWorkers) + if (av_storage_param_cost_limit > 0) + vacuum_cost_limit = av_storage_param_cost_limit; + else { - WorkerInfo worker = dlist_container(WorkerInfoData, wi_links, iter.cur); + int nworkers_for_balance; + + if (autovacuum_vac_cost_limit > 0) + vacuum_cost_limit = autovacuum_vac_cost_limit; + else + vacuum_cost_limit = VacuumCostLimit; + + /* Only balance limit if no cost-related storage parameters specified */ + if (pg_atomic_unlocked_test_flag(&MyWorkerInfo->wi_dobalance)) + return; + + Assert(vacuum_cost_limit > 0); - if (worker->wi_proc != NULL && - worker->wi_dobalance && - worker->wi_cost_limit_base > 0 && worker->wi_cost_delay > 0) - cost_total += - (double) worker->wi_cost_limit_base / worker->wi_cost_delay; + nworkers_for_balance = pg_atomic_read_u32(&AutoVacuumShmem->av_nworkersForBalance); + + /* There is at least 1 autovac worker (this worker) */ + if (nworkers_for_balance <= 0) + elog(ERROR, "nworkers_for_balance must be > 0"); + + vacuum_cost_limit = Max(vacuum_cost_limit / nworkers_for_balance, 1); } +} - /* there are no cost limits -- nothing to do */ - if (cost_total <= 0) - return; +/* + * autovac_recalculate_workers_for_balance + * Recalculate the number of workers to consider, given cost-related + * storage parameters and the current number of active workers. + * + * Caller must hold the AutovacuumLock in at least shared mode to access + * worker->wi_proc. + */ +static void +autovac_recalculate_workers_for_balance(void) +{ + dlist_iter iter; + int orig_nworkers_for_balance; + int nworkers_for_balance = 0; + + Assert(LWLockHeldByMe(AutovacuumLock)); + + orig_nworkers_for_balance = + pg_atomic_read_u32(&AutoVacuumShmem->av_nworkersForBalance); - /* - * Adjust cost limit of each active worker to balance the total of cost - * limit to autovacuum_vacuum_cost_limit. - */ - cost_avail = (double) vac_cost_limit / vac_cost_delay; dlist_foreach(iter, &AutoVacuumShmem->av_runningWorkers) { WorkerInfo worker = dlist_container(WorkerInfoData, wi_links, iter.cur); - if (worker->wi_proc != NULL && - worker->wi_dobalance && - worker->wi_cost_limit_base > 0 && worker->wi_cost_delay > 0) - { - int limit = (int) - (cost_avail * worker->wi_cost_limit_base / cost_total); - - /* - * We put a lower bound of 1 on the cost_limit, to avoid division- - * by-zero in the vacuum code. Also, in case of roundoff trouble - * in these calculations, let's be sure we don't ever set - * cost_limit to more than the base value. - */ - worker->wi_cost_limit = Max(Min(limit, - worker->wi_cost_limit_base), - 1); - } + if (worker->wi_proc == NULL || + pg_atomic_unlocked_test_flag(&worker->wi_dobalance)) + continue; - if (worker->wi_proc != NULL) - elog(DEBUG2, "autovac_balance_cost(pid=%d db=%u, rel=%u, dobalance=%s cost_limit=%d, cost_limit_base=%d, cost_delay=%g)", - worker->wi_proc->pid, worker->wi_dboid, worker->wi_tableoid, - worker->wi_dobalance ? "yes" : "no", - worker->wi_cost_limit, worker->wi_cost_limit_base, - worker->wi_cost_delay); + nworkers_for_balance++; } + + if (nworkers_for_balance != orig_nworkers_for_balance) + pg_atomic_write_u32(&AutoVacuumShmem->av_nworkersForBalance, + nworkers_for_balance); } /* @@ -2421,23 +2475,34 @@ do_autovacuum(void) continue; } - /* Must hold AutovacuumLock while mucking with cost balance info */ - LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); + /* + * Save the cost-related storage parameter values in global variables + * for reference when updating vacuum_cost_delay and vacuum_cost_limit + * during vacuuming this table. + */ + av_storage_param_cost_delay = tab->at_storage_param_vac_cost_delay; + av_storage_param_cost_limit = tab->at_storage_param_vac_cost_limit; - /* advertise my cost delay parameters for the balancing algorithm */ - MyWorkerInfo->wi_dobalance = tab->at_dobalance; - MyWorkerInfo->wi_cost_delay = tab->at_vacuum_cost_delay; - MyWorkerInfo->wi_cost_limit = tab->at_vacuum_cost_limit; - MyWorkerInfo->wi_cost_limit_base = tab->at_vacuum_cost_limit; + /* + * We only expect this worker to ever set the flag, so don't bother + * checking the return value. We shouldn't have to retry. + */ + if (tab->at_dobalance) + pg_atomic_test_set_flag(&MyWorkerInfo->wi_dobalance); + else + pg_atomic_clear_flag(&MyWorkerInfo->wi_dobalance); - /* do a balance */ - autovac_balance_cost(); + LWLockAcquire(AutovacuumLock, LW_SHARED); + autovac_recalculate_workers_for_balance(); + LWLockRelease(AutovacuumLock); - /* set the active cost parameters from the result of that */ + /* + * We wait until this point to update cost delay and cost limit + * values, even though we reloaded the configuration file above, so + * that we can take into account the cost-related storage parameters. + */ VacuumUpdateCosts(); - /* done */ - LWLockRelease(AutovacuumLock); /* clean up memory before each iteration */ MemoryContextResetAndDeleteChildren(PortalContext); @@ -2521,16 +2586,17 @@ deleted: pfree(tab); /* - * Remove my info from shared memory. We could, but intentionally - * don't, clear wi_cost_limit and friends --- this is on the - * assumption that we probably have more to do with similar cost - * settings, so we don't want to give up our share of I/O for a very - * short interval and thereby thrash the global balance. + * Remove my info from shared memory. We set wi_dobalance on the + * assumption that we are more likely than not to vacuum a table with + * no cost-related storage parameters next, so we want to claim our + * share of I/O as soon as possible to avoid thrashing the global + * balance. */ LWLockAcquire(AutovacuumScheduleLock, LW_EXCLUSIVE); MyWorkerInfo->wi_tableoid = InvalidOid; MyWorkerInfo->wi_sharedrel = false; LWLockRelease(AutovacuumScheduleLock); + pg_atomic_test_set_flag(&MyWorkerInfo->wi_dobalance); } /* @@ -2562,6 +2628,7 @@ deleted: { ConfigReloadPending = false; ProcessConfigFile(PGC_SIGHUP); + VacuumUpdateCosts(); } LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); @@ -2797,8 +2864,6 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map, int freeze_table_age; int multixact_freeze_min_age; int multixact_freeze_table_age; - int vac_cost_limit; - double vac_cost_delay; int log_min_duration; /* @@ -2808,20 +2873,6 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map, * defaults, autovacuum's own first and plain vacuum second. */ - /* -1 in autovac setting means use plain vacuum_cost_delay */ - vac_cost_delay = (avopts && avopts->vacuum_cost_delay >= 0) - ? avopts->vacuum_cost_delay - : (autovacuum_vac_cost_delay >= 0) - ? autovacuum_vac_cost_delay - : VacuumCostDelay; - - /* 0 or -1 in autovac setting means use plain vacuum_cost_limit */ - vac_cost_limit = (avopts && avopts->vacuum_cost_limit > 0) - ? avopts->vacuum_cost_limit - : (autovacuum_vac_cost_limit > 0) - ? autovacuum_vac_cost_limit - : VacuumCostLimit; - /* -1 in autovac setting means use log_autovacuum_min_duration */ log_min_duration = (avopts && avopts->log_min_duration >= 0) ? avopts->log_min_duration @@ -2877,8 +2928,10 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map, tab->at_params.multixact_freeze_table_age = multixact_freeze_table_age; tab->at_params.is_wraparound = wraparound; tab->at_params.log_min_duration = log_min_duration; - tab->at_vacuum_cost_limit = vac_cost_limit; - tab->at_vacuum_cost_delay = vac_cost_delay; + tab->at_storage_param_vac_cost_limit = avopts ? + avopts->vacuum_cost_limit : 0; + tab->at_storage_param_vac_cost_delay = avopts ? + avopts->vacuum_cost_delay : -1; tab->at_relname = NULL; tab->at_nspname = NULL; tab->at_datname = NULL; @@ -3380,10 +3433,18 @@ AutoVacuumShmemInit(void) worker = (WorkerInfo) ((char *) AutoVacuumShmem + MAXALIGN(sizeof(AutoVacuumShmemStruct))); + /* initialize the WorkerInfo free list */ for (i = 0; i < autovacuum_max_workers; i++) + { dlist_push_head(&AutoVacuumShmem->av_freeWorkers, &worker[i].wi_links); + + pg_atomic_init_flag(&worker[i].wi_dobalance); + } + + pg_atomic_init_u32(&AutoVacuumShmem->av_nworkersForBalance, 0); + } else Assert(found); diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h index 50caf1315d..2a856b0e5e 100644 --- a/src/include/commands/vacuum.h +++ b/src/include/commands/vacuum.h @@ -350,6 +350,7 @@ extern IndexBulkDeleteResult *vac_cleanup_one_index(IndexVacuumInfo *ivinfo, extern Size vac_max_items_to_alloc_size(int max_items); /* In postmaster/autovacuum.c */ +extern void AutoVacuumUpdateCostLimit(void); extern void VacuumUpdateCosts(void); /* in commands/vacuumparallel.c */ -- 2.37.2 [text/x-patch] v19-0002-Separate-vacuum-cost-variables-from-GUCs.patch (8.7K, ../../CAAKRu_Znk6WCCQqEw8hv3+X1Fm8gG0x4KKRAkKmO2JOcA-77-g@mail.gmail.com/4-v19-0002-Separate-vacuum-cost-variables-from-GUCs.patch) download | inline diff: From 92fc87969a496c1a1f5fd612d3c7c32251dec6e1 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Thu, 6 Apr 2023 16:02:12 -0400 Subject: [PATCH v19 2/3] Separate vacuum cost variables from GUCs Vacuum code run both by autovacuum workers and a backend doing VACUUM/ANALYZE previously inspected VacuumCostLimit and VacuumCostDelay, which are the global variables backing the GUCs vacuum_cost_limit and vacuum_cost_delay. Autovacuum workers needed to override these variables with their own values, derived from autovacuum_vacuum_cost_limit and autovacuum_vacuum_cost_delay and worker cost limit balancing logic. This led to confusing code which, in some cases, both derived and set a new value of VacuumCostLimit from VacuumCostLimit. In preparation for refreshing these GUC values more often, introduce new, independent global variables and add a function to update them using the GUCs and existing logic. Per suggestion by Kyotaro Horiguchi Reviewed-by: Masahiko Sawada <[email protected]> Reviewed-by: Daniel Gustafsson <[email protected]> Reviewed-by: Kyotaro Horiguchi <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7%2BDXhrjDA0gw%40mail.gmail.com --- src/backend/commands/vacuum.c | 29 +++++++++++++++-------- src/backend/commands/vacuumparallel.c | 3 ++- src/backend/postmaster/autovacuum.c | 34 +++++++++++---------------- src/include/commands/vacuum.h | 5 ++++ src/include/postmaster/autovacuum.h | 3 --- 5 files changed, 40 insertions(+), 34 deletions(-) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 7fc5c19e37..f2be74cdb5 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -72,6 +72,15 @@ int vacuum_multixact_freeze_table_age; int vacuum_failsafe_age; int vacuum_multixact_failsafe_age; +/* + * Variables for cost-based vacuum delay. The defaults differ between + * autovacuum and vacuum. They should be set with the appropriate GUC value in + * vacuum code. They are initialized here to the defaults for client backends + * executing VACUUM or ANALYZE. + */ +double vacuum_cost_delay = 0; +int vacuum_cost_limit = 200; + /* * VacuumFailsafeActive is a defined as a global so that we can determine * whether or not to re-enable cost-based vacuum delay when vacuuming a table. @@ -514,8 +523,9 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy, { ListCell *cur; + VacuumUpdateCosts(); in_vacuum = true; - VacuumCostActive = (VacuumCostDelay > 0); + VacuumCostActive = (vacuum_cost_delay > 0); VacuumCostBalance = 0; VacuumPageHit = 0; VacuumPageMiss = 0; @@ -2244,14 +2254,14 @@ vacuum_delay_point(void) */ if (VacuumSharedCostBalance != NULL) msec = compute_parallel_delay(); - else if (VacuumCostBalance >= VacuumCostLimit) - msec = VacuumCostDelay * VacuumCostBalance / VacuumCostLimit; + else if (VacuumCostBalance >= vacuum_cost_limit) + msec = vacuum_cost_delay * VacuumCostBalance / vacuum_cost_limit; /* Nap if appropriate */ if (msec > 0) { - if (msec > VacuumCostDelay * 4) - msec = VacuumCostDelay * 4; + if (msec > vacuum_cost_delay * 4) + msec = vacuum_cost_delay * 4; pgstat_report_wait_start(WAIT_EVENT_VACUUM_DELAY); pg_usleep(msec * 1000); @@ -2268,8 +2278,7 @@ vacuum_delay_point(void) VacuumCostBalance = 0; - /* update balance values for workers */ - AutoVacuumUpdateDelay(); + VacuumUpdateCosts(); /* Might have gotten an interrupt while sleeping */ CHECK_FOR_INTERRUPTS(); @@ -2319,11 +2328,11 @@ compute_parallel_delay(void) /* Compute the total local balance for the current worker */ VacuumCostBalanceLocal += VacuumCostBalance; - if ((shared_balance >= VacuumCostLimit) && - (VacuumCostBalanceLocal > 0.5 * ((double) VacuumCostLimit / nworkers))) + if ((shared_balance >= vacuum_cost_limit) && + (VacuumCostBalanceLocal > 0.5 * ((double) vacuum_cost_limit / nworkers))) { /* Compute sleep time based on the local cost balance */ - msec = VacuumCostDelay * VacuumCostBalanceLocal / VacuumCostLimit; + msec = vacuum_cost_delay * VacuumCostBalanceLocal / vacuum_cost_limit; pg_atomic_sub_fetch_u32(VacuumSharedCostBalance, VacuumCostBalanceLocal); VacuumCostBalanceLocal = 0; } diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c index 563117a8f6..cc0aff7904 100644 --- a/src/backend/commands/vacuumparallel.c +++ b/src/backend/commands/vacuumparallel.c @@ -995,7 +995,8 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc) false); /* Set cost-based vacuum delay */ - VacuumCostActive = (VacuumCostDelay > 0); + VacuumCostActive = (vacuum_cost_delay > 0); + VacuumUpdateCosts(); VacuumCostBalance = 0; VacuumPageHit = 0; VacuumPageMiss = 0; diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index c1e911b1b3..3644b86443 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -1773,16 +1773,24 @@ FreeWorkerInfo(int code, Datum arg) } /* - * Update the cost-based delay parameters, so that multiple workers consume - * each a fraction of the total available I/O. + * Update vacuum cost-based delay-related parameters for autovacuum workers and + * backends executing VACUUM or ANALYZE using the value of relevant gucs and + * global state. This must be called during setup for vacuum and after every + * config reload to ensure up-to-date values. */ void -AutoVacuumUpdateDelay(void) +VacuumUpdateCosts(void) { if (MyWorkerInfo) { - VacuumCostDelay = MyWorkerInfo->wi_cost_delay; - VacuumCostLimit = MyWorkerInfo->wi_cost_limit; + vacuum_cost_delay = MyWorkerInfo->wi_cost_delay; + vacuum_cost_limit = MyWorkerInfo->wi_cost_limit; + } + else + { + /* Must be explicit VACUUM or ANALYZE */ + vacuum_cost_delay = VacuumCostDelay; + vacuum_cost_limit = VacuumCostLimit; } } @@ -2311,8 +2319,6 @@ do_autovacuum(void) autovac_table *tab; bool isshared; bool skipit; - double stdVacuumCostDelay; - int stdVacuumCostLimit; dlist_iter iter; CHECK_FOR_INTERRUPTS(); @@ -2415,14 +2421,6 @@ do_autovacuum(void) continue; } - /* - * Remember the prevailing values of the vacuum cost GUCs. We have to - * restore these at the bottom of the loop, else we'll compute wrong - * values in the next iteration of autovac_balance_cost(). - */ - stdVacuumCostDelay = VacuumCostDelay; - stdVacuumCostLimit = VacuumCostLimit; - /* Must hold AutovacuumLock while mucking with cost balance info */ LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); @@ -2436,7 +2434,7 @@ do_autovacuum(void) autovac_balance_cost(); /* set the active cost parameters from the result of that */ - AutoVacuumUpdateDelay(); + VacuumUpdateCosts(); /* done */ LWLockRelease(AutovacuumLock); @@ -2533,10 +2531,6 @@ deleted: MyWorkerInfo->wi_tableoid = InvalidOid; MyWorkerInfo->wi_sharedrel = false; LWLockRelease(AutovacuumScheduleLock); - - /* restore vacuum cost GUCs for the next iteration */ - VacuumCostDelay = stdVacuumCostDelay; - VacuumCostLimit = stdVacuumCostLimit; } /* diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h index 1223d15e0d..50caf1315d 100644 --- a/src/include/commands/vacuum.h +++ b/src/include/commands/vacuum.h @@ -307,6 +307,8 @@ extern PGDLLIMPORT pg_atomic_uint32 *VacuumActiveNWorkers; extern PGDLLIMPORT int VacuumCostBalanceLocal; extern PGDLLIMPORT bool VacuumFailsafeActive; +extern PGDLLIMPORT double vacuum_cost_delay; +extern PGDLLIMPORT int vacuum_cost_limit; /* in commands/vacuum.c */ extern void ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel); @@ -347,6 +349,9 @@ extern IndexBulkDeleteResult *vac_cleanup_one_index(IndexVacuumInfo *ivinfo, IndexBulkDeleteResult *istat); extern Size vac_max_items_to_alloc_size(int max_items); +/* In postmaster/autovacuum.c */ +extern void VacuumUpdateCosts(void); + /* in commands/vacuumparallel.c */ extern ParallelVacuumState *parallel_vacuum_init(Relation rel, Relation *indrels, int nindexes, int nrequested_workers, diff --git a/src/include/postmaster/autovacuum.h b/src/include/postmaster/autovacuum.h index c140371b51..65afd1ea1e 100644 --- a/src/include/postmaster/autovacuum.h +++ b/src/include/postmaster/autovacuum.h @@ -63,9 +63,6 @@ extern int StartAutoVacWorker(void); /* called from postmaster when a worker could not be forked */ extern void AutoVacWorkerFailed(void); -/* autovacuum cost-delay balancer */ -extern void AutoVacuumUpdateDelay(void); - #ifdef EXEC_BACKEND extern void AutoVacLauncherMain(int argc, char *argv[]) pg_attribute_noreturn(); extern void AutoVacWorkerMain(int argc, char *argv[]) pg_attribute_noreturn(); -- 2.37.2 ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-04-06 23:08 Daniel Gustafsson <[email protected]> parent: Melanie Plageman <[email protected]> 0 siblings, 2 replies; 26+ messages in thread From: Daniel Gustafsson @ 2023-04-06 23:08 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Robert Haas <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Masahiko Sawada <[email protected]>; Kyotaro Horiguchi <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> > On 7 Apr 2023, at 00:12, Melanie Plageman <[email protected]> wrote: > > On Thu, Apr 6, 2023 at 5:45 PM Daniel Gustafsson <[email protected]> wrote: >> >>> On 6 Apr 2023, at 23:06, Melanie Plageman <[email protected]> wrote: >> >>> Autovacuum workers, at the end of VacuumUpdateCosts(), check if cost >>> limit or cost delay have been changed. If they have, they assert that >>> they don't already hold the AutovacuumLock, take it in shared mode, and >>> do the logging. >> >> Another idea would be to copy the values to local temp variables while holding >> the lock, and release the lock before calling elog() to avoid holding the lock >> over potential IO. > > Good idea. I've done this in attached v19. > Also I looked through the docs and everything still looks correct for > balancing algo. I had another read-through and test-through of this version, and have applied it with some minor changes to comments and whitespace. Thanks for the quick turnaround times on reviews in this thread! I opted for keeping the three individual commits, squashing them didn't seem helpful enough to future commitlog readers and no other combination of the three made more sense than what has been in the thread. -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-04-07 06:52 Masahiko Sawada <[email protected]> parent: Daniel Gustafsson <[email protected]> 1 sibling, 2 replies; 26+ messages in thread From: Masahiko Sawada @ 2023-04-07 06:52 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Melanie Plageman <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> On Fri, Apr 7, 2023 at 8:08 AM Daniel Gustafsson <[email protected]> wrote: > > > On 7 Apr 2023, at 00:12, Melanie Plageman <[email protected]> wrote: > > > > On Thu, Apr 6, 2023 at 5:45 PM Daniel Gustafsson <[email protected]> wrote: > >> > >>> On 6 Apr 2023, at 23:06, Melanie Plageman <[email protected]> wrote: > >> > >>> Autovacuum workers, at the end of VacuumUpdateCosts(), check if cost > >>> limit or cost delay have been changed. If they have, they assert that > >>> they don't already hold the AutovacuumLock, take it in shared mode, and > >>> do the logging. > >> > >> Another idea would be to copy the values to local temp variables while holding > >> the lock, and release the lock before calling elog() to avoid holding the lock > >> over potential IO. > > > > Good idea. I've done this in attached v19. > > Also I looked through the docs and everything still looks correct for > > balancing algo. > > I had another read-through and test-through of this version, and have applied > it with some minor changes to comments and whitespace. Thanks for the quick > turnaround times on reviews in this thread! Cool! Regarding the commit 7d71d3dd08, I have one comment: + /* Only log updates to cost-related variables */ + if (vacuum_cost_delay == original_cost_delay && + vacuum_cost_limit == original_cost_limit) + return; IIUC by default, we log not only before starting the vacuum but also when changing cost-related variables. Which is good, I think, because logging the initial values would also be helpful for investigation. However, I think that we don't log the initial vacuum cost values depending on the values. For example, if the autovacuum_vacuum_cost_delay storage option is set to 0, we don't log the initial values. I think that instead of comparing old and new values, we can write the log only if message_level_is_interesting(DEBUG2) is true. That way, we don't need to acquire the lwlock unnecessarily. And the code looks cleaner to me. I've attached the patch (use_message_level_is_interesting.patch) Also, while testing the autovacuum delay with relopt autovacuum_vacuum_cost_delay = 0, I realized that even if we set autovacuum_vacuum_cost_delay = 0 to a table, wi_dobalance is set to true. wi_dobalance comes from the following expression: /* * If any of the cost delay parameters has been set individually for * this table, disable the balancing algorithm. */ tab->at_dobalance = !(avopts && (avopts->vacuum_cost_limit > 0 || avopts->vacuum_cost_delay > 0)); The initial values of both avopts->vacuum_cost_limit and avopts->vacuum_cost_delay are -1. I think we should use ">= 0" instead of "> 0". Otherwise, we include the autovacuum worker working on a table whose autovacuum_vacuum_cost_delay is 0 to the balancing algorithm. Probably this behavior has existed also on back branches but I haven't checked it yet. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com Attachments: [application/octet-stream] fix.patch (584B, ../../CAD21AoBS7o6Ljt_vfqPQPf67AhzKu3fR0iqk8B=vVYczMugKMQ@mail.gmail.com/2-fix.patch) download | inline diff: diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index 53c8f8d79c..2036b39ad5 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -2951,8 +2951,8 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map, * this table, disable the balancing algorithm. */ tab->at_dobalance = - !(avopts && (avopts->vacuum_cost_limit > 0 || - avopts->vacuum_cost_delay > 0)); + !(avopts && (avopts->vacuum_cost_limit >= 0 || + avopts->vacuum_cost_delay >= 0)); } heap_freetuple(classTup); [application/octet-stream] use_message_level_is_interesting.patch (918B, ../../CAD21AoBS7o6Ljt_vfqPQPf67AhzKu3fR0iqk8B=vVYczMugKMQ@mail.gmail.com/3-use_message_level_is_interesting.patch) download | inline diff: diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index 53c8f8d79c..44497f6734 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -1785,9 +1785,6 @@ FreeWorkerInfo(int code, Datum arg) void VacuumUpdateCosts(void) { - double original_cost_delay = vacuum_cost_delay; - int original_cost_limit = vacuum_cost_limit; - if (MyWorkerInfo) { if (av_storage_param_cost_delay >= 0) @@ -1821,16 +1818,11 @@ VacuumUpdateCosts(void) VacuumCostBalance = 0; } - if (MyWorkerInfo) + if (MyWorkerInfo && message_level_is_interesting(DEBUG2)) { Oid dboid, tableoid; - /* Only log updates to cost-related variables */ - if (vacuum_cost_delay == original_cost_delay && - vacuum_cost_limit == original_cost_limit) - return; - Assert(!LWLockHeldByMe(AutovacuumLock)); LWLockAcquire(AutovacuumLock, LW_SHARED); ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-04-07 11:28 Daniel Gustafsson <[email protected]> parent: Masahiko Sawada <[email protected]> 1 sibling, 0 replies; 26+ messages in thread From: Daniel Gustafsson @ 2023-04-07 11:28 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: Melanie Plageman <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> > On 7 Apr 2023, at 08:52, Masahiko Sawada <[email protected]> wrote: > On Fri, Apr 7, 2023 at 8:08 AM Daniel Gustafsson <[email protected]> wrote: >> I had another read-through and test-through of this version, and have applied >> it with some minor changes to comments and whitespace. Thanks for the quick >> turnaround times on reviews in this thread! > > Cool! > > Regarding the commit 7d71d3dd08, I have one comment: > > + /* Only log updates to cost-related variables */ > + if (vacuum_cost_delay == original_cost_delay && > + vacuum_cost_limit == original_cost_limit) > + return; > > IIUC by default, we log not only before starting the vacuum but also > when changing cost-related variables. Which is good, I think, because > logging the initial values would also be helpful for investigation. > However, I think that we don't log the initial vacuum cost values > depending on the values. For example, if the > autovacuum_vacuum_cost_delay storage option is set to 0, we don't log > the initial values. I think that instead of comparing old and new > values, we can write the log only if > message_level_is_interesting(DEBUG2) is true. That way, we don't need > to acquire the lwlock unnecessarily. And the code looks cleaner to me. > I've attached the patch (use_message_level_is_interesting.patch) That's a good idea, unless Melanie has conflicting opinions I think we should go ahead with this. Avoiding taking a lock here is a good save. > Also, while testing the autovacuum delay with relopt > autovacuum_vacuum_cost_delay = 0, I realized that even if we set > autovacuum_vacuum_cost_delay = 0 to a table, wi_dobalance is set to > true. wi_dobalance comes from the following expression: > > /* > * If any of the cost delay parameters has been set individually for > * this table, disable the balancing algorithm. > */ > tab->at_dobalance = > !(avopts && (avopts->vacuum_cost_limit > 0 || > avopts->vacuum_cost_delay > 0)); > > The initial values of both avopts->vacuum_cost_limit and > avopts->vacuum_cost_delay are -1. I think we should use ">= 0" instead > of "> 0". Otherwise, we include the autovacuum worker working on a > table whose autovacuum_vacuum_cost_delay is 0 to the balancing > algorithm. Probably this behavior has existed also on back branches > but I haven't checked it yet. Interesting, good find. Looking quickly at the back branches I think there is a variant of this for vacuum_cost_limit even there but needs more investigation. -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-04-07 13:07 Melanie Plageman <[email protected]> parent: Masahiko Sawada <[email protected]> 1 sibling, 2 replies; 26+ messages in thread From: Melanie Plageman @ 2023-04-07 13:07 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> On Fri, Apr 7, 2023 at 2:53 AM Masahiko Sawada <[email protected]> wrote: > > On Fri, Apr 7, 2023 at 8:08 AM Daniel Gustafsson <[email protected]> wrote: > > > > > On 7 Apr 2023, at 00:12, Melanie Plageman <[email protected]> wrote: > > > > > > On Thu, Apr 6, 2023 at 5:45 PM Daniel Gustafsson <[email protected]> wrote: > > >> > > >>> On 6 Apr 2023, at 23:06, Melanie Plageman <[email protected]> wrote: > > >> > > >>> Autovacuum workers, at the end of VacuumUpdateCosts(), check if cost > > >>> limit or cost delay have been changed. If they have, they assert that > > >>> they don't already hold the AutovacuumLock, take it in shared mode, and > > >>> do the logging. > > >> > > >> Another idea would be to copy the values to local temp variables while holding > > >> the lock, and release the lock before calling elog() to avoid holding the lock > > >> over potential IO. > > > > > > Good idea. I've done this in attached v19. > > > Also I looked through the docs and everything still looks correct for > > > balancing algo. > > > > I had another read-through and test-through of this version, and have applied > > it with some minor changes to comments and whitespace. Thanks for the quick > > turnaround times on reviews in this thread! > > Cool! > > Regarding the commit 7d71d3dd08, I have one comment: > > + /* Only log updates to cost-related variables */ > + if (vacuum_cost_delay == original_cost_delay && > + vacuum_cost_limit == original_cost_limit) > + return; > > IIUC by default, we log not only before starting the vacuum but also > when changing cost-related variables. Which is good, I think, because > logging the initial values would also be helpful for investigation. > However, I think that we don't log the initial vacuum cost values > depending on the values. For example, if the > autovacuum_vacuum_cost_delay storage option is set to 0, we don't log > the initial values. I think that instead of comparing old and new > values, we can write the log only if > message_level_is_interesting(DEBUG2) is true. That way, we don't need > to acquire the lwlock unnecessarily. And the code looks cleaner to me. > I've attached the patch (use_message_level_is_interesting.patch) Thanks for coming up with the case you thought of with storage param for cost delay = 0. In that case we wouldn't print the message initially and we should fix that. I disagree, however, that we should condition it only on message_level_is_interesting(). Actually, outside of printing initial values when the autovacuum worker first starts (before vacuuming all tables), I don't think we should log these values except when they are being updated. Autovacuum workers could vacuum tons of small tables and having this print out at least once per table (which I know is how it is on master) would be distracting. Also, you could be reloading the config to update some other GUCs and be oblivious to an ongoing autovacuum and get these messages printed out, which I would also find distracting. You will have to stare very hard at the logs to tell if your changes to vacuum cost delay and limit took effect when you reload config. I think with our changes to update the values more often, we should take the opportunity to make this logging more useful by making it happen only when the values are changed. I would be open to elevating the log level to DEBUG1 for logging only updates and, perhaps, having an option if you set log level to DEBUG2, for example, to always log these values in VacuumUpdateCosts(). I'd even argue that, potentially, having the cost-delay related parameters printed at the beginning of vacuuming could be interesting to regular VACUUM as well (even though it doesn't benefit from config reload while in progress). To fix the issue you mentioned and ensure the logging is printed when autovacuum workers start up before vacuuming tables, we could either initialize vacuum_cost_delay and vacuum_cost_limit to something invalid that will always be different than what they are set to in VacuumUpdateCosts() (not sure if this poses a problem for VACUUM using these values since they are set to the defaults for VACUUM). Or, we could duplicate this logging message in do_autovacuum(). Finally, one other point about message_level_is_interesting(). I liked the idea of using it a lot, since log level DEBUG2 will not be the common case. I thought of it but hesitated because all other users of message_level_is_interesting() are avoiding some memory allocation or string copying -- not avoiding take a lock. Making this conditioned on log level made me a bit uncomfortable. I can't think of a situation when it would be a problem, but it felt a bit off. > Also, while testing the autovacuum delay with relopt > autovacuum_vacuum_cost_delay = 0, I realized that even if we set > autovacuum_vacuum_cost_delay = 0 to a table, wi_dobalance is set to > true. wi_dobalance comes from the following expression: > > /* > * If any of the cost delay parameters has been set individually for > * this table, disable the balancing algorithm. > */ > tab->at_dobalance = > !(avopts && (avopts->vacuum_cost_limit > 0 || > avopts->vacuum_cost_delay > 0)); > > The initial values of both avopts->vacuum_cost_limit and > avopts->vacuum_cost_delay are -1. I think we should use ">= 0" instead > of "> 0". Otherwise, we include the autovacuum worker working on a > table whose autovacuum_vacuum_cost_delay is 0 to the balancing > algorithm. Probably this behavior has existed also on back branches > but I haven't checked it yet. Thank you for catching this. Indeed this exists in master since 1021bd6a89b which was backpatched. I checked and it is true all the way back through REL_11_STABLE. Definitely seems worth fixing as it kind of defeats the purpose of the original commit. I wish I had noticed before! Your fix has: !(avopts && (avopts->vacuum_cost_limit >= 0 || avopts->vacuum_cost_delay >= 0)); And though delay is required to be >= 0 avopts->vacuum_cost_delay >= 0 Limit does not. It can just be > 0. postgres=# create table foo (a int) with (autovacuum_vacuum_cost_limit = 0); ERROR: value 0 out of bounds for option "autovacuum_vacuum_cost_limit" DETAIL: Valid values are between "1" and "10000". Though >= is also fine, the rest of the code in all versions always checks if limit > 0 and delay >= 0 since 0 is a valid value for delay and not for limit. Probably best we keep it consistent (though the whole thing is quite confusing). - Melanie ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-04-07 13:23 Daniel Gustafsson <[email protected]> parent: Melanie Plageman <[email protected]> 1 sibling, 1 reply; 26+ messages in thread From: Daniel Gustafsson @ 2023-04-07 13:23 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Masahiko Sawada <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> > On 7 Apr 2023, at 15:07, Melanie Plageman <[email protected]> wrote: > On Fri, Apr 7, 2023 at 2:53 AM Masahiko Sawada <[email protected]> wrote: >> + /* Only log updates to cost-related variables */ >> + if (vacuum_cost_delay == original_cost_delay && >> + vacuum_cost_limit == original_cost_limit) >> + return; >> >> IIUC by default, we log not only before starting the vacuum but also >> when changing cost-related variables. Which is good, I think, because >> logging the initial values would also be helpful for investigation. >> However, I think that we don't log the initial vacuum cost values >> depending on the values. For example, if the >> autovacuum_vacuum_cost_delay storage option is set to 0, we don't log >> the initial values. I think that instead of comparing old and new >> values, we can write the log only if >> message_level_is_interesting(DEBUG2) is true. That way, we don't need >> to acquire the lwlock unnecessarily. And the code looks cleaner to me. >> I've attached the patch (use_message_level_is_interesting.patch) > > Thanks for coming up with the case you thought of with storage param for > cost delay = 0. In that case we wouldn't print the message initially and > we should fix that. > > I disagree, however, that we should condition it only on > message_level_is_interesting(). I think we should keep the logging frequency as committed, but condition taking the lock on message_level_is_interesting(). > Actually, outside of printing initial values when the autovacuum worker > first starts (before vacuuming all tables), I don't think we should log > these values except when they are being updated. Autovacuum workers > could vacuum tons of small tables and having this print out at least > once per table (which I know is how it is on master) would be > distracting. Also, you could be reloading the config to update some > other GUCs and be oblivious to an ongoing autovacuum and get these > messages printed out, which I would also find distracting. > > You will have to stare very hard at the logs to tell if your changes to > vacuum cost delay and limit took effect when you reload config. I think > with our changes to update the values more often, we should take the > opportunity to make this logging more useful by making it happen only > when the values are changed. > > I would be open to elevating the log level to DEBUG1 for logging only > updates and, perhaps, having an option if you set log level to DEBUG2, > for example, to always log these values in VacuumUpdateCosts(). > > I'd even argue that, potentially, having the cost-delay related > parameters printed at the beginning of vacuuming could be interesting to > regular VACUUM as well (even though it doesn't benefit from config > reload while in progress). > > To fix the issue you mentioned and ensure the logging is printed when > autovacuum workers start up before vacuuming tables, we could either > initialize vacuum_cost_delay and vacuum_cost_limit to something invalid > that will always be different than what they are set to in > VacuumUpdateCosts() (not sure if this poses a problem for VACUUM using > these values since they are set to the defaults for VACUUM). Or, we > could duplicate this logging message in do_autovacuum(). Duplicating logging, maybe with a slightly tailored message, seem the least bad option. > Finally, one other point about message_level_is_interesting(). I liked > the idea of using it a lot, since log level DEBUG2 will not be the > common case. I thought of it but hesitated because all other users of > message_level_is_interesting() are avoiding some memory allocation or > string copying -- not avoiding take a lock. Making this conditioned on > log level made me a bit uncomfortable. I can't think of a situation when > it would be a problem, but it felt a bit off. Considering how uncommon DEBUG2 will be in production, I think conditioning taking a lock on it makes sense. >> Also, while testing the autovacuum delay with relopt >> autovacuum_vacuum_cost_delay = 0, I realized that even if we set >> autovacuum_vacuum_cost_delay = 0 to a table, wi_dobalance is set to >> true. wi_dobalance comes from the following expression: >> >> /* >> * If any of the cost delay parameters has been set individually for >> * this table, disable the balancing algorithm. >> */ >> tab->at_dobalance = >> !(avopts && (avopts->vacuum_cost_limit > 0 || >> avopts->vacuum_cost_delay > 0)); >> >> The initial values of both avopts->vacuum_cost_limit and >> avopts->vacuum_cost_delay are -1. I think we should use ">= 0" instead >> of "> 0". Otherwise, we include the autovacuum worker working on a >> table whose autovacuum_vacuum_cost_delay is 0 to the balancing >> algorithm. Probably this behavior has existed also on back branches >> but I haven't checked it yet. > > Thank you for catching this. Indeed this exists in master since > 1021bd6a89b which was backpatched. I checked and it is true all the way > back through REL_11_STABLE. > > Definitely seems worth fixing as it kind of defeats the purpose of the > original commit. I wish I had noticed before! > > Your fix has: > !(avopts && (avopts->vacuum_cost_limit >= 0 || > avopts->vacuum_cost_delay >= 0)); > > And though delay is required to be >= 0 > avopts->vacuum_cost_delay >= 0 > > Limit does not. It can just be > 0. > > postgres=# create table foo (a int) with (autovacuum_vacuum_cost_limit = 0); > ERROR: value 0 out of bounds for option "autovacuum_vacuum_cost_limit" > DETAIL: Valid values are between "1" and "10000". > > Though >= is also fine, the rest of the code in all versions always > checks if limit > 0 and delay >= 0 since 0 is a valid value for delay > and not for limit. Probably best we keep it consistent (though the whole > thing is quite confusing). +1 -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-04-11 00:16 Melanie Plageman <[email protected]> parent: Melanie Plageman <[email protected]> 1 sibling, 0 replies; 26+ messages in thread From: Melanie Plageman @ 2023-04-11 00:16 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> On Fri, Apr 7, 2023 at 9:07 AM Melanie Plageman <[email protected]> wrote: > > On Fri, Apr 7, 2023 at 2:53 AM Masahiko Sawada <[email protected]> wrote: > > > > On Fri, Apr 7, 2023 at 8:08 AM Daniel Gustafsson <[email protected]> wrote: > > > > > > > On 7 Apr 2023, at 00:12, Melanie Plageman <[email protected]> wrote: > > > > > > > > On Thu, Apr 6, 2023 at 5:45 PM Daniel Gustafsson <[email protected]> wrote: > > > >> > > > >>> On 6 Apr 2023, at 23:06, Melanie Plageman <[email protected]> wrote: > > > >> > > > >>> Autovacuum workers, at the end of VacuumUpdateCosts(), check if cost > > > >>> limit or cost delay have been changed. If they have, they assert that > > > >>> they don't already hold the AutovacuumLock, take it in shared mode, and > > > >>> do the logging. > > > >> > > > >> Another idea would be to copy the values to local temp variables while holding > > > >> the lock, and release the lock before calling elog() to avoid holding the lock > > > >> over potential IO. > > > > > > > > Good idea. I've done this in attached v19. > > > > Also I looked through the docs and everything still looks correct for > > > > balancing algo. > > > > > > I had another read-through and test-through of this version, and have applied > > > it with some minor changes to comments and whitespace. Thanks for the quick > > > turnaround times on reviews in this thread! > > > > Cool! > > > > Regarding the commit 7d71d3dd08, I have one comment: > > > > + /* Only log updates to cost-related variables */ > > + if (vacuum_cost_delay == original_cost_delay && > > + vacuum_cost_limit == original_cost_limit) > > + return; > > > > IIUC by default, we log not only before starting the vacuum but also > > when changing cost-related variables. Which is good, I think, because > > logging the initial values would also be helpful for investigation. > > However, I think that we don't log the initial vacuum cost values > > depending on the values. For example, if the > > autovacuum_vacuum_cost_delay storage option is set to 0, we don't log > > the initial values. I think that instead of comparing old and new > > values, we can write the log only if > > message_level_is_interesting(DEBUG2) is true. That way, we don't need > > to acquire the lwlock unnecessarily. And the code looks cleaner to me. > > I've attached the patch (use_message_level_is_interesting.patch) > > Thanks for coming up with the case you thought of with storage param for > cost delay = 0. In that case we wouldn't print the message initially and > we should fix that. > > I disagree, however, that we should condition it only on > message_level_is_interesting(). > > Actually, outside of printing initial values when the autovacuum worker > first starts (before vacuuming all tables), I don't think we should log > these values except when they are being updated. Autovacuum workers > could vacuum tons of small tables and having this print out at least > once per table (which I know is how it is on master) would be > distracting. Also, you could be reloading the config to update some > other GUCs and be oblivious to an ongoing autovacuum and get these > messages printed out, which I would also find distracting. > > You will have to stare very hard at the logs to tell if your changes to > vacuum cost delay and limit took effect when you reload config. I think > with our changes to update the values more often, we should take the > opportunity to make this logging more useful by making it happen only > when the values are changed. > > I would be open to elevating the log level to DEBUG1 for logging only > updates and, perhaps, having an option if you set log level to DEBUG2, > for example, to always log these values in VacuumUpdateCosts(). > > I'd even argue that, potentially, having the cost-delay related > parameters printed at the beginning of vacuuming could be interesting to > regular VACUUM as well (even though it doesn't benefit from config > reload while in progress). > > To fix the issue you mentioned and ensure the logging is printed when > autovacuum workers start up before vacuuming tables, we could either > initialize vacuum_cost_delay and vacuum_cost_limit to something invalid > that will always be different than what they are set to in > VacuumUpdateCosts() (not sure if this poses a problem for VACUUM using > these values since they are set to the defaults for VACUUM). Or, we > could duplicate this logging message in do_autovacuum(). > > Finally, one other point about message_level_is_interesting(). I liked > the idea of using it a lot, since log level DEBUG2 will not be the > common case. I thought of it but hesitated because all other users of > message_level_is_interesting() are avoiding some memory allocation or > string copying -- not avoiding take a lock. Making this conditioned on > log level made me a bit uncomfortable. I can't think of a situation when > it would be a problem, but it felt a bit off. > > > Also, while testing the autovacuum delay with relopt > > autovacuum_vacuum_cost_delay = 0, I realized that even if we set > > autovacuum_vacuum_cost_delay = 0 to a table, wi_dobalance is set to > > true. wi_dobalance comes from the following expression: > > > > /* > > * If any of the cost delay parameters has been set individually for > > * this table, disable the balancing algorithm. > > */ > > tab->at_dobalance = > > !(avopts && (avopts->vacuum_cost_limit > 0 || > > avopts->vacuum_cost_delay > 0)); > > > > The initial values of both avopts->vacuum_cost_limit and > > avopts->vacuum_cost_delay are -1. I think we should use ">= 0" instead > > of "> 0". Otherwise, we include the autovacuum worker working on a > > table whose autovacuum_vacuum_cost_delay is 0 to the balancing > > algorithm. Probably this behavior has existed also on back branches > > but I haven't checked it yet. > > Thank you for catching this. Indeed this exists in master since > 1021bd6a89b which was backpatched. I checked and it is true all the way > back through REL_11_STABLE. > > Definitely seems worth fixing as it kind of defeats the purpose of the > original commit. I wish I had noticed before! > > Your fix has: > !(avopts && (avopts->vacuum_cost_limit >= 0 || > avopts->vacuum_cost_delay >= 0)); > > And though delay is required to be >= 0 > avopts->vacuum_cost_delay >= 0 > > Limit does not. It can just be > 0. > > postgres=# create table foo (a int) with (autovacuum_vacuum_cost_limit = 0); > ERROR: value 0 out of bounds for option "autovacuum_vacuum_cost_limit" > DETAIL: Valid values are between "1" and "10000". > > Though >= is also fine, the rest of the code in all versions always > checks if limit > 0 and delay >= 0 since 0 is a valid value for delay > and not for limit. Probably best we keep it consistent (though the whole > thing is quite confusing). I have created an open item for each of these issues on the wiki (one for 16 and one under the section "affects stable branches"). - Melanie ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-04-11 15:05 Masahiko Sawada <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Masahiko Sawada @ 2023-04-11 15:05 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Melanie Plageman <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> On Fri, Apr 7, 2023 at 10:23 PM Daniel Gustafsson <[email protected]> wrote: > > > On 7 Apr 2023, at 15:07, Melanie Plageman <[email protected]> wrote: > > On Fri, Apr 7, 2023 at 2:53 AM Masahiko Sawada <[email protected]> wrote: > > >> + /* Only log updates to cost-related variables */ > >> + if (vacuum_cost_delay == original_cost_delay && > >> + vacuum_cost_limit == original_cost_limit) > >> + return; > >> > >> IIUC by default, we log not only before starting the vacuum but also > >> when changing cost-related variables. Which is good, I think, because > >> logging the initial values would also be helpful for investigation. > >> However, I think that we don't log the initial vacuum cost values > >> depending on the values. For example, if the > >> autovacuum_vacuum_cost_delay storage option is set to 0, we don't log > >> the initial values. I think that instead of comparing old and new > >> values, we can write the log only if > >> message_level_is_interesting(DEBUG2) is true. That way, we don't need > >> to acquire the lwlock unnecessarily. And the code looks cleaner to me. > >> I've attached the patch (use_message_level_is_interesting.patch) > > > > Thanks for coming up with the case you thought of with storage param for > > cost delay = 0. In that case we wouldn't print the message initially and > > we should fix that. > > > > I disagree, however, that we should condition it only on > > message_level_is_interesting(). > > I think we should keep the logging frequency as committed, but condition taking > the lock on message_level_is_interesting(). > > > Actually, outside of printing initial values when the autovacuum worker > > first starts (before vacuuming all tables), I don't think we should log > > these values except when they are being updated. Autovacuum workers > > could vacuum tons of small tables and having this print out at least > > once per table (which I know is how it is on master) would be > > distracting. Also, you could be reloading the config to update some > > other GUCs and be oblivious to an ongoing autovacuum and get these > > messages printed out, which I would also find distracting. > > > > You will have to stare very hard at the logs to tell if your changes to > > vacuum cost delay and limit took effect when you reload config. I think > > with our changes to update the values more often, we should take the > > opportunity to make this logging more useful by making it happen only > > when the values are changed. > > For debugging purposes, I think it could also be important information that the cost values are not changed. Personally, I prefer to log the current state rather than deciding for ourselves which events are important. If always logging these values in DEBUG2 had been distracting, we might want to lower it to DEBUG3. > > I would be open to elevating the log level to DEBUG1 for logging only > > updates and, perhaps, having an option if you set log level to DEBUG2, > > for example, to always log these values in VacuumUpdateCosts(). I'm not really sure it's a good idea to change the log messages and events depending on elevel. Do you know we have any precedents ? > > > > I'd even argue that, potentially, having the cost-delay related > > parameters printed at the beginning of vacuuming could be interesting to > > regular VACUUM as well (even though it doesn't benefit from config > > reload while in progress). > > > > To fix the issue you mentioned and ensure the logging is printed when > > autovacuum workers start up before vacuuming tables, we could either > > initialize vacuum_cost_delay and vacuum_cost_limit to something invalid > > that will always be different than what they are set to in > > VacuumUpdateCosts() (not sure if this poses a problem for VACUUM using > > these values since they are set to the defaults for VACUUM). Or, we > > could duplicate this logging message in do_autovacuum(). > > Duplicating logging, maybe with a slightly tailored message, seem the least > bad option. > > > Finally, one other point about message_level_is_interesting(). I liked > > the idea of using it a lot, since log level DEBUG2 will not be the > > common case. I thought of it but hesitated because all other users of > > message_level_is_interesting() are avoiding some memory allocation or > > string copying -- not avoiding take a lock. Making this conditioned on > > log level made me a bit uncomfortable. I can't think of a situation when > > it would be a problem, but it felt a bit off. > > Considering how uncommon DEBUG2 will be in production, I think conditioning > taking a lock on it makes sense. The comment of message_level_is_interesting() says: * This is useful to short-circuit any expensive preparatory work that * might be needed for a logging message. Which can apply to taking a lwlock, I think. > > >> Also, while testing the autovacuum delay with relopt > >> autovacuum_vacuum_cost_delay = 0, I realized that even if we set > >> autovacuum_vacuum_cost_delay = 0 to a table, wi_dobalance is set to > >> true. wi_dobalance comes from the following expression: > >> > >> /* > >> * If any of the cost delay parameters has been set individually for > >> * this table, disable the balancing algorithm. > >> */ > >> tab->at_dobalance = > >> !(avopts && (avopts->vacuum_cost_limit > 0 || > >> avopts->vacuum_cost_delay > 0)); > >> > >> The initial values of both avopts->vacuum_cost_limit and > >> avopts->vacuum_cost_delay are -1. I think we should use ">= 0" instead > >> of "> 0". Otherwise, we include the autovacuum worker working on a > >> table whose autovacuum_vacuum_cost_delay is 0 to the balancing > >> algorithm. Probably this behavior has existed also on back branches > >> but I haven't checked it yet. > > > > Thank you for catching this. Indeed this exists in master since > > 1021bd6a89b which was backpatched. I checked and it is true all the way > > back through REL_11_STABLE. Thanks for checking! > > > > Definitely seems worth fixing as it kind of defeats the purpose of the > > original commit. I wish I had noticed before! > > > > Your fix has: > > !(avopts && (avopts->vacuum_cost_limit >= 0 || > > avopts->vacuum_cost_delay >= 0)); > > > > And though delay is required to be >= 0 > > avopts->vacuum_cost_delay >= 0 > > > > Limit does not. It can just be > 0. > > > > postgres=# create table foo (a int) with (autovacuum_vacuum_cost_limit = 0); > > ERROR: value 0 out of bounds for option "autovacuum_vacuum_cost_limit" > > DETAIL: Valid values are between "1" and "10000". > > > > Though >= is also fine, the rest of the code in all versions always > > checks if limit > 0 and delay >= 0 since 0 is a valid value for delay > > and not for limit. Probably best we keep it consistent (though the whole > > thing is quite confusing). > > +1 +1. I misunderstood the initial value of autovacuum_vacuum_cost_limit reloption. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-04-15 20:40 Daniel Gustafsson <[email protected]> parent: Masahiko Sawada <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Daniel Gustafsson @ 2023-04-15 20:40 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: Melanie Plageman <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> > On 11 Apr 2023, at 17:05, Masahiko Sawada <[email protected]> wrote: > The comment of message_level_is_interesting() says: > > * This is useful to short-circuit any expensive preparatory work that > * might be needed for a logging message. > > Which can apply to taking a lwlock, I think. I agree that we can, and should, use message_level_is_interesting to skip taking this lock. Also, the more I think about the more I'm convinced that we should not change the current logging frequency of once per table from what we ship today. In DEGUG2 the logs should tell the whole story without requiring extrapolation based on missing entries. So I think we should use your patch to solve this open item. If there is interest in reducing the logging frequency we should discuss that in its own thread, insted of it being hidden in here. -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-04-27 09:29 John Naylor <[email protected]> parent: Daniel Gustafsson <[email protected]> 1 sibling, 2 replies; 26+ messages in thread From: John Naylor @ 2023-04-27 09:29 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Melanie Plageman <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Masahiko Sawada <[email protected]>; Kyotaro Horiguchi <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> On Fri, Apr 7, 2023 at 6:08 AM Daniel Gustafsson <[email protected]> wrote: > > I had another read-through and test-through of this version, and have applied > it with some minor changes to comments and whitespace. Thanks for the quick > turnaround times on reviews in this thread! - VacuumFailsafeActive = false; + Assert(!VacuumFailsafeActive); I can trigger this assert added in commit 7d71d3dd08. First build with the patch in [1], then: session 1: CREATE EXTENSION xid_wraparound ; CREATE TABLE autovacuum_disabled(id serial primary key, data text) WITH (autovacuum_enabled=false); INSERT INTO autovacuum_disabled(data) SELECT generate_series(1,1000); -- I can trigger without this, but just make sure it doesn't get vacuumed BEGIN; DELETE FROM autovacuum_disabled WHERE id % 2 = 0; session 2: -- get to failsafe limit SELECT consume_xids(1*1000*1000*1000); INSERT INTO autovacuum_disabled(data) SELECT 1; SELECT consume_xids(1*1000*1000*1000); INSERT INTO autovacuum_disabled(data) SELECT 1; VACUUM autovacuum_disabled; WARNING: cutoff for removing and freezing tuples is far in the past HINT: Close open transactions soon to avoid wraparound problems. You might also need to commit or roll back old prepared transactions, or drop stale replication slots. WARNING: bypassing nonessential maintenance of table "john.public.autovacuum_disabled" as a failsafe after 0 index scans DETAIL: The table's relfrozenxid or relminmxid is too far in the past. HINT: Consider increasing configuration parameter "maintenance_work_mem" or "autovacuum_work_mem". You might also need to consider other ways for VACUUM to keep up with the allocation of transaction IDs. server closed the connection unexpectedly #0 0x00007ff31f68ebec in __pthread_kill_implementation () from /lib64/libc.so.6 #1 0x00007ff31f63e956 in raise () from /lib64/libc.so.6 #2 0x00007ff31f6287f4 in abort () from /lib64/libc.so.6 #3 0x0000000000978032 in ExceptionalCondition ( conditionName=conditionName@entry=0xa4e970 "!VacuumFailsafeActive", fileName=fileName@entry=0xa4da38 "../src/backend/access/heap/vacuumlazy.c", lineNumber=lineNumber@entry=392) at ../src/backend/utils/error/assert.c:66 #4 0x000000000058c598 in heap_vacuum_rel (rel=0x7ff31d8a97d0, params=<optimized out>, bstrategy=<optimized out>) at ../src/backend/access/heap/vacuumlazy.c:392 #5 0x000000000069af1f in table_relation_vacuum (bstrategy=0x14ddca8, params=0x7ffec28585f0, rel=0x7ff31d8a97d0) at ../src/include/access/tableam.h:1705 #6 vacuum_rel (relid=relid@entry=16402, relation=relation@entry=0x0, params=params@entry=0x7ffec28585f0, skip_privs=skip_privs@entry=true, bstrategy=bstrategy@entry=0x14ddca8) at ../src/backend/commands/vacuum.c:2202 #7 0x000000000069b0e4 in vacuum_rel (relid=16398, relation=<optimized out>, params=params@entry=0x7ffec2858850, skip_privs=skip_privs@entry=false, bstrategy=bstrategy@entry=0x14ddca8) at ../src/backend/commands/vacuum.c:2236 #8 0x000000000069c594 in vacuum (relations=0x14dde38, params=0x7ffec2858850, bstrategy=0x14ddca8, vac_context=0x14ddb90, isTopLevel=<optimized out>) at ../src/backend/commands/vacuum.c:623 [1] https://www.postgresql.org/message-id/CAD21AoAyYBZOiB1UPCPZJHTLk0-arrq5zqNGj%2BPrsbpdUy%3Dg-g%40mail... -- John Naylor EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-04-27 09:32 Daniel Gustafsson <[email protected]> parent: John Naylor <[email protected]> 1 sibling, 0 replies; 26+ messages in thread From: Daniel Gustafsson @ 2023-04-27 09:32 UTC (permalink / raw) To: John Naylor <[email protected]>; +Cc: Melanie Plageman <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Masahiko Sawada <[email protected]>; Kyotaro Horiguchi <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> > On 27 Apr 2023, at 11:29, John Naylor <[email protected]> wrote: > On Fri, Apr 7, 2023 at 6:08 AM Daniel Gustafsson <[email protected]> wrote: > > I had another read-through and test-through of this version, and have applied > > it with some minor changes to comments and whitespace. Thanks for the quick > > turnaround times on reviews in this thread! > > - VacuumFailsafeActive = false; > + Assert(!VacuumFailsafeActive); > > I can trigger this assert added in commit 7d71d3dd08. > > First build with the patch in [1], then: Interesting, thanks for the report! I'll look into it directly. -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-04-27 12:10 Masahiko Sawada <[email protected]> parent: John Naylor <[email protected]> 1 sibling, 1 reply; 26+ messages in thread From: Masahiko Sawada @ 2023-04-27 12:10 UTC (permalink / raw) To: John Naylor <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; Melanie Plageman <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> On Thu, Apr 27, 2023 at 6:30 PM John Naylor <[email protected]> wrote: > > > On Fri, Apr 7, 2023 at 6:08 AM Daniel Gustafsson <[email protected]> wrote: > > > > I had another read-through and test-through of this version, and have applied > > it with some minor changes to comments and whitespace. Thanks for the quick > > turnaround times on reviews in this thread! > > - VacuumFailsafeActive = false; > + Assert(!VacuumFailsafeActive); > > I can trigger this assert added in commit 7d71d3dd08. > > First build with the patch in [1], then: > > session 1: > > CREATE EXTENSION xid_wraparound ; > > CREATE TABLE autovacuum_disabled(id serial primary key, data text) WITH (autovacuum_enabled=false); > INSERT INTO autovacuum_disabled(data) SELECT generate_series(1,1000); > > -- I can trigger without this, but just make sure it doesn't get vacuumed > BEGIN; > DELETE FROM autovacuum_disabled WHERE id % 2 = 0; > > session 2: > > -- get to failsafe limit > SELECT consume_xids(1*1000*1000*1000); > INSERT INTO autovacuum_disabled(data) SELECT 1; > SELECT consume_xids(1*1000*1000*1000); > INSERT INTO autovacuum_disabled(data) SELECT 1; > > VACUUM autovacuum_disabled; > > WARNING: cutoff for removing and freezing tuples is far in the past > HINT: Close open transactions soon to avoid wraparound problems. > You might also need to commit or roll back old prepared transactions, or drop stale replication slots. > WARNING: bypassing nonessential maintenance of table "john.public.autovacuum_disabled" as a failsafe after 0 index scans > DETAIL: The table's relfrozenxid or relminmxid is too far in the past. > HINT: Consider increasing configuration parameter "maintenance_work_mem" or "autovacuum_work_mem". > You might also need to consider other ways for VACUUM to keep up with the allocation of transaction IDs. > server closed the connection unexpectedly > > #0 0x00007ff31f68ebec in __pthread_kill_implementation () > from /lib64/libc.so.6 > #1 0x00007ff31f63e956 in raise () from /lib64/libc.so.6 > #2 0x00007ff31f6287f4 in abort () from /lib64/libc.so.6 > #3 0x0000000000978032 in ExceptionalCondition ( > conditionName=conditionName@entry=0xa4e970 "!VacuumFailsafeActive", > fileName=fileName@entry=0xa4da38 "../src/backend/access/heap/vacuumlazy.c", lineNumber=lineNumber@entry=392) at ../src/backend/utils/error/assert.c:66 > #4 0x000000000058c598 in heap_vacuum_rel (rel=0x7ff31d8a97d0, > params=<optimized out>, bstrategy=<optimized out>) > at ../src/backend/access/heap/vacuumlazy.c:392 > #5 0x000000000069af1f in table_relation_vacuum (bstrategy=0x14ddca8, > params=0x7ffec28585f0, rel=0x7ff31d8a97d0) > at ../src/include/access/tableam.h:1705 > #6 vacuum_rel (relid=relid@entry=16402, relation=relation@entry=0x0, > params=params@entry=0x7ffec28585f0, skip_privs=skip_privs@entry=true, > bstrategy=bstrategy@entry=0x14ddca8) > at ../src/backend/commands/vacuum.c:2202 > #7 0x000000000069b0e4 in vacuum_rel (relid=16398, relation=<optimized out>, > params=params@entry=0x7ffec2858850, skip_privs=skip_privs@entry=false, > bstrategy=bstrategy@entry=0x14ddca8) > at ../src/backend/commands/vacuum.c:2236 Good catch. I think the problem is that vacuum_rel() is called recursively and we don't reset VacuumFailsafeActive before vacuuming the toast table. I think we should reset it in heap_vacuum_rel() instead of Assert(). It's possible that we trigger the failsafe mode only for either one.Please find the attached patch. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com Attachments: [application/octet-stream] reset_VacuumFailsafeActive.patch (628B, ../../CAD21AoB_NQwdznBqcTMPv=vx71feDvBTNEMAvaUMAGqNcruk9Q@mail.gmail.com/2-reset_VacuumFailsafeActive.patch) download | inline diff: diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 0a9ebd22bd..2ba85bd3d6 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -389,7 +389,7 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, Assert(params->index_cleanup != VACOPTVALUE_UNSPECIFIED); Assert(params->truncate != VACOPTVALUE_UNSPECIFIED && params->truncate != VACOPTVALUE_AUTO); - Assert(!VacuumFailsafeActive); + VacuumFailsafeActive = false; vacrel->consider_bypass_optimization = true; vacrel->do_index_vacuuming = true; vacrel->do_index_cleanup = true; ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-04-27 12:54 Daniel Gustafsson <[email protected]> parent: Masahiko Sawada <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Daniel Gustafsson @ 2023-04-27 12:54 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: John Naylor <[email protected]>; Melanie Plageman <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> > On 27 Apr 2023, at 14:10, Masahiko Sawada <[email protected]> wrote: > > On Thu, Apr 27, 2023 at 6:30 PM John Naylor > <[email protected]> wrote: >> >> >> On Fri, Apr 7, 2023 at 6:08 AM Daniel Gustafsson <[email protected]> wrote: >>> >>> I had another read-through and test-through of this version, and have applied >>> it with some minor changes to comments and whitespace. Thanks for the quick >>> turnaround times on reviews in this thread! >> >> - VacuumFailsafeActive = false; >> + Assert(!VacuumFailsafeActive); >> >> I can trigger this assert added in commit 7d71d3dd08. >> >> First build with the patch in [1], then: >> >> session 1: >> >> CREATE EXTENSION xid_wraparound ; >> >> CREATE TABLE autovacuum_disabled(id serial primary key, data text) WITH (autovacuum_enabled=false); >> INSERT INTO autovacuum_disabled(data) SELECT generate_series(1,1000); >> >> -- I can trigger without this, but just make sure it doesn't get vacuumed >> BEGIN; >> DELETE FROM autovacuum_disabled WHERE id % 2 = 0; >> >> session 2: >> >> -- get to failsafe limit >> SELECT consume_xids(1*1000*1000*1000); >> INSERT INTO autovacuum_disabled(data) SELECT 1; >> SELECT consume_xids(1*1000*1000*1000); >> INSERT INTO autovacuum_disabled(data) SELECT 1; >> >> VACUUM autovacuum_disabled; >> >> WARNING: cutoff for removing and freezing tuples is far in the past >> HINT: Close open transactions soon to avoid wraparound problems. >> You might also need to commit or roll back old prepared transactions, or drop stale replication slots. >> WARNING: bypassing nonessential maintenance of table "john.public.autovacuum_disabled" as a failsafe after 0 index scans >> DETAIL: The table's relfrozenxid or relminmxid is too far in the past. >> HINT: Consider increasing configuration parameter "maintenance_work_mem" or "autovacuum_work_mem". >> You might also need to consider other ways for VACUUM to keep up with the allocation of transaction IDs. >> server closed the connection unexpectedly >> >> #0 0x00007ff31f68ebec in __pthread_kill_implementation () >> from /lib64/libc.so.6 >> #1 0x00007ff31f63e956 in raise () from /lib64/libc.so.6 >> #2 0x00007ff31f6287f4 in abort () from /lib64/libc.so.6 >> #3 0x0000000000978032 in ExceptionalCondition ( >> conditionName=conditionName@entry=0xa4e970 "!VacuumFailsafeActive", >> fileName=fileName@entry=0xa4da38 "../src/backend/access/heap/vacuumlazy.c", lineNumber=lineNumber@entry=392) at ../src/backend/utils/error/assert.c:66 >> #4 0x000000000058c598 in heap_vacuum_rel (rel=0x7ff31d8a97d0, >> params=<optimized out>, bstrategy=<optimized out>) >> at ../src/backend/access/heap/vacuumlazy.c:392 >> #5 0x000000000069af1f in table_relation_vacuum (bstrategy=0x14ddca8, >> params=0x7ffec28585f0, rel=0x7ff31d8a97d0) >> at ../src/include/access/tableam.h:1705 >> #6 vacuum_rel (relid=relid@entry=16402, relation=relation@entry=0x0, >> params=params@entry=0x7ffec28585f0, skip_privs=skip_privs@entry=true, >> bstrategy=bstrategy@entry=0x14ddca8) >> at ../src/backend/commands/vacuum.c:2202 >> #7 0x000000000069b0e4 in vacuum_rel (relid=16398, relation=<optimized out>, >> params=params@entry=0x7ffec2858850, skip_privs=skip_privs@entry=false, >> bstrategy=bstrategy@entry=0x14ddca8) >> at ../src/backend/commands/vacuum.c:2236 > > Good catch. I think the problem is that vacuum_rel() is called > recursively and we don't reset VacuumFailsafeActive before vacuuming > the toast table. I think we should reset it in heap_vacuum_rel() > instead of Assert(). It's possible that we trigger the failsafe mode > only for either one.Please find the attached patch. Agreed, that matches my research and testing, I have the same diff here and it passes testing and works as intended. This was briefly discussed in [0] and slightly upthread from there but then missed. I will do some more looking and testing but I'm fairly sure this is the right fix, so unless I find something else I will go ahead with this. xid_wraparound is a really nifty testing tool. Very cool. -- Daniel Gustafsson [0] CAAKRu_b1HjGCTsFpUnmwLNS8NeXJ+JnrDLhT1osP+Gq9HCU+Rw@mail.gmail.com ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-04-27 14:53 Melanie Plageman <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Melanie Plageman @ 2023-04-27 14:53 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Masahiko Sawada <[email protected]>; John Naylor <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> On Thu, Apr 27, 2023 at 8:55 AM Daniel Gustafsson <[email protected]> wrote: > > > On 27 Apr 2023, at 14:10, Masahiko Sawada <[email protected]> wrote: > > > > On Thu, Apr 27, 2023 at 6:30 PM John Naylor > > <[email protected]> wrote: > >> > >> > >> On Fri, Apr 7, 2023 at 6:08 AM Daniel Gustafsson <[email protected]> wrote: > >>> > >>> I had another read-through and test-through of this version, and have applied > >>> it with some minor changes to comments and whitespace. Thanks for the quick > >>> turnaround times on reviews in this thread! > >> > >> - VacuumFailsafeActive = false; > >> + Assert(!VacuumFailsafeActive); > >> > >> I can trigger this assert added in commit 7d71d3dd08. > >> > >> First build with the patch in [1], then: > >> > >> session 1: > >> > >> CREATE EXTENSION xid_wraparound ; > >> > >> CREATE TABLE autovacuum_disabled(id serial primary key, data text) WITH (autovacuum_enabled=false); > >> INSERT INTO autovacuum_disabled(data) SELECT generate_series(1,1000); > >> > >> -- I can trigger without this, but just make sure it doesn't get vacuumed > >> BEGIN; > >> DELETE FROM autovacuum_disabled WHERE id % 2 = 0; > >> > >> session 2: > >> > >> -- get to failsafe limit > >> SELECT consume_xids(1*1000*1000*1000); > >> INSERT INTO autovacuum_disabled(data) SELECT 1; > >> SELECT consume_xids(1*1000*1000*1000); > >> INSERT INTO autovacuum_disabled(data) SELECT 1; > >> > >> VACUUM autovacuum_disabled; > >> > >> WARNING: cutoff for removing and freezing tuples is far in the past > >> HINT: Close open transactions soon to avoid wraparound problems. > >> You might also need to commit or roll back old prepared transactions, or drop stale replication slots. > >> WARNING: bypassing nonessential maintenance of table "john.public.autovacuum_disabled" as a failsafe after 0 index scans > >> DETAIL: The table's relfrozenxid or relminmxid is too far in the past. > >> HINT: Consider increasing configuration parameter "maintenance_work_mem" or "autovacuum_work_mem". > >> You might also need to consider other ways for VACUUM to keep up with the allocation of transaction IDs. > >> server closed the connection unexpectedly > >> > >> #0 0x00007ff31f68ebec in __pthread_kill_implementation () > >> from /lib64/libc.so.6 > >> #1 0x00007ff31f63e956 in raise () from /lib64/libc.so.6 > >> #2 0x00007ff31f6287f4 in abort () from /lib64/libc.so.6 > >> #3 0x0000000000978032 in ExceptionalCondition ( > >> conditionName=conditionName@entry=0xa4e970 "!VacuumFailsafeActive", > >> fileName=fileName@entry=0xa4da38 "../src/backend/access/heap/vacuumlazy.c", lineNumber=lineNumber@entry=392) at ../src/backend/utils/error/assert.c:66 > >> #4 0x000000000058c598 in heap_vacuum_rel (rel=0x7ff31d8a97d0, > >> params=<optimized out>, bstrategy=<optimized out>) > >> at ../src/backend/access/heap/vacuumlazy.c:392 > >> #5 0x000000000069af1f in table_relation_vacuum (bstrategy=0x14ddca8, > >> params=0x7ffec28585f0, rel=0x7ff31d8a97d0) > >> at ../src/include/access/tableam.h:1705 > >> #6 vacuum_rel (relid=relid@entry=16402, relation=relation@entry=0x0, > >> params=params@entry=0x7ffec28585f0, skip_privs=skip_privs@entry=true, > >> bstrategy=bstrategy@entry=0x14ddca8) > >> at ../src/backend/commands/vacuum.c:2202 > >> #7 0x000000000069b0e4 in vacuum_rel (relid=16398, relation=<optimized out>, > >> params=params@entry=0x7ffec2858850, skip_privs=skip_privs@entry=false, > >> bstrategy=bstrategy@entry=0x14ddca8) > >> at ../src/backend/commands/vacuum.c:2236 > > > > Good catch. I think the problem is that vacuum_rel() is called > > recursively and we don't reset VacuumFailsafeActive before vacuuming > > the toast table. I think we should reset it in heap_vacuum_rel() > > instead of Assert(). It's possible that we trigger the failsafe mode > > only for either one.Please find the attached patch. > > Agreed, that matches my research and testing, I have the same diff here and it > passes testing and works as intended. This was briefly discussed in [0] and > slightly upthread from there but then missed. I will do some more looking and > testing but I'm fairly sure this is the right fix, so unless I find something > else I will go ahead with this. > > xid_wraparound is a really nifty testing tool. Very cool.Makes sense to me too. Fix LGTM. Though we previously set it to false before this series of patches, perhaps it is worth adding a comment about why VacuumFailsafeActive must be reset here even though we reset it before vacuuming each table? - Melanie ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-04-27 21:25 Daniel Gustafsson <[email protected]> parent: Melanie Plageman <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Daniel Gustafsson @ 2023-04-27 21:25 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Masahiko Sawada <[email protected]>; John Naylor <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> > On 27 Apr 2023, at 16:53, Melanie Plageman <[email protected]> wrote: > On Thu, Apr 27, 2023 at 8:55 AM Daniel Gustafsson <[email protected]> wrote: >> >>> On 27 Apr 2023, at 14:10, Masahiko Sawada <[email protected]> wrote: >>> Good catch. I think the problem is that vacuum_rel() is called >>> recursively and we don't reset VacuumFailsafeActive before vacuuming >>> the toast table. I think we should reset it in heap_vacuum_rel() >>> instead of Assert(). It's possible that we trigger the failsafe mode >>> only for either one.Please find the attached patch. >> >> Agreed, that matches my research and testing, I have the same diff here and it >> passes testing and works as intended. This was briefly discussed in [0] and >> slightly upthread from there but then missed. I will do some more looking and >> testing but I'm fairly sure this is the right fix, so unless I find something >> else I will go ahead with this. >> >> xid_wraparound is a really nifty testing tool. Very cool.Makes sense to me too. > > Fix LGTM. Thanks for review. I plan to push this in the morning. > Though we previously set it to false before this series of patches, > perhaps it is > worth adding a comment about why VacuumFailsafeActive must be reset here > even though we reset it before vacuuming each table? Agreed. -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-04-28 10:52 Daniel Gustafsson <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Daniel Gustafsson @ 2023-04-28 10:52 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Masahiko Sawada <[email protected]>; John Naylor <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> > On 27 Apr 2023, at 23:25, Daniel Gustafsson <[email protected]> wrote: >> On 27 Apr 2023, at 16:53, Melanie Plageman <[email protected]> wrote: >> Fix LGTM. > > Thanks for review. I plan to push this in the morning. Done, thanks. -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH v21 6/8] Row pattern recognition patch (docs). @ 2024-08-26 04:32 Tatsuo Ishii <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Tatsuo Ishii @ 2024-08-26 04:32 UTC (permalink / raw) --- doc/src/sgml/advanced.sgml | 82 ++++++++++++++++++++++++++++++++++++ doc/src/sgml/func.sgml | 54 ++++++++++++++++++++++++ doc/src/sgml/ref/select.sgml | 38 ++++++++++++++++- 3 files changed, 172 insertions(+), 2 deletions(-) diff --git a/doc/src/sgml/advanced.sgml b/doc/src/sgml/advanced.sgml index 755c9f1485..b0b1d1c51e 100644 --- a/doc/src/sgml/advanced.sgml +++ b/doc/src/sgml/advanced.sgml @@ -537,6 +537,88 @@ WHERE pos < 3; <literal>rank</literal> less than 3. </para> + <para> + Row pattern common syntax can be used to perform row pattern recognition + in a query. The row pattern common syntax includes two sub + clauses: <literal>DEFINE</literal> + and <literal>PATTERN</literal>. <literal>DEFINE</literal> defines + definition variables along with an expression. The expression must be a + logical expression, which means it must + return <literal>TRUE</literal>, <literal>FALSE</literal> + or <literal>NULL</literal>. The expression may comprise column references + and functions. Window functions, aggregate functions and subqueries are + not allowed. An example of <literal>DEFINE</literal> is as follows. + +<programlisting> +DEFINE + LOWPRICE AS price <= 100, + UP AS price > PREV(price), + DOWN AS price < PREV(price) +</programlisting> + + Note that <function>PREV</function> returns the price column in the + previous row if it's called in a context of row pattern recognition. Thus in + the second line the definition variable "UP" is <literal>TRUE</literal> + when the price column in the current row is greater than the price column + in the previous row. Likewise, "DOWN" is <literal>TRUE</literal> when when + the price column in the current row is lower than the price column in the + previous row. + </para> + <para> + Once <literal>DEFINE</literal> exists, <literal>PATTERN</literal> can be + used. <literal>PATTERN</literal> defines a sequence of rows that satisfies + certain conditions. For example following <literal>PATTERN</literal> + defines that a row starts with the condition "LOWPRICE", then one or more + rows satisfy "UP" and finally one or more rows satisfy "DOWN". Note that + "+" means one or more matches. Also you can use "*", which means zero or + more matches. If a sequence of rows which satisfies the PATTERN is found, + in the starting row of the sequence of rows all window functions and + aggregates are shown in the target list. Note that aggregations only look + into the matched rows, rather than whole frame. On the second or + subsequent rows all window functions are NULL. Aggregates are NULL or 0 + (count case) depending on its aggregation definition. For rows that do not + match on the PATTERN, all window functions and aggregates are shown AS + NULL too, except count showing 0. This is because the rows do not match, + thus they are in an empty frame. Example of a <literal>SELECT</literal> + using the <literal>DEFINE</literal> and <literal>PATTERN</literal> clause + is as follows. + +<programlisting> +SELECT company, tdate, price, + first_value(price) OVER w, + max(price) OVER w, + count(price) OVER w +FROM stock + WINDOW w AS ( + PARTITION BY company + ORDER BY tdate + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + AFTER MATCH SKIP PAST LAST ROW + INITIAL + PATTERN (LOWPRICE UP+ DOWN+) + DEFINE + LOWPRICE AS price <= 100, + UP AS price > PREV(price), + DOWN AS price < PREV(price) +); +</programlisting> +<screen> + company | tdate | price | first_value | max | count +----------+------------+-------+-------------+-----+------- + company1 | 2023-07-01 | 100 | 100 | 200 | 4 + company1 | 2023-07-02 | 200 | | | 0 + company1 | 2023-07-03 | 150 | | | 0 + company1 | 2023-07-04 | 140 | | | 0 + company1 | 2023-07-05 | 150 | | | 0 + company1 | 2023-07-06 | 90 | 90 | 130 | 4 + company1 | 2023-07-07 | 110 | | | 0 + company1 | 2023-07-08 | 130 | | | 0 + company1 | 2023-07-09 | 120 | | | 0 + company1 | 2023-07-10 | 130 | | | 0 +(10 rows) +</screen> + </para> + <para> When a query involves multiple window functions, it is possible to write out each one with a separate <literal>OVER</literal> clause, but this is diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 461fc3f437..02ad2b0195 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -23258,6 +23258,7 @@ SELECT count(*) FROM sometable; returns <literal>NULL</literal> if there is no such row. </para></entry> </row> + </tbody> </tgroup> </table> @@ -23297,6 +23298,59 @@ SELECT count(*) FROM sometable; Other frame specifications can be used to obtain other effects. </para> + <para> + Row pattern recognition navigation functions are listed in + <xref linkend="functions-rpr-navigation-table"/>. These functions + can be used to describe DEFINE clause of Row pattern recognition. + </para> + + <table id="functions-rpr-navigation-table"> + <title>Row Pattern Navigation Functions</title> + <tgroup cols="1"> + <thead> + <row> + <entry role="func_table_entry"><para role="func_signature"> + Function + </para> + <para> + Description + </para></entry> + </row> + </thead> + + <tbody> + <row> + <entry role="func_table_entry"><para role="func_signature"> + <indexterm> + <primary>prev</primary> + </indexterm> + <function>prev</function> ( <parameter>value</parameter> <type>anyelement</type> ) + <returnvalue>anyelement</returnvalue> + </para> + <para> + Returns the column value at the previous row; + returns NULL if there is no previous row in the window frame. + </para></entry> + </row> + + <row> + <entry role="func_table_entry"><para role="func_signature"> + <indexterm> + <primary>next</primary> + </indexterm> + <function>next</function> ( <parameter>value</parameter> <type>anyelement</type> ) + <returnvalue>anyelement</returnvalue> + </para> + <para> + Returns the column value at the next row; + returns NULL if there is no next row in the window frame. + </para></entry> + </row> + + </tbody> + </tgroup> + </table> + <note> <para> The SQL standard defines a <literal>RESPECT NULLS</literal> or diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml index d7089eac0b..7e1c9989ba 100644 --- a/doc/src/sgml/ref/select.sgml +++ b/doc/src/sgml/ref/select.sgml @@ -969,8 +969,8 @@ WINDOW <replaceable class="parameter">window_name</replaceable> AS ( <replaceabl The <replaceable class="parameter">frame_clause</replaceable> can be one of <synopsis> -{ RANGE | ROWS | GROUPS } <replaceable>frame_start</replaceable> [ <replaceable>frame_exclusion</replaceable> ] -{ RANGE | ROWS | GROUPS } BETWEEN <replaceable>frame_start</replaceable> AND <replaceable>frame_end</replaceable> [ <replaceable>frame_exclusion</replaceable> ] +{ RANGE | ROWS | GROUPS } <replaceable>frame_start</replaceable> [ <replaceable>frame_exclusion</replaceable> ] [row_pattern_common_syntax] +{ RANGE | ROWS | GROUPS } BETWEEN <replaceable>frame_start</replaceable> AND <replaceable>frame_end</replaceable> [ <replaceable>frame_exclusion</replaceable> ] [row_pattern_common_syntax] </synopsis> where <replaceable>frame_start</replaceable> @@ -1077,6 +1077,40 @@ EXCLUDE NO OTHERS a given peer group will be in the frame or excluded from it. </para> + <para> + The + optional <replaceable class="parameter">row_pattern_common_syntax</replaceable> + defines the <firstterm>row pattern recognition condition</firstterm> for + this + window. <replaceable class="parameter">row_pattern_common_syntax</replaceable> + includes following subclauses. <literal>AFTER MATCH SKIP PAST LAST + ROW</literal> or <literal>AFTER MATCH SKIP TO NEXT ROW</literal> controls + how to proceed to next row position after a match + found. With <literal>AFTER MATCH SKIP PAST LAST ROW</literal> (the + default) next row position is next to the last row of previous match. On + the other hand, with <literal>AFTER MATCH SKIP TO NEXT ROW</literal> next + row position is always next to the last row of previous + match. <literal>DEFINE</literal> defines definition variables along with a + boolean expression. <literal>PATTERN</literal> defines a sequence of rows + that satisfies certain conditions using variables defined + in <literal>DEFINE</literal> clause. If the variable is not defined in + the <literal>DEFINE</literal> clause, it is implicitly assumed + following is defined in the <literal>DEFINE</literal> clause. + +<synopsis> +<literal>variable_name</literal> AS TRUE +</synopsis> + + Note that the maximu number of variables defined + in <literal>DEFINE</literal> clause is 26. + +<synopsis> +[ AFTER MATCH SKIP PAST LAST ROW | AFTER MATCH SKIP TO NEXT ROW ] +PATTERN <replaceable class="parameter">pattern_variable_name</replaceable>[+] [, ...] +DEFINE <replaceable class="parameter">definition_varible_name</replaceable> AS <replaceable class="parameter">expression</replaceable> [, ...] +</synopsis> + </para> + <para> The purpose of a <literal>WINDOW</literal> clause is to specify the behavior of <firstterm>window functions</firstterm> appearing in the query's -- 2.25.1 ----Next_Part(Mon_Aug_26_13_39_47_2024_878)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v21-0007-Row-pattern-recognition-patch-tests.patch" ^ permalink raw reply [nested|flat] 26+ messages in thread
end of thread, other threads:[~2024-08-26 04:32 UTC | newest] Thread overview: 26+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-04-05 19:43 Re: Should vacuum process config file reload more often Melanie Plageman <[email protected]> 2023-04-05 19:58 ` Robert Haas <[email protected]> 2023-04-06 03:10 ` Melanie Plageman <[email protected]> 2023-04-06 15:52 ` Melanie Plageman <[email protected]> 2023-04-06 17:18 ` Robert Haas <[email protected]> 2023-04-06 18:55 ` Daniel Gustafsson <[email protected]> 2023-04-06 19:09 ` Melanie Plageman <[email protected]> 2023-04-06 21:06 ` Melanie Plageman <[email protected]> 2023-04-06 21:45 ` Daniel Gustafsson <[email protected]> 2023-04-06 22:12 ` Melanie Plageman <[email protected]> 2023-04-06 23:08 ` Daniel Gustafsson <[email protected]> 2023-04-07 06:52 ` Masahiko Sawada <[email protected]> 2023-04-07 11:28 ` Daniel Gustafsson <[email protected]> 2023-04-07 13:07 ` Melanie Plageman <[email protected]> 2023-04-07 13:23 ` Daniel Gustafsson <[email protected]> 2023-04-11 15:05 ` Masahiko Sawada <[email protected]> 2023-04-15 20:40 ` Daniel Gustafsson <[email protected]> 2023-04-11 00:16 ` Melanie Plageman <[email protected]> 2023-04-27 09:29 ` John Naylor <[email protected]> 2023-04-27 09:32 ` Daniel Gustafsson <[email protected]> 2023-04-27 12:10 ` Masahiko Sawada <[email protected]> 2023-04-27 12:54 ` Daniel Gustafsson <[email protected]> 2023-04-27 14:53 ` Melanie Plageman <[email protected]> 2023-04-27 21:25 ` Daniel Gustafsson <[email protected]> 2023-04-28 10:52 ` Daniel Gustafsson <[email protected]> 2024-08-26 04:32 [PATCH v21 6/8] Row pattern recognition patch (docs). Tatsuo Ishii <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox