public inbox for [email protected]help / color / mirror / Atom feed
Re: Should vacuum process config file reload more often 11+ messages / 4 participants [nested] [flat]
* Re: Should vacuum process config file reload more often @ 2023-03-15 05:13 Masahiko Sawada <[email protected]> 2023-03-18 22:47 ` Re: Should vacuum process config file reload more often Melanie Plageman <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Masahiko Sawada @ 2023-03-15 05:13 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: pgsql-hackers; Andres Freund <[email protected]>; Amit Kapila <[email protected]> On Sat, Mar 11, 2023 at 8:11 AM Melanie Plageman <[email protected]> wrote: > > Quotes below are combined from two of Sawada-san's emails. > > I've also attached a patch with my suggested current version. > > On Thu, Mar 9, 2023 at 10:27 PM Masahiko Sawada <[email protected]> wrote: > > > > On Fri, Mar 10, 2023 at 11:23 AM Melanie Plageman > > <[email protected]> wrote: > > > > > > On Tue, Mar 7, 2023 at 12:10 AM Masahiko Sawada <[email protected]> wrote: > > > > > > > > On Mon, Mar 6, 2023 at 5:26 AM Melanie Plageman > > > > <[email protected]> wrote: > > > > > > > > > > On Thu, Mar 2, 2023 at 6:37 PM Melanie Plageman > > > > > In this version I've removed wi_cost_delay from WorkerInfoData. There is > > > > > no synchronization of cost_delay amongst workers, so there is no reason > > > > > to keep it in shared memory. > > > > > > > > > > One consequence of not updating VacuumCostDelay from wi_cost_delay is > > > > > that we have to have a way to keep track of whether or not autovacuum > > > > > table options are in use. > > > > > > > > > > This patch does this in a cringeworthy way. I added two global > > > > > variables, one to track whether or not cost delay table options are in > > > > > use and the other to store the value of the table option cost delay. I > > > > > didn't want to use a single variable with a special value to indicate > > > > > that table option cost delay is in use because > > > > > autovacuum_vacuum_cost_delay already has special values that mean > > > > > certain things. My code needs a better solution. > > > > > > > > While it's true that wi_cost_delay doesn't need to be shared, it seems > > > > to make the logic somewhat complex. We need to handle cost_delay in a > > > > different way from other vacuum-related parameters and we need to make > > > > sure av[_use]_table_option_cost_delay are set properly. Removing > > > > wi_cost_delay from WorkerInfoData saves 8 bytes shared memory per > > > > autovacuum worker but it might be worth considering to keep > > > > wi_cost_delay for simplicity. > > > > > > Ah, it turns out we can't really remove wi_cost_delay from WorkerInfo > > > anyway because the launcher doesn't know anything about table options > > > and so the workers have to keep an updated wi_cost_delay that the > > > launcher or other autovac workers who are not vacuuming that table can > > > read from when calculating the new limit in autovac_balance_cost(). > > > > IIUC if any of the cost delay parameters has been set individually, > > the autovacuum worker is excluded from the balance algorithm. > > Ah, yes! That's right. So it is not a problem. Then I still think > removing wi_cost_delay from the worker info makes sense. wi_cost_delay > is a double and can't easily be accessed atomically the way > wi_cost_limit can be. > > Keeping the cost delay local to the backends also makes it clear that > cost delay is not something that should be written to by other backends > or that can differ from worker to worker. Without table options in the > picture, the cost delay should be the same for any worker who has > reloaded the config file. Agreed. > > As for the cost limit safe access issue, maybe we can avoid a LWLock > acquisition for reading wi_cost_limit by using an atomic similar to what > you suggested here for "did_rebalance". > > > > I've added in a shared lock for reading from wi_cost_limit in this > > > patch. However, AutoVacuumUpdateLimit() is called unconditionally in > > > vacuum_delay_point(), which is called quite often (per block-ish), so I > > > was trying to think if there is a way we could avoid having to check > > > this shared memory variable on every call to vacuum_delay_point(). > > > Rebalances shouldn't happen very often (done by the launcher when a new > > > worker is launched and by workers between vacuuming tables). Maybe we > > > can read from it less frequently? > > > > Yeah, acquiring the lwlock for every call to vacuum_delay_point() > > seems to be harmful. One idea would be to have one sig_atomic_t > > variable in WorkerInfoData and autovac_balance_cost() set it to true > > after rebalancing the worker's cost-limit. The worker can check it > > without locking and update its delay parameters if the flag is true. > > Instead of having the atomic indicate whether or not someone (launcher > or another worker) did a rebalance, it would simply store the current > cost limit. Then the worker can normally access it with a simple read. > > My rationale is that if we used an atomic to indicate whether or not we > did a rebalance ("did_rebalance"), we would have the same cache > coherency guarantees as if we just used the atomic for the cost limit. > If we read from the "did_rebalance" variable and missed someone having > written to it on another core, we still wouldn't get around to checking > the wi_cost_limit variable in shared memory, so it doesn't matter that > we bothered to keep it in shared memory and use a lock to access it. > > I noticed we don't allow wi_cost_limit to ever be less than 0, so we > could store wi_cost_limit in an atomic uint32. > > I'm not sure if it is okay to do pg_atomic_read_u32() and > pg_atomic_unlocked_write_u32() or if we need pg_atomic_write_u32() in > most cases. I agree to use pg_atomic_uin32. Given that the comment of pg_atomic_unlocked_write_u32() says: * pg_atomic_compare_exchange_u32. This should only be used in cases where * minor performance regressions due to atomics emulation are unacceptable. I think pg_atomic_write_u32() is enough for our use case. > > I've implemented the atomic cost limit in the attached patch. Though, > I'm pretty unsure about how I initialized the atomics in > AutoVacuumShmemInit()... + /* initialize the WorkerInfo free list */ for (i = 0; i < autovacuum_max_workers; i++) dlist_push_head(&AutoVacuumShmem->av_freeWorkers, &worker[i].wi_links); + + dlist_foreach(iter, &AutoVacuumShmem->av_freeWorkers) + pg_atomic_init_u32( + &(dlist_container(WorkerInfoData, wi_links, iter.cur))->wi_cost_limit, + 0); + I think we can do like: /* 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_u32(&(worker[i].wi_cost_limit)); } > > If the consensus is that it is simply too confusing to take > wi_cost_delay out of WorkerInfo, we might be able to afford using a > shared lock to access it because we won't call AutoVacuumUpdateDelay() > on every invocation of vacuum_delay_point() -- only when we've reloaded > the config file. > > One potential option to avoid taking a shared lock on every call to > AutoVacuumUpdateDelay() is to set a global variable to indicate that we > did update it (since we are the only ones updating it) and then only > take the shared LWLock in AutoVacuumUpdateDelay() if that flag is true. > If we remove wi_cost_delay from WorkerInfo, probably we don't need to acquire the lwlock in AutoVacuumUpdateDelay()? The shared field we access in that function will be only wi_dobalance, but this field is updated only by its owner autovacuum worker. > > --- > > void > > AutoVacuumUpdateDelay(void) > > { > > - if (MyWorkerInfo) > > + /* > > + * We are using autovacuum-related GUCs to update > > VacuumCostDelay, so we > > + * only want autovacuum workers and autovacuum launcher to do this. > > + */ > > + if (!(am_autovacuum_worker || am_autovacuum_launcher)) > > + return; > > > > Is there any case where the autovacuum launcher calls > > AutoVacuumUpdateDelay() function? > > I had meant to add it to HandleAutoVacLauncherInterrupts() after > reloading the config file (done in attached patch). When using the > global variables for cost delay (instead of wi_cost_delay in worker > info), the autovac launcher also has to do the check in the else branch > of AutoVacuumUpdateDelay() > > VacuumCostDelay = autovacuum_vac_cost_delay >= 0 ? > autovacuum_vac_cost_delay : VacuumCostDelay; > > to make sure VacuumCostDelay is correct for when it calls > autovac_balance_cost(). But doesn't the launcher do a similar thing at the beginning of autovac_balance_cost()? double vac_cost_delay = (autovacuum_vac_cost_delay >= 0 ? autovacuum_vac_cost_delay : VacuumCostDelay); Related to this point, I think autovac_balance_cost() should use globally-set cost_limit and cost_delay values to calculate worker's vacuum-delay parameters. IOW, vac_cost_limit and vac_cost_delay should come from the config file setting, not table option etc: 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); If my understanding is right, the following change is not right; AutoVacUpdateLimit() updates the VacuumCostLimit based on the value in MyWorkerInfo: MyWorkerInfo->wi_cost_limit_base = tab->at_vacuum_cost_limit; + AutoVacuumUpdateLimit(); /* do a balance */ autovac_balance_cost(); - /* set the active cost parameters from the result of that */ - AutoVacuumUpdateDelay(); Also, even when using the global variables for cost delay, the launcher doesn't need to check the global variable. It should always be able to use either autovacuum_vac_cost_delay/limit or VacuumCostDelay/Limit. > > This also made me think about whether or not we still need cost_limit_base. > It is used to ensure that autovac_balance_cost() never ends up setting > workers' wi_cost_limits above the current autovacuum_vacuum_cost_limit > (or VacuumCostLimit). However, the launcher and all the workers should > know what the value is without cost_limit_base, no? Yeah, the current balancing algorithm looks to respect the cost_limit value set when starting to vacuum the table. The proportion of the amount of I/O that a worker can consume is calculated based on the base value and the new worker's cost_limit value cannot exceed the base value. Given that we're trying to dynamically tune worker's cost parameters (delay and limit), this concept seems to need to be updated. > > > > Also not sure how the patch interacts with failsafe autovac and parallel > > > vacuum. > > > > Good point. > > > > When entering the failsafe mode, we disable the vacuum delays (see > > lazy_check_wraparound_failsafe()). We need to keep disabling the > > vacuum delays even after reloading the config file. One idea is to > > have another global variable indicating we're in the failsafe mode. > > vacuum_delay_point() doesn't update VacuumCostActive if the flag is > > true. > > I think we might not need to do this. Other than in > lazy_check_wraparound_failsafe(), VacuumCostActive is only updated in > two places: > > 1) in vacuum() which autovacuum will call per table. And failsafe is > reset per table as well. > > 2) in vacuum_delay_point(), but, since VacuumCostActive will already be > false when we enter vacuum_delay_point() the next time after > lazy_check_wraparound_failsafe(), we won't set VacuumCostActive there. Indeed. But does it mean that there is no code path to turn vacuum-delay on, even when vacuum_cost_delay is updated from 0 to non-0? Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Should vacuum process config file reload more often 2023-03-15 05:13 Re: Should vacuum process config file reload more often Masahiko Sawada <[email protected]> @ 2023-03-18 22:47 ` Melanie Plageman <[email protected]> 2023-03-19 16:48 ` Re: Should vacuum process config file reload more often Melanie Plageman <[email protected]> 2023-03-23 06:08 ` Re: Should vacuum process config file reload more often Masahiko Sawada <[email protected]> 0 siblings, 2 replies; 11+ messages in thread From: Melanie Plageman @ 2023-03-18 22:47 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: pgsql-hackers; Andres Freund <[email protected]>; Amit Kapila <[email protected]> On Wed, Mar 15, 2023 at 1:14 AM Masahiko Sawada <[email protected]> wrote: > On Sat, Mar 11, 2023 at 8:11 AM Melanie Plageman > <[email protected]> wrote: > > I've implemented the atomic cost limit in the attached patch. Though, > > I'm pretty unsure about how I initialized the atomics in > > AutoVacuumShmemInit()... > > + > /* initialize the WorkerInfo free list */ > for (i = 0; i < autovacuum_max_workers; i++) > dlist_push_head(&AutoVacuumShmem->av_freeWorkers, > &worker[i].wi_links); > + > + dlist_foreach(iter, &AutoVacuumShmem->av_freeWorkers) > + pg_atomic_init_u32( > + > &(dlist_container(WorkerInfoData, wi_links, iter.cur))->wi_cost_limit, > + 0); > + > > I think we can do like: > > /* 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_u32(&(worker[i].wi_cost_limit)); > } Ah, yes, I was distracted by the variable name "worker" (as opposed to "workers"). > > If the consensus is that it is simply too confusing to take > > wi_cost_delay out of WorkerInfo, we might be able to afford using a > > shared lock to access it because we won't call AutoVacuumUpdateDelay() > > on every invocation of vacuum_delay_point() -- only when we've reloaded > > the config file. > > > > One potential option to avoid taking a shared lock on every call to > > AutoVacuumUpdateDelay() is to set a global variable to indicate that we > > did update it (since we are the only ones updating it) and then only > > take the shared LWLock in AutoVacuumUpdateDelay() if that flag is true. > > > > If we remove wi_cost_delay from WorkerInfo, probably we don't need to > acquire the lwlock in AutoVacuumUpdateDelay()? The shared field we > access in that function will be only wi_dobalance, but this field is > updated only by its owner autovacuum worker. I realized that we cannot use dobalance to decide whether or not to update wi_cost_delay because dobalance could be false because of table option cost limit being set (with no table option cost delay) and we would still need to update VacuumCostDelay and wi_cost_delay with the new value of autovacuum_vacuum_cost_delay. But v5 skirts around this issue altogether. > > > --- > > > void > > > AutoVacuumUpdateDelay(void) > > > { > > > - if (MyWorkerInfo) > > > + /* > > > + * We are using autovacuum-related GUCs to update > > > VacuumCostDelay, so we > > > + * only want autovacuum workers and autovacuum launcher to do this. > > > + */ > > > + if (!(am_autovacuum_worker || am_autovacuum_launcher)) > > > + return; > > > > > > Is there any case where the autovacuum launcher calls > > > AutoVacuumUpdateDelay() function? > > > > I had meant to add it to HandleAutoVacLauncherInterrupts() after > > reloading the config file (done in attached patch). When using the > > global variables for cost delay (instead of wi_cost_delay in worker > > info), the autovac launcher also has to do the check in the else branch > > of AutoVacuumUpdateDelay() > > > > VacuumCostDelay = autovacuum_vac_cost_delay >= 0 ? > > autovacuum_vac_cost_delay : VacuumCostDelay; > > > > to make sure VacuumCostDelay is correct for when it calls > > autovac_balance_cost(). > > But doesn't the launcher do a similar thing at the beginning of > autovac_balance_cost()? > > double vac_cost_delay = (autovacuum_vac_cost_delay >= 0 ? > autovacuum_vac_cost_delay : VacuumCostDelay); Ah, yes. You are right. > Related to this point, I think autovac_balance_cost() should use > globally-set cost_limit and cost_delay values to calculate worker's > vacuum-delay parameters. IOW, vac_cost_limit and vac_cost_delay should > come from the config file setting, not table option etc: > > 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); > > If my understanding is right, the following change is not right; > AutoVacUpdateLimit() updates the VacuumCostLimit based on the value in > MyWorkerInfo: > > MyWorkerInfo->wi_cost_limit_base = tab->at_vacuum_cost_limit; > + AutoVacuumUpdateLimit(); > > /* do a balance */ > autovac_balance_cost(); > > - /* set the active cost parameters from the result of that */ > - AutoVacuumUpdateDelay(); > > Also, even when using the global variables for cost delay, the > launcher doesn't need to check the global variable. It should always > be able to use either autovacuum_vac_cost_delay/limit or > VacuumCostDelay/Limit. Yes, that is true. But, I actually think we can do something more radical, which relates to this point as well as the issue with cost_limit_base below. > > This also made me think about whether or not we still need cost_limit_base. > > It is used to ensure that autovac_balance_cost() never ends up setting > > workers' wi_cost_limits above the current autovacuum_vacuum_cost_limit > > (or VacuumCostLimit). However, the launcher and all the workers should > > know what the value is without cost_limit_base, no? > > Yeah, the current balancing algorithm looks to respect the cost_limit > value set when starting to vacuum the table. The proportion of the > amount of I/O that a worker can consume is calculated based on the > base value and the new worker's cost_limit value cannot exceed the > base value. Given that we're trying to dynamically tune worker's cost > parameters (delay and limit), this concept seems to need to be > updated. In master, autovacuum workers reload the config file at most once per table vacuumed. And that is the same time that they update their wi_cost_limit_base and wi_cost_delay. Thus, when autovac_balance_cost() is called, there is a good chance that different workers will have different values for wi_cost_limit_base and wi_cost_delay (and we are only talking about workers not vacuuming a table with table option cost-related gucs). So, it made sense that the balancing algorithm tried to use a ratio to determine what to set the cost limit of each worker to. It is clamped to the base value, as you say, but it also gives workers a proportion of the new limit equal to what proportion their base cost represents of the total cost. I think all of this doesn't matter anymore now that everyone can reload the config file often and dynamically change these values. Thus, in the attached v5, I have removed both wi_cost_limit and wi_cost_delay from WorkerInfo. I've added a new variable to AutoVacuumShmem called nworkers_for_balance. Now, autovac_balance_cost() only recalculates this number and updates it if it has changed. Then, in AutoVacuumUpdateLimit() workers read from this atomic value and divide the value of the cost limit gucs by that number to get their own cost limit. I keep the table option value of cost limit and cost delay in backend-local memory to reference when updating the worker cost limit. One nice thing is autovac_balance_cost() only requires an access shared lock now (though most callers are updating other members before calling it and still take an exclusive lock). What do you think? > > > > Also not sure how the patch interacts with failsafe autovac and parallel > > > > vacuum. > > > > > > Good point. > > > > > > When entering the failsafe mode, we disable the vacuum delays (see > > > lazy_check_wraparound_failsafe()). We need to keep disabling the > > > vacuum delays even after reloading the config file. One idea is to > > > have another global variable indicating we're in the failsafe mode. > > > vacuum_delay_point() doesn't update VacuumCostActive if the flag is > > > true. > > > > I think we might not need to do this. Other than in > > lazy_check_wraparound_failsafe(), VacuumCostActive is only updated in > > two places: > > > > 1) in vacuum() which autovacuum will call per table. And failsafe is > > reset per table as well. > > > > 2) in vacuum_delay_point(), but, since VacuumCostActive will already be > > false when we enter vacuum_delay_point() the next time after > > lazy_check_wraparound_failsafe(), we won't set VacuumCostActive there. > > Indeed. But does it mean that there is no code path to turn > vacuum-delay on, even when vacuum_cost_delay is updated from 0 to > non-0? Ah yes! Good point. This is true. I'm not sure how to cheaply allow for re-enabling delays after disabling them in the middle of a table vacuum. I don't see a way around checking if we need to reload the config file on every call to vacuum_delay_point() (currently, we are only doing this when we have to wait anyway). It seems expensive to do this check every time. If we do do this, we would update VacuumCostActive when updating VacuumCostDelay, and we would need a global variable keeping the failsafe status, as you mentioned. It could be okay to say that you can only disable cost-based delays in the middle of vacuuming a table (i.e. you cannot enable them if they are already disabled until you start vacuuming the next table). Though maybe it is weird that you can increase the delay but not re-enable it... On an unrelated note, I was wondering if there were any docs anywhere that should be updated to go along with this. And, I was wondering if it was worth trying to split up the part that reloads the config file and all of the autovacuum stuff. The reloading of the config file by itself won't actually result in autovacuum workers having updated cost delays because of them overwriting it with wi_cost_delay, but it will allow VACUUM to have those updated values. - Melanie Attachments: [text/x-patch] v5-0001-auto-vacuum-reloads-config-file-more-often.patch (18.2K, ../../CAAKRu_Z4NdHk6EofruFzrasO10bzQYCNuvZ=Rri1wJ6vwoV4-g@mail.gmail.com/2-v5-0001-auto-vacuum-reloads-config-file-more-often.patch) download | inline diff: From f77dc11eb38c96efe8a0defd8cb89ac44481d8f5 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Sat, 18 Mar 2023 15:42:39 -0400 Subject: [PATCH v5] [auto]vacuum reloads config file more often Previously, VACUUM and autovacuum workers would reload the configuration file only between vacuuming tables. This precluded user updates to cost-based delay parameters from taking effect while vacuuming a table. Check if a reload is pending roughly once per block now, when checking if we need to delay. In order for this change to have the intended effect on autovacuum, autovacuum workers must start updating their cost delay more frequently as well. With this new paradigm, balancing the cost limit amongst workers also must work differently. Previously, a 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 table options could still have different values for their wi_cost_limit_base and wi_cost_delay. With this change, workers will (within some margin of error) have no reason to have different values for cost limit and cost delay (in the absence of table options). Thus, remove cost limit and cost delay from shared memory and keep track only of the number of workers actively vacuuming tables with no cost-related table options. Then, use this value to determine what each worker's effective cost limit should be. Reviewed-by: Masahiko Sawada <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_buP5wzsho3qNw5o9_R0pF69FRM5hgCmr-mvXmGXwdA7A%40mail.gmail.com#5e6771d4cdca4db6efc2acec2dce0bc7 --- src/backend/commands/vacuum.c | 40 ++++-- src/backend/postmaster/autovacuum.c | 206 ++++++++++++---------------- src/include/postmaster/autovacuum.h | 2 + 3 files changed, 121 insertions(+), 127 deletions(-) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index c54360a6a0..6534fd748d 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" @@ -76,6 +77,7 @@ int vacuum_multixact_failsafe_age; /* A few variables that don't seem worth passing around as parameters */ static MemoryContext vac_context = NULL; static BufferAccessStrategy vac_strategy; +static bool analyze_in_outer_xact = false; /* @@ -314,8 +316,7 @@ vacuum(List *relations, VacuumParams *params, static bool in_vacuum = false; const char *stmttype; - volatile bool in_outer_xact, - use_own_xacts; + volatile bool use_own_xacts; Assert(params != NULL); @@ -332,10 +333,10 @@ vacuum(List *relations, VacuumParams *params, if (params->options & VACOPT_VACUUM) { PreventInTransactionBlock(isTopLevel, stmttype); - in_outer_xact = false; + analyze_in_outer_xact = false; } else - in_outer_xact = IsInTransactionBlock(isTopLevel); + analyze_in_outer_xact = IsInTransactionBlock(isTopLevel); /* * Due to static variables vac_context, anl_context and vac_strategy, @@ -457,7 +458,7 @@ vacuum(List *relations, VacuumParams *params, Assert(params->options & VACOPT_ANALYZE); if (IsAutoVacuumWorkerProcess()) use_own_xacts = true; - else if (in_outer_xact) + else if (analyze_in_outer_xact) use_own_xacts = false; else if (list_length(relations) > 1) use_own_xacts = true; @@ -475,7 +476,7 @@ vacuum(List *relations, VacuumParams *params, */ if (use_own_xacts) { - Assert(!in_outer_xact); + Assert(!analyze_in_outer_xact); /* ActiveSnapshot is not set by autovacuum */ if (ActiveSnapshotSet()) @@ -527,7 +528,7 @@ vacuum(List *relations, VacuumParams *params, } analyze_rel(vrel->oid, vrel->relation, params, - vrel->va_cols, in_outer_xact, vac_strategy); + vrel->va_cols, analyze_in_outer_xact, vac_strategy); if (use_own_xacts) { @@ -550,6 +551,7 @@ vacuum(List *relations, VacuumParams *params, { in_vacuum = false; VacuumCostActive = false; + analyze_in_outer_xact = false; } PG_END_TRY(); @@ -2246,10 +2248,30 @@ vacuum_delay_point(void) if (IsUnderPostmaster && !PostmasterIsAlive()) exit(1); + /* + * 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 && !analyze_in_outer_xact) + { + ConfigReloadPending = false; + ProcessConfigFile(PGC_SIGHUP); + AutoVacuumUpdateDelay(); + } + VacuumCostBalance = 0; - /* update balance values for workers */ - AutoVacuumUpdateDelay(); + /* + * Update balance values for 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. + */ + AutoVacuumUpdateLimit(); + + VacuumCostActive = (VacuumCostDelay > 0); /* Might have gotten an interrupt while sleeping */ CHECK_FOR_INTERRUPTS(); diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index c0e2e00a7e..f69f011589 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -139,6 +139,9 @@ int Log_autovacuum_min_duration = 600000; static bool am_autovacuum_launcher = false; static bool am_autovacuum_worker = false; +static double av_table_option_cost_delay = -1; +static int av_table_option_cost_limit = 0; + /* Flags set by signal handlers */ static volatile sig_atomic_t got_SIGUSR2 = false; @@ -189,8 +192,8 @@ typedef struct autovac_table { Oid at_relid; VacuumParams at_params; - double at_vacuum_cost_delay; - int at_vacuum_cost_limit; + double at_table_option_vac_cost_delay; + int at_table_option_vac_cost_limit; bool at_dobalance; bool at_sharedrel; char *at_relname; @@ -209,7 +212,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 @@ -225,9 +228,6 @@ typedef struct WorkerInfoData TimestampTz wi_launchtime; bool wi_dobalance; bool wi_sharedrel; - double wi_cost_delay; - int wi_cost_limit; - int wi_cost_limit_base; } WorkerInfoData; typedef struct WorkerInfoData *WorkerInfo; @@ -286,6 +286,7 @@ typedef struct dlist_head av_runningWorkers; WorkerInfo av_startingWorker; AutoVacuumWorkItem av_workItems[NUM_WORKITEMS]; + pg_atomic_uint32 nworkers_for_balance; } AutoVacuumShmemStruct; static AutoVacuumShmemStruct *AutoVacuumShmem; @@ -820,7 +821,7 @@ HandleAutoVacLauncherInterrupts(void) AutoVacLauncherShutdown(); /* rebalance in case the default cost parameters changed */ - LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); + LWLockAcquire(AutovacuumLock, LW_SHARED); autovac_balance_cost(); LWLockRelease(AutovacuumLock); @@ -1756,9 +1757,6 @@ FreeWorkerInfo(int code, Datum arg) 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; dlist_push_head(&AutoVacuumShmem->av_freeWorkers, &MyWorkerInfo->wi_links); /* not mine anymore */ @@ -1774,99 +1772,95 @@ 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 VacuumCostDelay with the correct value for an autovacuum worker, + * given the value of other relevant cost-based delay parameters. Autovacuum + * workers should call this after every config reload, in case VacuumCostDelay + * was overwritten. */ void AutoVacuumUpdateDelay(void) { - if (MyWorkerInfo) + if (!am_autovacuum_worker) + return; + + if (av_table_option_cost_delay >= 0) + VacuumCostDelay = av_table_option_cost_delay; + else + VacuumCostDelay = autovacuum_vac_cost_delay >= 0 ? + autovacuum_vac_cost_delay : VacuumCostDelay; +} + +/* + * 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 nworkers_for_balance has been updated by another + * worker or by the autovacuum launcher. They also must call this after every + * config reload, in case VacuumCostLimit was overwritten. + */ +void +AutoVacuumUpdateLimit(void) +{ + if (!am_autovacuum_worker) + return; + + /* + * note: in cost_limit, zero also means use value from elsewhere, because + * zero is not a valid value. + */ + if (av_table_option_cost_limit > 0) + VacuumCostLimit = av_table_option_cost_limit; + else { - VacuumCostDelay = MyWorkerInfo->wi_cost_delay; - VacuumCostLimit = MyWorkerInfo->wi_cost_limit; + int vac_cost_limit = autovacuum_vac_cost_limit > 0 ? + autovacuum_vac_cost_limit : VacuumCostLimit; + + int balanced_cost_limit = vac_cost_limit / + pg_atomic_read_u32(&AutoVacuumShmem->nworkers_for_balance); + + VacuumCostLimit = Max(Min(balanced_cost_limit, vac_cost_limit), 1); } } /* * autovac_balance_cost - * Recalculate the cost limit setting for each active worker. + * Recalculate the number of workers to consider, given table options and + * the current number of active workers. * - * Caller must hold the AutovacuumLock in exclusive mode. + * Caller must hold the AutovacuumLock in at least shared mode. */ static void autovac_balance_cost(void) { - /* - * 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; + int orig_nworkers_for_balance; + int nworkers_for_balance = 0; - /* not set? nothing to do */ - if (vac_cost_limit <= 0 || vac_cost_delay <= 0) + if (autovacuum_vac_cost_delay == 0 || + (autovacuum_vac_cost_delay == -1 && VacuumCostDelay == 0)) return; - /* calculate the total base cost limit of participating active workers */ - cost_total = 0.0; - 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) - cost_total += - (double) worker->wi_cost_limit_base / worker->wi_cost_delay; - } - - /* there are no cost limits -- nothing to do */ - if (cost_total <= 0) + if (autovacuum_vac_cost_limit <= 0 && VacuumCostLimit <= 0) return; - /* - * 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; + orig_nworkers_for_balance = + pg_atomic_read_u32(&AutoVacuumShmem->nworkers_for_balance); + 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 || !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++; } + + nworkers_for_balance = Max(nworkers_for_balance, 1); + + if (nworkers_for_balance != orig_nworkers_for_balance) + pg_atomic_write_u32(&AutoVacuumShmem->nworkers_for_balance, + nworkers_for_balance); } /* @@ -2312,14 +2306,15 @@ do_autovacuum(void) autovac_table *tab; bool isshared; bool skipit; - double stdVacuumCostDelay; - int stdVacuumCostLimit; dlist_iter iter; CHECK_FOR_INTERRUPTS(); /* * Check for config changes before processing each collected table. + * Autovacuum workers must update VacuumCostDelay and VacuumCostLimit + * in case they were overridden by the reload. However, we will do + * this as soon as we check table options a bit later. */ if (ConfigReloadPending) { @@ -2416,32 +2411,18 @@ 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; + av_table_option_cost_limit = tab->at_table_option_vac_cost_limit; + av_table_option_cost_delay = tab->at_table_option_vac_cost_delay; /* Must hold AutovacuumLock while mucking with cost balance info */ LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); - - /* 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; - - /* do a balance */ autovac_balance_cost(); + LWLockRelease(AutovacuumLock); - /* set the active cost parameters from the result of that */ + AutoVacuumUpdateLimit(); AutoVacuumUpdateDelay(); - /* done */ - LWLockRelease(AutovacuumLock); - /* clean up memory before each iteration */ MemoryContextResetAndDeleteChildren(PortalContext); @@ -2534,10 +2515,6 @@ deleted: MyWorkerInfo->wi_tableoid = InvalidOid; MyWorkerInfo->wi_sharedrel = false; LWLockRelease(AutovacuumScheduleLock); - - /* restore vacuum cost GUCs for the next iteration */ - VacuumCostDelay = stdVacuumCostDelay; - VacuumCostLimit = stdVacuumCostLimit; } /* @@ -2569,6 +2546,8 @@ deleted: { ConfigReloadPending = false; ProcessConfigFile(PGC_SIGHUP); + AutoVacuumUpdateDelay(); + AutoVacuumUpdateLimit(); } LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); @@ -2801,8 +2780,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; /* @@ -2812,20 +2789,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 @@ -2881,8 +2844,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_table_option_vac_cost_limit = avopts ? + avopts->vacuum_cost_limit : 0; + tab->at_table_option_vac_cost_delay = avopts ? + avopts->vacuum_cost_delay : -1; tab->at_relname = NULL; tab->at_nspname = NULL; tab->at_datname = NULL; @@ -3374,10 +3339,15 @@ 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); + + /* initialize to 1, as it should be a minimum of 1 */ + pg_atomic_init_u32(&AutoVacuumShmem->nworkers_for_balance, 1); + } else Assert(found); diff --git a/src/include/postmaster/autovacuum.h b/src/include/postmaster/autovacuum.h index c140371b51..558358911c 100644 --- a/src/include/postmaster/autovacuum.h +++ b/src/include/postmaster/autovacuum.h @@ -66,6 +66,8 @@ extern void AutoVacWorkerFailed(void); /* autovacuum cost-delay balancer */ extern void AutoVacuumUpdateDelay(void); +extern void AutoVacuumUpdateLimit(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] 11+ messages in thread
* Re: Should vacuum process config file reload more often 2023-03-15 05:13 Re: Should vacuum process config file reload more often Masahiko Sawada <[email protected]> 2023-03-18 22:47 ` Re: Should vacuum process config file reload more often Melanie Plageman <[email protected]> @ 2023-03-19 16:48 ` Melanie Plageman <[email protected]> 1 sibling, 0 replies; 11+ messages in thread From: Melanie Plageman @ 2023-03-19 16:48 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: pgsql-hackers; Andres Freund <[email protected]>; Amit Kapila <[email protected]> On Sat, Mar 18, 2023 at 6:47 PM Melanie Plageman <[email protected]> wrote: > On Wed, Mar 15, 2023 at 1:14 AM Masahiko Sawada <[email protected]> wrote: > > On Sat, Mar 11, 2023 at 8:11 AM Melanie Plageman > > <[email protected]> wrote: > > > > > Also not sure how the patch interacts with failsafe autovac and parallel > > > > > vacuum. > > > > > > > > Good point. > > > > > > > > When entering the failsafe mode, we disable the vacuum delays (see > > > > lazy_check_wraparound_failsafe()). We need to keep disabling the > > > > vacuum delays even after reloading the config file. One idea is to > > > > have another global variable indicating we're in the failsafe mode. > > > > vacuum_delay_point() doesn't update VacuumCostActive if the flag is > > > > true. > > > > > > I think we might not need to do this. Other than in > > > lazy_check_wraparound_failsafe(), VacuumCostActive is only updated in > > > two places: > > > > > > 1) in vacuum() which autovacuum will call per table. And failsafe is > > > reset per table as well. > > > > > > 2) in vacuum_delay_point(), but, since VacuumCostActive will already be > > > false when we enter vacuum_delay_point() the next time after > > > lazy_check_wraparound_failsafe(), we won't set VacuumCostActive there. > > > > Indeed. But does it mean that there is no code path to turn > > vacuum-delay on, even when vacuum_cost_delay is updated from 0 to > > non-0? > > Ah yes! Good point. This is true. > I'm not sure how to cheaply allow for re-enabling delays after disabling > them in the middle of a table vacuum. > > I don't see a way around checking if we need to reload the config file > on every call to vacuum_delay_point() (currently, we are only doing this > when we have to wait anyway). It seems expensive to do this check every > time. If we do do this, we would update VacuumCostActive when updating > VacuumCostDelay, and we would need a global variable keeping the > failsafe status, as you mentioned. > > It could be okay to say that you can only disable cost-based delays in > the middle of vacuuming a table (i.e. you cannot enable them if they are > already disabled until you start vacuuming the next table). Though maybe > it is weird that you can increase the delay but not re-enable it... So, I thought about it some more, and I think it is a bit odd that you can increase the delay and limit but not re-enable them if they were disabled. And, perhaps it would be okay to check ConfigReloadPending at the top of vacuum_delay_point() instead of only after sleeping. It is just one more branch. We can check if VacuumCostActive is false after checking if we should reload and doing so if needed and return early. I've implemented that in attached v6. I added in the global we discussed for VacuumFailsafeActive. If we keep it, we can probably remove the one in LVRelState -- as it seems redundant. Let me know what you think. - Melanie Attachments: [text/x-patch] v6-0001-auto-vacuum-reloads-config-file-more-often.patch (21.2K, ../../CAAKRu_YUar3FuGS9Xwh9jjw=9neL7H=5-2M4Uq5VUrB=3mbDWQ@mail.gmail.com/2-v6-0001-auto-vacuum-reloads-config-file-more-often.patch) download | inline diff: From 1218c1852794a1310d25359a37b87d068282500e Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Sat, 18 Mar 2023 15:42:39 -0400 Subject: [PATCH v6] [auto]vacuum reloads config file more often Previously, VACUUM and autovacuum workers would reload the configuration file only between vacuuming tables. This precluded user updates to cost-based delay parameters from taking effect while vacuuming a table. Check if a reload is pending roughly once per block now, when checking if we need to delay. In order for this change to have the intended effect on autovacuum, autovacuum workers must start updating their cost delay more frequently as well. With this new paradigm, balancing the cost limit amongst workers also must work differently. Previously, a 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 table options could still have different values for their wi_cost_limit_base and wi_cost_delay. With this change, workers will (within some margin of error) have no reason to have different values for cost limit and cost delay (in the absence of table options). Thus, remove cost limit and cost delay from shared memory and keep track only of the number of workers actively vacuuming tables with no cost-related table options. Then, use this value to determine what each worker's effective cost limit should be. Reviewed-by: Masahiko Sawada <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_buP5wzsho3qNw5o9_R0pF69FRM5hgCmr-mvXmGXwdA7A%40mail.gmail.com#5e6771d4cdca4db6efc2acec2dce0bc7 --- src/backend/access/heap/vacuumlazy.c | 1 + src/backend/commands/vacuum.c | 55 +++++-- src/backend/commands/vacuumparallel.c | 1 + src/backend/postmaster/autovacuum.c | 209 +++++++++++--------------- src/backend/utils/init/globals.c | 1 + src/include/miscadmin.h | 1 + src/include/postmaster/autovacuum.h | 2 + 7 files changed, 142 insertions(+), 128 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 8f14cf85f3..38dce9ae66 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -2622,6 +2622,7 @@ lazy_check_wraparound_failsafe(LVRelState *vacrel) if (unlikely(vacuum_xid_failsafe_check(&vacrel->cutoffs))) { vacrel->failsafe_active = true; + VacuumFailsafeActive = true; /* Disable index vacuuming, index cleanup, and heap rel truncation */ vacrel->do_index_vacuuming = false; diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index c54360a6a0..a5eb22c5ca 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" @@ -76,6 +77,7 @@ int vacuum_multixact_failsafe_age; /* A few variables that don't seem worth passing around as parameters */ static MemoryContext vac_context = NULL; static BufferAccessStrategy vac_strategy; +static bool analyze_in_outer_xact = false; /* @@ -314,8 +316,7 @@ vacuum(List *relations, VacuumParams *params, static bool in_vacuum = false; const char *stmttype; - volatile bool in_outer_xact, - use_own_xacts; + volatile bool use_own_xacts; Assert(params != NULL); @@ -332,10 +333,10 @@ vacuum(List *relations, VacuumParams *params, if (params->options & VACOPT_VACUUM) { PreventInTransactionBlock(isTopLevel, stmttype); - in_outer_xact = false; + analyze_in_outer_xact = false; } else - in_outer_xact = IsInTransactionBlock(isTopLevel); + analyze_in_outer_xact = IsInTransactionBlock(isTopLevel); /* * Due to static variables vac_context, anl_context and vac_strategy, @@ -457,7 +458,7 @@ vacuum(List *relations, VacuumParams *params, Assert(params->options & VACOPT_ANALYZE); if (IsAutoVacuumWorkerProcess()) use_own_xacts = true; - else if (in_outer_xact) + else if (analyze_in_outer_xact) use_own_xacts = false; else if (list_length(relations) > 1) use_own_xacts = true; @@ -475,7 +476,7 @@ vacuum(List *relations, VacuumParams *params, */ if (use_own_xacts) { - Assert(!in_outer_xact); + Assert(!analyze_in_outer_xact); /* ActiveSnapshot is not set by autovacuum */ if (ActiveSnapshotSet()) @@ -492,6 +493,7 @@ vacuum(List *relations, VacuumParams *params, in_vacuum = true; VacuumCostActive = (VacuumCostDelay > 0); + VacuumFailsafeActive = false; VacuumCostBalance = 0; VacuumPageHit = 0; VacuumPageMiss = 0; @@ -527,7 +529,7 @@ vacuum(List *relations, VacuumParams *params, } analyze_rel(vrel->oid, vrel->relation, params, - vrel->va_cols, in_outer_xact, vac_strategy); + vrel->va_cols, analyze_in_outer_xact, vac_strategy); if (use_own_xacts) { @@ -550,6 +552,7 @@ vacuum(List *relations, VacuumParams *params, { in_vacuum = false; VacuumCostActive = false; + analyze_in_outer_xact = false; } PG_END_TRY(); @@ -1850,6 +1853,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params, bool skip_privs) Assert(params != NULL); + VacuumFailsafeActive = false; + /* Begin a transaction for vacuuming this relation */ StartTransactionCommand(); @@ -2215,9 +2220,33 @@ vacuum_delay_point(void) /* Always check for interrupts */ CHECK_FOR_INTERRUPTS(); - if (!VacuumCostActive || InterruptPending) + if (InterruptPending || VacuumFailsafeActive || + (!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 && !analyze_in_outer_xact) + { + ConfigReloadPending = false; + ProcessConfigFile(PGC_SIGHUP); + AutoVacuumUpdateDelay(); + AutoVacuumUpdateLimit(); + } + + /* + * If we disabled cost-based delays after reloading the config file, + * return + */ + if (!VacuumCostActive) + { + VacuumCostBalance = 0; + return; + } + /* * For parallel vacuum, the delay is computed based on the shared cost * balance. See compute_parallel_delay. @@ -2248,8 +2277,14 @@ vacuum_delay_point(void) VacuumCostBalance = 0; - /* update balance values for workers */ - AutoVacuumUpdateDelay(); + /* + * Update limit values for 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. + */ + AutoVacuumUpdateLimit(); /* 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 bcd40c80a1..57188500d0 100644 --- a/src/backend/commands/vacuumparallel.c +++ b/src/backend/commands/vacuumparallel.c @@ -990,6 +990,7 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc) false); /* Set cost-based vacuum delay */ + VacuumFailsafeActive = false; VacuumCostActive = (VacuumCostDelay > 0); VacuumCostBalance = 0; VacuumPageHit = 0; diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index c0e2e00a7e..1033e6db62 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -139,6 +139,9 @@ int Log_autovacuum_min_duration = 600000; static bool am_autovacuum_launcher = false; static bool am_autovacuum_worker = false; +static double av_table_option_cost_delay = -1; +static int av_table_option_cost_limit = 0; + /* Flags set by signal handlers */ static volatile sig_atomic_t got_SIGUSR2 = false; @@ -189,8 +192,8 @@ typedef struct autovac_table { Oid at_relid; VacuumParams at_params; - double at_vacuum_cost_delay; - int at_vacuum_cost_limit; + double at_table_option_vac_cost_delay; + int at_table_option_vac_cost_limit; bool at_dobalance; bool at_sharedrel; char *at_relname; @@ -209,7 +212,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 @@ -225,9 +228,6 @@ typedef struct WorkerInfoData TimestampTz wi_launchtime; bool wi_dobalance; bool wi_sharedrel; - double wi_cost_delay; - int wi_cost_limit; - int wi_cost_limit_base; } WorkerInfoData; typedef struct WorkerInfoData *WorkerInfo; @@ -286,6 +286,7 @@ typedef struct dlist_head av_runningWorkers; WorkerInfo av_startingWorker; AutoVacuumWorkItem av_workItems[NUM_WORKITEMS]; + pg_atomic_uint32 nworkers_for_balance; } AutoVacuumShmemStruct; static AutoVacuumShmemStruct *AutoVacuumShmem; @@ -820,7 +821,7 @@ HandleAutoVacLauncherInterrupts(void) AutoVacLauncherShutdown(); /* rebalance in case the default cost parameters changed */ - LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); + LWLockAcquire(AutovacuumLock, LW_SHARED); autovac_balance_cost(); LWLockRelease(AutovacuumLock); @@ -1756,9 +1757,6 @@ FreeWorkerInfo(int code, Datum arg) 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; dlist_push_head(&AutoVacuumShmem->av_freeWorkers, &MyWorkerInfo->wi_links); /* not mine anymore */ @@ -1774,99 +1772,98 @@ 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 VacuumCostDelay with the correct value for an autovacuum worker, + * given the value of other relevant cost-based delay parameters. Autovacuum + * workers should call this after every config reload, in case VacuumCostDelay + * was overwritten. */ void AutoVacuumUpdateDelay(void) { - if (MyWorkerInfo) + if (!am_autovacuum_worker) + return; + + if (av_table_option_cost_delay >= 0) + VacuumCostDelay = av_table_option_cost_delay; + else + VacuumCostDelay = autovacuum_vac_cost_delay >= 0 ? + autovacuum_vac_cost_delay : VacuumCostDelay; + + if (!VacuumFailsafeActive) + VacuumCostActive = (VacuumCostDelay > 0); +} + +/* + * 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 nworkers_for_balance has been updated by another + * worker or by the autovacuum launcher. They also must call this after every + * config reload, in case VacuumCostLimit was overwritten. + */ +void +AutoVacuumUpdateLimit(void) +{ + if (!am_autovacuum_worker) + return; + + /* + * note: in cost_limit, zero also means use value from elsewhere, because + * zero is not a valid value. + */ + if (av_table_option_cost_limit > 0) + VacuumCostLimit = av_table_option_cost_limit; + else { - VacuumCostDelay = MyWorkerInfo->wi_cost_delay; - VacuumCostLimit = MyWorkerInfo->wi_cost_limit; + int vac_cost_limit = autovacuum_vac_cost_limit > 0 ? + autovacuum_vac_cost_limit : VacuumCostLimit; + + int balanced_cost_limit = vac_cost_limit / + pg_atomic_read_u32(&AutoVacuumShmem->nworkers_for_balance); + + VacuumCostLimit = Max(Min(balanced_cost_limit, vac_cost_limit), 1); } } /* * autovac_balance_cost - * Recalculate the cost limit setting for each active worker. + * Recalculate the number of workers to consider, given table options and + * the current number of active workers. * - * Caller must hold the AutovacuumLock in exclusive mode. + * Caller must hold the AutovacuumLock in at least shared mode. */ static void autovac_balance_cost(void) { - /* - * 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; + int orig_nworkers_for_balance; + int nworkers_for_balance = 0; - /* not set? nothing to do */ - if (vac_cost_limit <= 0 || vac_cost_delay <= 0) + if (autovacuum_vac_cost_delay == 0 || + (autovacuum_vac_cost_delay == -1 && VacuumCostDelay == 0)) return; - /* calculate the total base cost limit of participating active workers */ - cost_total = 0.0; - 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) - cost_total += - (double) worker->wi_cost_limit_base / worker->wi_cost_delay; - } - - /* there are no cost limits -- nothing to do */ - if (cost_total <= 0) + if (autovacuum_vac_cost_limit <= 0 && VacuumCostLimit <= 0) return; - /* - * 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; + orig_nworkers_for_balance = + pg_atomic_read_u32(&AutoVacuumShmem->nworkers_for_balance); + 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 || !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++; } + + nworkers_for_balance = Max(nworkers_for_balance, 1); + + if (nworkers_for_balance != orig_nworkers_for_balance) + pg_atomic_write_u32(&AutoVacuumShmem->nworkers_for_balance, + nworkers_for_balance); } /* @@ -2312,14 +2309,15 @@ do_autovacuum(void) autovac_table *tab; bool isshared; bool skipit; - double stdVacuumCostDelay; - int stdVacuumCostLimit; dlist_iter iter; CHECK_FOR_INTERRUPTS(); /* * Check for config changes before processing each collected table. + * Autovacuum workers must update VacuumCostDelay and VacuumCostLimit + * in case they were overridden by the reload. However, we will do + * this as soon as we check table options a bit later. */ if (ConfigReloadPending) { @@ -2416,32 +2414,18 @@ 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; + av_table_option_cost_limit = tab->at_table_option_vac_cost_limit; + av_table_option_cost_delay = tab->at_table_option_vac_cost_delay; /* Must hold AutovacuumLock while mucking with cost balance info */ LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); - - /* 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; - - /* do a balance */ autovac_balance_cost(); + LWLockRelease(AutovacuumLock); - /* set the active cost parameters from the result of that */ + AutoVacuumUpdateLimit(); AutoVacuumUpdateDelay(); - /* done */ - LWLockRelease(AutovacuumLock); - /* clean up memory before each iteration */ MemoryContextResetAndDeleteChildren(PortalContext); @@ -2534,10 +2518,6 @@ deleted: MyWorkerInfo->wi_tableoid = InvalidOid; MyWorkerInfo->wi_sharedrel = false; LWLockRelease(AutovacuumScheduleLock); - - /* restore vacuum cost GUCs for the next iteration */ - VacuumCostDelay = stdVacuumCostDelay; - VacuumCostLimit = stdVacuumCostLimit; } /* @@ -2569,6 +2549,8 @@ deleted: { ConfigReloadPending = false; ProcessConfigFile(PGC_SIGHUP); + AutoVacuumUpdateDelay(); + AutoVacuumUpdateLimit(); } LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); @@ -2801,8 +2783,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; /* @@ -2812,20 +2792,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 @@ -2881,8 +2847,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_table_option_vac_cost_limit = avopts ? + avopts->vacuum_cost_limit : 0; + tab->at_table_option_vac_cost_delay = avopts ? + avopts->vacuum_cost_delay : -1; tab->at_relname = NULL; tab->at_nspname = NULL; tab->at_datname = NULL; @@ -3374,10 +3342,15 @@ 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); + + /* initialize to 1, as it should be a minimum of 1 */ + pg_atomic_init_u32(&AutoVacuumShmem->nworkers_for_balance, 1); + } else Assert(found); diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c index 1b1d814254..aeb8ed0e46 100644 --- a/src/backend/utils/init/globals.c +++ b/src/backend/utils/init/globals.c @@ -151,3 +151,4 @@ int64 VacuumPageDirty = 0; int VacuumCostBalance = 0; /* working state for vacuum */ bool VacuumCostActive = false; +bool VacuumFailsafeActive = false; diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 06a86f9ac1..b1297677d3 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -275,6 +275,7 @@ extern PGDLLIMPORT int64 VacuumPageDirty; extern PGDLLIMPORT int VacuumCostBalance; extern PGDLLIMPORT bool VacuumCostActive; +extern PGDLLIMPORT bool VacuumFailsafeActive; /* in tcop/postgres.c */ diff --git a/src/include/postmaster/autovacuum.h b/src/include/postmaster/autovacuum.h index c140371b51..558358911c 100644 --- a/src/include/postmaster/autovacuum.h +++ b/src/include/postmaster/autovacuum.h @@ -66,6 +66,8 @@ extern void AutoVacWorkerFailed(void); /* autovacuum cost-delay balancer */ extern void AutoVacuumUpdateDelay(void); +extern void AutoVacuumUpdateLimit(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] 11+ messages in thread
* Re: Should vacuum process config file reload more often 2023-03-15 05:13 Re: Should vacuum process config file reload more often Masahiko Sawada <[email protected]> 2023-03-18 22:47 ` Re: Should vacuum process config file reload more often Melanie Plageman <[email protected]> @ 2023-03-23 06:08 ` Masahiko Sawada <[email protected]> 2023-03-23 16:24 ` Re: Should vacuum process config file reload more often Daniel Gustafsson <[email protected]> 1 sibling, 1 reply; 11+ messages in thread From: Masahiko Sawada @ 2023-03-23 06:08 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: pgsql-hackers; Andres Freund <[email protected]>; Amit Kapila <[email protected]> On Sun, Mar 19, 2023 at 7:47 AM Melanie Plageman <[email protected]> wrote: > > On Wed, Mar 15, 2023 at 1:14 AM Masahiko Sawada <[email protected]> wrote: > > On Sat, Mar 11, 2023 at 8:11 AM Melanie Plageman > > <[email protected]> wrote: > > > I've implemented the atomic cost limit in the attached patch. Though, > > > I'm pretty unsure about how I initialized the atomics in > > > AutoVacuumShmemInit()... > > > > + > > /* initialize the WorkerInfo free list */ > > for (i = 0; i < autovacuum_max_workers; i++) > > dlist_push_head(&AutoVacuumShmem->av_freeWorkers, > > &worker[i].wi_links); > > + > > + dlist_foreach(iter, &AutoVacuumShmem->av_freeWorkers) > > + pg_atomic_init_u32( > > + > > &(dlist_container(WorkerInfoData, wi_links, iter.cur))->wi_cost_limit, > > + 0); > > + > > > > I think we can do like: > > > > /* 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_u32(&(worker[i].wi_cost_limit)); > > } > > Ah, yes, I was distracted by the variable name "worker" (as opposed to > "workers"). > > > > If the consensus is that it is simply too confusing to take > > > wi_cost_delay out of WorkerInfo, we might be able to afford using a > > > shared lock to access it because we won't call AutoVacuumUpdateDelay() > > > on every invocation of vacuum_delay_point() -- only when we've reloaded > > > the config file. > > > > > > One potential option to avoid taking a shared lock on every call to > > > AutoVacuumUpdateDelay() is to set a global variable to indicate that we > > > did update it (since we are the only ones updating it) and then only > > > take the shared LWLock in AutoVacuumUpdateDelay() if that flag is true. > > > > > > > If we remove wi_cost_delay from WorkerInfo, probably we don't need to > > acquire the lwlock in AutoVacuumUpdateDelay()? The shared field we > > access in that function will be only wi_dobalance, but this field is > > updated only by its owner autovacuum worker. > > I realized that we cannot use dobalance to decide whether or not to > update wi_cost_delay because dobalance could be false because of table > option cost limit being set (with no table option cost delay) and we > would still need to update VacuumCostDelay and wi_cost_delay with the > new value of autovacuum_vacuum_cost_delay. > > But v5 skirts around this issue altogether. > > > > > --- > > > > void > > > > AutoVacuumUpdateDelay(void) > > > > { > > > > - if (MyWorkerInfo) > > > > + /* > > > > + * We are using autovacuum-related GUCs to update > > > > VacuumCostDelay, so we > > > > + * only want autovacuum workers and autovacuum launcher to do this. > > > > + */ > > > > + if (!(am_autovacuum_worker || am_autovacuum_launcher)) > > > > + return; > > > > > > > > Is there any case where the autovacuum launcher calls > > > > AutoVacuumUpdateDelay() function? > > > > > > I had meant to add it to HandleAutoVacLauncherInterrupts() after > > > reloading the config file (done in attached patch). When using the > > > global variables for cost delay (instead of wi_cost_delay in worker > > > info), the autovac launcher also has to do the check in the else branch > > > of AutoVacuumUpdateDelay() > > > > > > VacuumCostDelay = autovacuum_vac_cost_delay >= 0 ? > > > autovacuum_vac_cost_delay : VacuumCostDelay; > > > > > > to make sure VacuumCostDelay is correct for when it calls > > > autovac_balance_cost(). > > > > But doesn't the launcher do a similar thing at the beginning of > > autovac_balance_cost()? > > > > double vac_cost_delay = (autovacuum_vac_cost_delay >= 0 ? > > autovacuum_vac_cost_delay : VacuumCostDelay); > > Ah, yes. You are right. > > > Related to this point, I think autovac_balance_cost() should use > > globally-set cost_limit and cost_delay values to calculate worker's > > vacuum-delay parameters. IOW, vac_cost_limit and vac_cost_delay should > > come from the config file setting, not table option etc: > > > > 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); > > > > If my understanding is right, the following change is not right; > > AutoVacUpdateLimit() updates the VacuumCostLimit based on the value in > > MyWorkerInfo: > > > > MyWorkerInfo->wi_cost_limit_base = tab->at_vacuum_cost_limit; > > + AutoVacuumUpdateLimit(); > > > > /* do a balance */ > > autovac_balance_cost(); > > > > - /* set the active cost parameters from the result of that */ > > - AutoVacuumUpdateDelay(); > > > > Also, even when using the global variables for cost delay, the > > launcher doesn't need to check the global variable. It should always > > be able to use either autovacuum_vac_cost_delay/limit or > > VacuumCostDelay/Limit. > > Yes, that is true. But, I actually think we can do something more > radical, which relates to this point as well as the issue with > cost_limit_base below. > > > > This also made me think about whether or not we still need cost_limit_base. > > > It is used to ensure that autovac_balance_cost() never ends up setting > > > workers' wi_cost_limits above the current autovacuum_vacuum_cost_limit > > > (or VacuumCostLimit). However, the launcher and all the workers should > > > know what the value is without cost_limit_base, no? > > > > Yeah, the current balancing algorithm looks to respect the cost_limit > > value set when starting to vacuum the table. The proportion of the > > amount of I/O that a worker can consume is calculated based on the > > base value and the new worker's cost_limit value cannot exceed the > > base value. Given that we're trying to dynamically tune worker's cost > > parameters (delay and limit), this concept seems to need to be > > updated. > > In master, autovacuum workers reload the config file at most once per > table vacuumed. And that is the same time that they update their > wi_cost_limit_base and wi_cost_delay. Thus, when autovac_balance_cost() > is called, there is a good chance that different workers will have > different values for wi_cost_limit_base and wi_cost_delay (and we are > only talking about workers not vacuuming a table with table option > cost-related gucs). So, it made sense that the balancing algorithm tried > to use a ratio to determine what to set the cost limit of each worker > to. It is clamped to the base value, as you say, but it also gives > workers a proportion of the new limit equal to what proportion their base > cost represents of the total cost. > > I think all of this doesn't matter anymore now that everyone can reload > the config file often and dynamically change these values. > > Thus, in the attached v5, I have removed both wi_cost_limit and wi_cost_delay > from WorkerInfo. I've added a new variable to AutoVacuumShmem called > nworkers_for_balance. Now, autovac_balance_cost() only recalculates this > number and updates it if it has changed. Then, in > AutoVacuumUpdateLimit() workers read from this atomic value and divide > the value of the cost limit gucs by that number to get their own cost limit. > > I keep the table option value of cost limit and cost delay in > backend-local memory to reference when updating the worker cost limit. > > One nice thing is autovac_balance_cost() only requires an access shared > lock now (though most callers are updating other members before calling > it and still take an exclusive lock). > > What do you think? I think this is a good idea. Do we need to calculate the number of workers running with nworkers_for_balance by iterating over the running worker list? I guess autovacuum workers can increment/decrement it at the beginning and end of vacuum. > > > > > > Also not sure how the patch interacts with failsafe autovac and parallel > > > > > vacuum. > > > > > > > > Good point. > > > > > > > > When entering the failsafe mode, we disable the vacuum delays (see > > > > lazy_check_wraparound_failsafe()). We need to keep disabling the > > > > vacuum delays even after reloading the config file. One idea is to > > > > have another global variable indicating we're in the failsafe mode. > > > > vacuum_delay_point() doesn't update VacuumCostActive if the flag is > > > > true. > > > > > > I think we might not need to do this. Other than in > > > lazy_check_wraparound_failsafe(), VacuumCostActive is only updated in > > > two places: > > > > > > 1) in vacuum() which autovacuum will call per table. And failsafe is > > > reset per table as well. > > > > > > 2) in vacuum_delay_point(), but, since VacuumCostActive will already be > > > false when we enter vacuum_delay_point() the next time after > > > lazy_check_wraparound_failsafe(), we won't set VacuumCostActive there. > > > > Indeed. But does it mean that there is no code path to turn > > vacuum-delay on, even when vacuum_cost_delay is updated from 0 to > > non-0? > > Ah yes! Good point. This is true. > I'm not sure how to cheaply allow for re-enabling delays after disabling > them in the middle of a table vacuum. > > I don't see a way around checking if we need to reload the config file > on every call to vacuum_delay_point() (currently, we are only doing this > when we have to wait anyway). It seems expensive to do this check every > time. If we do do this, we would update VacuumCostActive when updating > VacuumCostDelay, and we would need a global variable keeping the > failsafe status, as you mentioned. > > It could be okay to say that you can only disable cost-based delays in > the middle of vacuuming a table (i.e. you cannot enable them if they are > already disabled until you start vacuuming the next table). Though maybe > it is weird that you can increase the delay but not re-enable it... On Mon, Mar 20, 2023 at 1:48 AM Melanie Plageman <[email protected]> wrote: > So, I thought about it some more, and I think it is a bit odd that you > can increase the delay and limit but not re-enable them if they were > disabled. And, perhaps it would be okay to check ConfigReloadPending at > the top of vacuum_delay_point() instead of only after sleeping. It is > just one more branch. We can check if VacuumCostActive is false after > checking if we should reload and doing so if needed and return early. > I've implemented that in attached v6. > > I added in the global we discussed for VacuumFailsafeActive. If we keep > it, we can probably remove the one in LVRelState -- as it seems > redundant. Let me know what you think. I think the following change is related: - if (!VacuumCostActive || InterruptPending) + if (InterruptPending || VacuumFailsafeActive || + (!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 && !analyze_in_outer_xact) + { + ConfigReloadPending = false; + ProcessConfigFile(PGC_SIGHUP); + AutoVacuumUpdateDelay(); + AutoVacuumUpdateLimit(); + } It makes sense to me that we need to reload the config file even when vacuum-delay is disabled. But I think it's not convenient for users that we don't reload the configuration file once the failsafe is triggered. I think users might want to change some GUCs such as log_autovacuum_min_duration. > > On an unrelated note, I was wondering if there were any docs anywhere > that should be updated to go along with this. The current patch improves the internal mechanism of (re)balancing vacuum-cost but doesn't change user-visible behavior. I don't have any idea so far that we should update somewhere in the doc. > > And, I was wondering if it was worth trying to split up the part that > reloads the config file and all of the autovacuum stuff. The reloading > of the config file by itself won't actually result in autovacuum workers > having updated cost delays because of them overwriting it with > wi_cost_delay, but it will allow VACUUM to have those updated values. It makes sense to me to have changes for overhauling the rebalance mechanism in a separate patch. Looking back at the original concern you mentioned[1]: speed up long-running vacuum of a large table by decreasing autovacuum_vacuum_cost_delay/vacuum_cost_delay, however the config file is only reloaded between tables (for autovacuum) or after the statement (for explicit vacuum). does it make sense to have autovac_balance_cost() update workers' wi_cost_delay too? Autovacuum launcher already reloads the config file and does the rebalance. So I thought autovac_balance_cost() can update the cost_delay as well, and this might be a minimal change to deal with your concern. This doesn't have the effect for manual VACUUM but since vacuum delay is disabled by default it won't be a big problem. As for manual VACUUMs, we would need to reload the config file in vacuum_delay_point() as the part of your patch does. Overhauling the rebalance mechanism would be another patch to improve it further. Regards, [1] https://www.postgresql.org/message-id/CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7%2BDXhrjDA0gw%40mail.g... -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Should vacuum process config file reload more often 2023-03-15 05:13 Re: Should vacuum process config file reload more often Masahiko Sawada <[email protected]> 2023-03-18 22:47 ` Re: Should vacuum process config file reload more often Melanie Plageman <[email protected]> 2023-03-23 06:08 ` Re: Should vacuum process config file reload more often Masahiko Sawada <[email protected]> @ 2023-03-23 16:24 ` Daniel Gustafsson <[email protected]> 2023-03-24 00:27 ` Re: Should vacuum process config file reload more often Melanie Plageman <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Daniel Gustafsson @ 2023-03-23 16:24 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: Melanie Plageman <[email protected]>; pgsql-hackers; Andres Freund <[email protected]>; Amit Kapila <[email protected]> > On 23 Mar 2023, at 07:08, Masahiko Sawada <[email protected]> wrote: > On Sun, Mar 19, 2023 at 7:47 AM Melanie Plageman <[email protected]> wrote: > It makes sense to me that we need to reload the config file even when > vacuum-delay is disabled. But I think it's not convenient for users > that we don't reload the configuration file once the failsafe is > triggered. I think users might want to change some GUCs such as > log_autovacuum_min_duration. I agree with this. >> On an unrelated note, I was wondering if there were any docs anywhere >> that should be updated to go along with this. > > The current patch improves the internal mechanism of (re)balancing > vacuum-cost but doesn't change user-visible behavior. I don't have any > idea so far that we should update somewhere in the doc. I had a look as well and can't really spot anywhere where the current behavior is detailed, so there is little to update. On top of that, I also don't think it's worth adding this to the docs. >> And, I was wondering if it was worth trying to split up the part that >> reloads the config file and all of the autovacuum stuff. The reloading >> of the config file by itself won't actually result in autovacuum workers >> having updated cost delays because of them overwriting it with >> wi_cost_delay, but it will allow VACUUM to have those updated values. > > It makes sense to me to have changes for overhauling the rebalance > mechanism in a separate patch. It would for sure be worth considering, +bool VacuumFailsafeActive = false; This needs documentation, how it's used and how it relates to failsafe_active in LVRelState (which it might replace(?), but until then). + pg_atomic_uint32 nworkers_for_balance; This needs a short oneline documentation update to the struct comment. - double wi_cost_delay; - int wi_cost_limit; - int wi_cost_limit_base; This change makes the below comment in do_autovacuum in need of an update: /* * 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. */ + if (av_table_option_cost_delay >= 0) + VacuumCostDelay = av_table_option_cost_delay; + else + VacuumCostDelay = autovacuum_vac_cost_delay >= 0 ? + autovacuum_vac_cost_delay : VacuumCostDelay; While it's a matter of personal preference, I for one would like if we reduced the number of ternary operators in the vacuum code, especially those mixed into if statements. The vacuum code is full of this already though so this isn't less of an objection (as it follows style) than an observation. + * 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; Not mentioning the fact that a magic value in a GUC means it's using the value from another GUC (which is not great IMHO), it seems we are using zero as well as -1 as that magic value here? (not introduced in this patch.) The docs does AFAICT only specify -1 as that value though. Am I missing something or is the code and documentation slightly out of sync? I need another few readthroughs to figure out of VacuumFailsafeActive does what I think it does, and should be doing, but in general I think this is a good idea and a patch in good condition close to being committable. -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Should vacuum process config file reload more often 2023-03-15 05:13 Re: Should vacuum process config file reload more often Masahiko Sawada <[email protected]> 2023-03-18 22:47 ` Re: Should vacuum process config file reload more often Melanie Plageman <[email protected]> 2023-03-23 06:08 ` Re: Should vacuum process config file reload more often Masahiko Sawada <[email protected]> 2023-03-23 16:24 ` Re: Should vacuum process config file reload more often Daniel Gustafsson <[email protected]> @ 2023-03-24 00:27 ` Melanie Plageman <[email protected]> 2023-03-24 05:21 ` Re: Should vacuum process config file reload more often Masahiko Sawada <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Melanie Plageman @ 2023-03-24 00:27 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Masahiko Sawada <[email protected]>; pgsql-hackers; Andres Freund <[email protected]>; Amit Kapila <[email protected]> On Thu, Mar 23, 2023 at 2:09 AM Masahiko Sawada <[email protected]> wrote: > On Sun, Mar 19, 2023 at 7:47 AM Melanie Plageman <[email protected]> wrote: > Do we need to calculate the number of workers running with > nworkers_for_balance by iterating over the running worker list? I > guess autovacuum workers can increment/decrement it at the beginning > and end of vacuum. I don't think we can do that because if a worker crashes, we have no way of knowing if it had incremented or decremented the number, so we can't adjust for it. > > > > > > Also not sure how the patch interacts with failsafe autovac and parallel > > > > > > vacuum. > > > > > > > > > > Good point. > > > > > > > > > > When entering the failsafe mode, we disable the vacuum delays (see > > > > > lazy_check_wraparound_failsafe()). We need to keep disabling the > > > > > vacuum delays even after reloading the config file. One idea is to > > > > > have another global variable indicating we're in the failsafe mode. > > > > > vacuum_delay_point() doesn't update VacuumCostActive if the flag is > > > > > true. > > > > > > > > I think we might not need to do this. Other than in > > > > lazy_check_wraparound_failsafe(), VacuumCostActive is only updated in > > > > two places: > > > > > > > > 1) in vacuum() which autovacuum will call per table. And failsafe is > > > > reset per table as well. > > > > > > > > 2) in vacuum_delay_point(), but, since VacuumCostActive will already be > > > > false when we enter vacuum_delay_point() the next time after > > > > lazy_check_wraparound_failsafe(), we won't set VacuumCostActive there. > > > > > > Indeed. But does it mean that there is no code path to turn > > > vacuum-delay on, even when vacuum_cost_delay is updated from 0 to > > > non-0? > > > > Ah yes! Good point. This is true. > > I'm not sure how to cheaply allow for re-enabling delays after disabling > > them in the middle of a table vacuum. > > > > I don't see a way around checking if we need to reload the config file > > on every call to vacuum_delay_point() (currently, we are only doing this > > when we have to wait anyway). It seems expensive to do this check every > > time. If we do do this, we would update VacuumCostActive when updating > > VacuumCostDelay, and we would need a global variable keeping the > > failsafe status, as you mentioned. > > > > It could be okay to say that you can only disable cost-based delays in > > the middle of vacuuming a table (i.e. you cannot enable them if they are > > already disabled until you start vacuuming the next table). Though maybe > > it is weird that you can increase the delay but not re-enable it... > > On Mon, Mar 20, 2023 at 1:48 AM Melanie Plageman > <[email protected]> wrote: > > So, I thought about it some more, and I think it is a bit odd that you > > can increase the delay and limit but not re-enable them if they were > > disabled. And, perhaps it would be okay to check ConfigReloadPending at > > the top of vacuum_delay_point() instead of only after sleeping. It is > > just one more branch. We can check if VacuumCostActive is false after > > checking if we should reload and doing so if needed and return early. > > I've implemented that in attached v6. > > > > I added in the global we discussed for VacuumFailsafeActive. If we keep > > it, we can probably remove the one in LVRelState -- as it seems > > redundant. Let me know what you think. > > I think the following change is related: > > - if (!VacuumCostActive || InterruptPending) > + if (InterruptPending || VacuumFailsafeActive || > + (!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 && !analyze_in_outer_xact) > + { > + ConfigReloadPending = false; > + ProcessConfigFile(PGC_SIGHUP); > + AutoVacuumUpdateDelay(); > + AutoVacuumUpdateLimit(); > + } > > It makes sense to me that we need to reload the config file even when > vacuum-delay is disabled. But I think it's not convenient for users > that we don't reload the configuration file once the failsafe is > triggered. I think users might want to change some GUCs such as > log_autovacuum_min_duration. Ah, okay. Attached v7 has this change (it reloads even if failsafe is active). > > And, I was wondering if it was worth trying to split up the part that > > reloads the config file and all of the autovacuum stuff. The reloading > > of the config file by itself won't actually result in autovacuum workers > > having updated cost delays because of them overwriting it with > > wi_cost_delay, but it will allow VACUUM to have those updated values. > > It makes sense to me to have changes for overhauling the rebalance > mechanism in a separate patch. > > Looking back at the original concern you mentioned[1]: > > speed up long-running vacuum of a large table by > decreasing autovacuum_vacuum_cost_delay/vacuum_cost_delay, however the > config file is only reloaded between tables (for autovacuum) or after > the statement (for explicit vacuum). > > does it make sense to have autovac_balance_cost() update workers' > wi_cost_delay too? Autovacuum launcher already reloads the config file > and does the rebalance. So I thought autovac_balance_cost() can update > the cost_delay as well, and this might be a minimal change to deal > with your concern. This doesn't have the effect for manual VACUUM but > since vacuum delay is disabled by default it won't be a big problem. > As for manual VACUUMs, we would need to reload the config file in > vacuum_delay_point() as the part of your patch does. Overhauling the > rebalance mechanism would be another patch to improve it further. So, we can't do this without acquiring an access shared lock on every call to vacuum_delay_point() because cost delay is a double. I will work on a patchset with separate commits for reloading the config file, though (with autovac not benefitting in the first commit). On Thu, Mar 23, 2023 at 12:24 PM Daniel Gustafsson <[email protected]> wrote: > > +bool VacuumFailsafeActive = false; > This needs documentation, how it's used and how it relates to failsafe_active > in LVRelState (which it might replace(?), but until then). Thanks! I've removed LVRelState->failsafe_active. I've also separated the VacuumFailsafeActive change into its own commit. I will say that that commit message needs some work. > + pg_atomic_uint32 nworkers_for_balance; > This needs a short oneline documentation update to the struct comment. Done. I also prefixed with av to match the other members. I am thinking that this variable name could be better. I want to convey that it is the number of workers sharing a cost limit, so I considered av_limit_sharers or something like that. I am looking to convey that it is the number of workers amongst whom we must split the cost limit. > > - double wi_cost_delay; > - int wi_cost_limit; > - int wi_cost_limit_base; > This change makes the below comment in do_autovacuum in need of an update: > /* > * 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. > */ Updated to mention wi_dobalance instead. On the topic of wi_dobalance, should we bother making it an atomic flag instead? We would avoid taking a lock a few times, though probably not frequently enough to matter. I was wondering if making it atomically accessible would be less confusing than acquiring a lock only to set one member in do_autovacuum() (and otherwise it is only read). I think if I had to make it an atomic flag, I would reverse the logic and make it wi_skip_balance or something like that. > + if (av_table_option_cost_delay >= 0) > + VacuumCostDelay = av_table_option_cost_delay; > + else > + VacuumCostDelay = autovacuum_vac_cost_delay >= 0 ? > + autovacuum_vac_cost_delay : VacuumCostDelay; > While it's a matter of personal preference, I for one would like if we reduced > the number of ternary operators in the vacuum code, especially those mixed into > if statements. The vacuum code is full of this already though so this isn't > less of an objection (as it follows style) than an observation. I agree. This one was better served as an "else if" anyway -- updated! > > + * 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; > Not mentioning the fact that a magic value in a GUC means it's using the value > from another GUC (which is not great IMHO), it seems we are using zero as well > as -1 as that magic value here? (not introduced in this patch.) The docs does > AFAICT only specify -1 as that value though. Am I missing something or is the > code and documentation slightly out of sync? I copied that comment from elsewhere, but, yes it is a weird situation. So, you can set autovacuum_vacuum_cost_limit to 0, -1 or a positive number. You can only set vacuum_cost_limit to a positive value. The documentation mentions that setting autovacuum_vacuum_cost_limit to -1, the default, will have it use vacuum_cost_limit. However, it says nothing about what setting it to 0 does. In the code, everywhere assumes if autovacuum_vacuum_cost_limit is 0 OR -1, use vacuum_cost_limit. This is in contrast to autovacuum_vacuum_cost_delay, for which 0 means to disable it -- so setting autovacuum_vacuum_cost_delay to 0 will specifically not fall back to vacuum_cost_limit. I think the problem is that 0 is not a valid cost limit (i.e. it has no meaning like infinity/no limit), so we basically don't want to allow the cost limit to be set to 0, but GUC values have to be a range with a max and a min, so we can't just exclude 0 if we want to allow -1 (as far as I know). I think it would be nice to be able to specify multiple valid ranges for GUCs to the GUC machinery. So, to answer your question, yes, the code and docs are a bit out-of-sync. > I need another few readthroughs to figure out of VacuumFailsafeActive does what > I think it does, and should be doing, but in general I think this is a good > idea and a patch in good condition close to being committable. I will take a pass at splitting up the main commit into two. However, I have attached a new version with the other specific updates discussed in this thread. Feel free to provide review on this version in the meantime. - Melanie Attachments: [text/x-patch] v7-0001-Make-failsafe_active-global.patch (5.3K, ../../CAAKRu_YraQ1mtqRJSqv+rvp0Z3VSWhy19gEkAoYqRkmOFRbYXw@mail.gmail.com/2-v7-0001-Make-failsafe_active-global.patch) download | inline diff: From c48b9cc5da87d980fff7a0131da72c28865ef310 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Thu, 23 Mar 2023 18:21:24 -0400 Subject: [PATCH v7 1/2] Make failsafe_active global In preparation for future work to update the cost-based delay parameters more frequently during vacuum, move the failsafe_active status into a global variable which can be accessed from all parts of vacuum code. It will be used in combination with VacuumCostDelay to keep VacuumCostActive up-to-date during failsafe vacuuming.. --- src/backend/access/heap/vacuumlazy.c | 16 +++++++--------- src/backend/commands/vacuum.c | 4 ++++ src/backend/commands/vacuumparallel.c | 1 + src/include/commands/vacuum.h | 3 +++ 4 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 8f14cf85f3..f4755bcc4b 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; /* Disable index vacuuming, index cleanup, and heap rel truncation */ vacrel->do_index_vacuuming = false; @@ -2811,7 +2809,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 c54360a6a0..2d5ea570a2 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -77,6 +77,7 @@ int vacuum_multixact_failsafe_age; static MemoryContext vac_context = NULL; static BufferAccessStrategy vac_strategy; +bool VacuumFailsafeActive = false; /* * Variables for cost-based parallel vacuum. See comments atop @@ -492,6 +493,7 @@ vacuum(List *relations, VacuumParams *params, in_vacuum = true; VacuumCostActive = (VacuumCostDelay > 0); + VacuumFailsafeActive = false; VacuumCostBalance = 0; VacuumPageHit = 0; VacuumPageMiss = 0; @@ -1850,6 +1852,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params, bool skip_privs) Assert(params != NULL); + VacuumFailsafeActive = false; + /* Begin a transaction for vacuuming this relation */ StartTransactionCommand(); diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c index bcd40c80a1..57188500d0 100644 --- a/src/backend/commands/vacuumparallel.c +++ b/src/backend/commands/vacuumparallel.c @@ -990,6 +990,7 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc) false); /* Set cost-based vacuum delay */ + VacuumFailsafeActive = false; VacuumCostActive = (VacuumCostDelay > 0); VacuumCostBalance = 0; VacuumPageHit = 0; diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h index bdfd96cfec..a1bf3bfaa5 100644 --- a/src/include/commands/vacuum.h +++ b/src/include/commands/vacuum.h @@ -301,6 +301,9 @@ extern PGDLLIMPORT int vacuum_multixact_freeze_table_age; extern PGDLLIMPORT int vacuum_failsafe_age; extern PGDLLIMPORT int vacuum_multixact_failsafe_age; +/* Indicates if wraparound failsafe has been triggered */ +extern bool VacuumFailsafeActive; + /* Variables for cost-based parallel vacuum */ extern PGDLLIMPORT pg_atomic_uint32 *VacuumSharedCostBalance; extern PGDLLIMPORT pg_atomic_uint32 *VacuumActiveNWorkers; -- 2.37.2 [text/x-patch] v7-0002-auto-vacuum-reloads-config-file-more-often.patch (19.9K, ../../CAAKRu_YraQ1mtqRJSqv+rvp0Z3VSWhy19gEkAoYqRkmOFRbYXw@mail.gmail.com/3-v7-0002-auto-vacuum-reloads-config-file-more-often.patch) download | inline diff: From 37b9db4404d51c7a4e695472f9ea9f96392f9dd1 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Thu, 23 Mar 2023 18:41:10 -0400 Subject: [PATCH v7 2/2] [auto]vacuum reloads config file more often Previously, VACUUM and autovacuum workers would reload the configuration file only between vacuuming tables. This precluded user updates to cost-based delay parameters from taking effect while vacuuming a table. Check if a reload is pending roughly once per block now, when checking if we need to delay. In order for this change to have the intended effect on autovacuum, autovacuum workers must start updating their cost delay more frequently as well. With this new paradigm, balancing the cost limit amongst workers also must work differently. Previously, a 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 table options could still have different values for their wi_cost_limit_base and wi_cost_delay. With this change, workers will (within some margin of error) have no reason to have different values for cost limit and cost delay (in the absence of table options). Thus, remove cost limit and cost delay from shared memory and keep track only of the number of workers actively vacuuming tables with no cost-related table options. Then, use this value to determine what each worker's effective cost limit should be. Reviewed-by: Masahiko Sawada <[email protected]> Reviewed-by: Daniel Gustafsson <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_buP5wzsho3qNw5o9_R0pF69FRM5hgCmr-mvXmGXwdA7A%40mail.gmail.com#5e6771d4cdca4db6efc2acec2dce0bc7 --- src/backend/commands/vacuum.c | 52 +++++-- src/backend/postmaster/autovacuum.c | 218 ++++++++++++---------------- src/include/postmaster/autovacuum.h | 2 + 3 files changed, 140 insertions(+), 132 deletions(-) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 2d5ea570a2..c4c361ae94 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" @@ -76,6 +77,7 @@ int vacuum_multixact_failsafe_age; /* A few variables that don't seem worth passing around as parameters */ static MemoryContext vac_context = NULL; static BufferAccessStrategy vac_strategy; +static bool analyze_in_outer_xact = false; bool VacuumFailsafeActive = false; @@ -315,8 +317,7 @@ vacuum(List *relations, VacuumParams *params, static bool in_vacuum = false; const char *stmttype; - volatile bool in_outer_xact, - use_own_xacts; + volatile bool use_own_xacts; Assert(params != NULL); @@ -333,10 +334,10 @@ vacuum(List *relations, VacuumParams *params, if (params->options & VACOPT_VACUUM) { PreventInTransactionBlock(isTopLevel, stmttype); - in_outer_xact = false; + analyze_in_outer_xact = false; } else - in_outer_xact = IsInTransactionBlock(isTopLevel); + analyze_in_outer_xact = IsInTransactionBlock(isTopLevel); /* * Due to static variables vac_context, anl_context and vac_strategy, @@ -458,7 +459,7 @@ vacuum(List *relations, VacuumParams *params, Assert(params->options & VACOPT_ANALYZE); if (IsAutoVacuumWorkerProcess()) use_own_xacts = true; - else if (in_outer_xact) + else if (analyze_in_outer_xact) use_own_xacts = false; else if (list_length(relations) > 1) use_own_xacts = true; @@ -476,7 +477,7 @@ vacuum(List *relations, VacuumParams *params, */ if (use_own_xacts) { - Assert(!in_outer_xact); + Assert(!analyze_in_outer_xact); /* ActiveSnapshot is not set by autovacuum */ if (ActiveSnapshotSet()) @@ -529,7 +530,7 @@ vacuum(List *relations, VacuumParams *params, } analyze_rel(vrel->oid, vrel->relation, params, - vrel->va_cols, in_outer_xact, vac_strategy); + vrel->va_cols, analyze_in_outer_xact, vac_strategy); if (use_own_xacts) { @@ -552,6 +553,7 @@ vacuum(List *relations, VacuumParams *params, { in_vacuum = false; VacuumCostActive = false; + analyze_in_outer_xact = false; } PG_END_TRY(); @@ -2219,9 +2221,33 @@ 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 && !analyze_in_outer_xact) + { + ConfigReloadPending = false; + ProcessConfigFile(PGC_SIGHUP); + AutoVacuumUpdateDelay(); + AutoVacuumUpdateLimit(); + } + + /* + * If we disabled cost-based delays after reloading the config file, + * return + */ + if (!VacuumCostActive) + { + VacuumCostBalance = 0; + return; + } + /* * For parallel vacuum, the delay is computed based on the shared cost * balance. See compute_parallel_delay. @@ -2252,8 +2278,14 @@ vacuum_delay_point(void) VacuumCostBalance = 0; - /* update balance values for workers */ - AutoVacuumUpdateDelay(); + /* + * Update limit values for 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. + */ + AutoVacuumUpdateLimit(); /* Might have gotten an interrupt while sleeping */ CHECK_FOR_INTERRUPTS(); diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index c0e2e00a7e..ad872d8c73 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -139,6 +139,9 @@ int Log_autovacuum_min_duration = 600000; static bool am_autovacuum_launcher = false; static bool am_autovacuum_worker = false; +static double av_table_option_cost_delay = -1; +static int av_table_option_cost_limit = 0; + /* Flags set by signal handlers */ static volatile sig_atomic_t got_SIGUSR2 = false; @@ -189,8 +192,8 @@ typedef struct autovac_table { Oid at_relid; VacuumParams at_params; - double at_vacuum_cost_delay; - int at_vacuum_cost_limit; + double at_table_option_vac_cost_delay; + int at_table_option_vac_cost_limit; bool at_dobalance; bool at_sharedrel; char *at_relname; @@ -209,7 +212,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 @@ -225,9 +228,6 @@ typedef struct WorkerInfoData TimestampTz wi_launchtime; bool 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 +273,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_nworkers_for_balance 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 +288,7 @@ typedef struct dlist_head av_runningWorkers; WorkerInfo av_startingWorker; AutoVacuumWorkItem av_workItems[NUM_WORKITEMS]; + pg_atomic_uint32 av_nworkers_for_balance; } AutoVacuumShmemStruct; static AutoVacuumShmemStruct *AutoVacuumShmem; @@ -820,7 +823,7 @@ HandleAutoVacLauncherInterrupts(void) AutoVacLauncherShutdown(); /* rebalance in case the default cost parameters changed */ - LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); + LWLockAcquire(AutovacuumLock, LW_SHARED); autovac_balance_cost(); LWLockRelease(AutovacuumLock); @@ -1756,9 +1759,6 @@ FreeWorkerInfo(int code, Datum arg) 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; dlist_push_head(&AutoVacuumShmem->av_freeWorkers, &MyWorkerInfo->wi_links); /* not mine anymore */ @@ -1774,99 +1774,97 @@ 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 VacuumCostDelay with the correct value for an autovacuum worker, + * given the value of other relevant cost-based delay parameters. Autovacuum + * workers should call this after every config reload, in case VacuumCostDelay + * was overwritten. */ void AutoVacuumUpdateDelay(void) { - if (MyWorkerInfo) + if (!am_autovacuum_worker) + return; + + if (av_table_option_cost_delay >= 0) + VacuumCostDelay = av_table_option_cost_delay; + else if (autovacuum_vac_cost_delay >= 0) + VacuumCostDelay = autovacuum_vac_cost_delay; + + if (!VacuumFailsafeActive) + VacuumCostActive = (VacuumCostDelay > 0); +} + +/* + * 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 also must call this after + * every config reload, in case VacuumCostLimit was overwritten. + */ +void +AutoVacuumUpdateLimit(void) +{ + if (!am_autovacuum_worker) + return; + + /* + * note: in cost_limit, zero also means use value from elsewhere, because + * zero is not a valid value. + */ + if (av_table_option_cost_limit > 0) + VacuumCostLimit = av_table_option_cost_limit; + else { - VacuumCostDelay = MyWorkerInfo->wi_cost_delay; - VacuumCostLimit = MyWorkerInfo->wi_cost_limit; + int vac_cost_limit = autovacuum_vac_cost_limit > 0 ? + autovacuum_vac_cost_limit : VacuumCostLimit; + + int balanced_cost_limit = vac_cost_limit / + pg_atomic_read_u32(&AutoVacuumShmem->av_nworkers_for_balance); + + VacuumCostLimit = Max(Min(balanced_cost_limit, vac_cost_limit), 1); } } /* * autovac_balance_cost - * Recalculate the cost limit setting for each active worker. + * Recalculate the number of workers to consider, given table options and + * the current number of active workers. * - * Caller must hold the AutovacuumLock in exclusive mode. + * Caller must hold the AutovacuumLock in at least shared mode. */ static void autovac_balance_cost(void) { - /* - * 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; + int orig_nworkers_for_balance; + int nworkers_for_balance = 0; - /* not set? nothing to do */ - if (vac_cost_limit <= 0 || vac_cost_delay <= 0) + if (autovacuum_vac_cost_delay == 0 || + (autovacuum_vac_cost_delay == -1 && VacuumCostDelay == 0)) return; - /* calculate the total base cost limit of participating active workers */ - cost_total = 0.0; - 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) - cost_total += - (double) worker->wi_cost_limit_base / worker->wi_cost_delay; - } - - /* there are no cost limits -- nothing to do */ - if (cost_total <= 0) + if (autovacuum_vac_cost_limit <= 0 && VacuumCostLimit <= 0) return; - /* - * 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; + orig_nworkers_for_balance = + pg_atomic_read_u32(&AutoVacuumShmem->av_nworkers_for_balance); + 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 || !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++; } + + nworkers_for_balance = Max(nworkers_for_balance, 1); + + if (nworkers_for_balance != orig_nworkers_for_balance) + pg_atomic_write_u32(&AutoVacuumShmem->av_nworkers_for_balance, + nworkers_for_balance); } /* @@ -2312,14 +2310,15 @@ do_autovacuum(void) autovac_table *tab; bool isshared; bool skipit; - double stdVacuumCostDelay; - int stdVacuumCostLimit; dlist_iter iter; CHECK_FOR_INTERRUPTS(); /* * Check for config changes before processing each collected table. + * Autovacuum workers must update VacuumCostDelay and VacuumCostLimit + * in case they were overridden by the reload. However, we will do + * this as soon as we check table options a bit later. */ if (ConfigReloadPending) { @@ -2416,32 +2415,18 @@ 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; + av_table_option_cost_limit = tab->at_table_option_vac_cost_limit; + av_table_option_cost_delay = tab->at_table_option_vac_cost_delay; /* Must hold AutovacuumLock while mucking with cost balance info */ LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); - - /* 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; - - /* do a balance */ autovac_balance_cost(); + LWLockRelease(AutovacuumLock); - /* set the active cost parameters from the result of that */ + AutoVacuumUpdateLimit(); AutoVacuumUpdateDelay(); - /* done */ - LWLockRelease(AutovacuumLock); - /* clean up memory before each iteration */ MemoryContextResetAndDeleteChildren(PortalContext); @@ -2525,19 +2510,15 @@ deleted: /* * 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. + * don't, set wi_dobalance to false on the assumption that we are more + * likely than not to vacuum a table with no table options 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); - - /* restore vacuum cost GUCs for the next iteration */ - VacuumCostDelay = stdVacuumCostDelay; - VacuumCostLimit = stdVacuumCostLimit; } /* @@ -2569,6 +2550,8 @@ deleted: { ConfigReloadPending = false; ProcessConfigFile(PGC_SIGHUP); + AutoVacuumUpdateDelay(); + AutoVacuumUpdateLimit(); } LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); @@ -2801,8 +2784,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; /* @@ -2812,20 +2793,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 @@ -2881,8 +2848,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_table_option_vac_cost_limit = avopts ? + avopts->vacuum_cost_limit : 0; + tab->at_table_option_vac_cost_delay = avopts ? + avopts->vacuum_cost_delay : -1; tab->at_relname = NULL; tab->at_nspname = NULL; tab->at_datname = NULL; @@ -3374,10 +3343,15 @@ 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); + + /* initialize to 1, as it should be a minimum of 1 */ + pg_atomic_init_u32(&AutoVacuumShmem->av_nworkers_for_balance, 1); + } else Assert(found); diff --git a/src/include/postmaster/autovacuum.h b/src/include/postmaster/autovacuum.h index c140371b51..558358911c 100644 --- a/src/include/postmaster/autovacuum.h +++ b/src/include/postmaster/autovacuum.h @@ -66,6 +66,8 @@ extern void AutoVacWorkerFailed(void); /* autovacuum cost-delay balancer */ extern void AutoVacuumUpdateDelay(void); +extern void AutoVacuumUpdateLimit(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] 11+ messages in thread
* Re: Should vacuum process config file reload more often 2023-03-15 05:13 Re: Should vacuum process config file reload more often Masahiko Sawada <[email protected]> 2023-03-18 22:47 ` Re: Should vacuum process config file reload more often Melanie Plageman <[email protected]> 2023-03-23 06:08 ` Re: Should vacuum process config file reload more often Masahiko Sawada <[email protected]> 2023-03-23 16:24 ` Re: Should vacuum process config file reload more often Daniel Gustafsson <[email protected]> 2023-03-24 00:27 ` Re: Should vacuum process config file reload more often Melanie Plageman <[email protected]> @ 2023-03-24 05:21 ` Masahiko Sawada <[email protected]> 2023-03-24 17:27 ` Re: Should vacuum process config file reload more often Melanie Plageman <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Masahiko Sawada @ 2023-03-24 05:21 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; pgsql-hackers; Andres Freund <[email protected]>; Amit Kapila <[email protected]> On Fri, Mar 24, 2023 at 9:27 AM Melanie Plageman <[email protected]> wrote: > > On Thu, Mar 23, 2023 at 2:09 AM Masahiko Sawada <[email protected]> wrote: > > On Sun, Mar 19, 2023 at 7:47 AM Melanie Plageman <[email protected]> wrote: > > Do we need to calculate the number of workers running with > > nworkers_for_balance by iterating over the running worker list? I > > guess autovacuum workers can increment/decrement it at the beginning > > and end of vacuum. > > I don't think we can do that because if a worker crashes, we have no way > of knowing if it had incremented or decremented the number, so we can't > adjust for it. What kind of crash are you concerned about? If a worker raises an ERROR, we can catch it in PG_CATCH() block. If it's a FATAL, we can do that in FreeWorkerInfo(). A PANIC error ends up crashing the entire server. > > > > > > > > Also not sure how the patch interacts with failsafe autovac and parallel > > > > > > > vacuum. > > > > > > > > > > > > Good point. > > > > > > > > > > > > When entering the failsafe mode, we disable the vacuum delays (see > > > > > > lazy_check_wraparound_failsafe()). We need to keep disabling the > > > > > > vacuum delays even after reloading the config file. One idea is to > > > > > > have another global variable indicating we're in the failsafe mode. > > > > > > vacuum_delay_point() doesn't update VacuumCostActive if the flag is > > > > > > true. > > > > > > > > > > I think we might not need to do this. Other than in > > > > > lazy_check_wraparound_failsafe(), VacuumCostActive is only updated in > > > > > two places: > > > > > > > > > > 1) in vacuum() which autovacuum will call per table. And failsafe is > > > > > reset per table as well. > > > > > > > > > > 2) in vacuum_delay_point(), but, since VacuumCostActive will already be > > > > > false when we enter vacuum_delay_point() the next time after > > > > > lazy_check_wraparound_failsafe(), we won't set VacuumCostActive there. > > > > > > > > Indeed. But does it mean that there is no code path to turn > > > > vacuum-delay on, even when vacuum_cost_delay is updated from 0 to > > > > non-0? > > > > > > Ah yes! Good point. This is true. > > > I'm not sure how to cheaply allow for re-enabling delays after disabling > > > them in the middle of a table vacuum. > > > > > > I don't see a way around checking if we need to reload the config file > > > on every call to vacuum_delay_point() (currently, we are only doing this > > > when we have to wait anyway). It seems expensive to do this check every > > > time. If we do do this, we would update VacuumCostActive when updating > > > VacuumCostDelay, and we would need a global variable keeping the > > > failsafe status, as you mentioned. > > > > > > It could be okay to say that you can only disable cost-based delays in > > > the middle of vacuuming a table (i.e. you cannot enable them if they are > > > already disabled until you start vacuuming the next table). Though maybe > > > it is weird that you can increase the delay but not re-enable it... > > > > On Mon, Mar 20, 2023 at 1:48 AM Melanie Plageman > > <[email protected]> wrote: > > > So, I thought about it some more, and I think it is a bit odd that you > > > can increase the delay and limit but not re-enable them if they were > > > disabled. And, perhaps it would be okay to check ConfigReloadPending at > > > the top of vacuum_delay_point() instead of only after sleeping. It is > > > just one more branch. We can check if VacuumCostActive is false after > > > checking if we should reload and doing so if needed and return early. > > > I've implemented that in attached v6. > > > > > > I added in the global we discussed for VacuumFailsafeActive. If we keep > > > it, we can probably remove the one in LVRelState -- as it seems > > > redundant. Let me know what you think. > > > > I think the following change is related: > > > > - if (!VacuumCostActive || InterruptPending) > > + if (InterruptPending || VacuumFailsafeActive || > > + (!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 && !analyze_in_outer_xact) > > + { > > + ConfigReloadPending = false; > > + ProcessConfigFile(PGC_SIGHUP); > > + AutoVacuumUpdateDelay(); > > + AutoVacuumUpdateLimit(); > > + } > > > > It makes sense to me that we need to reload the config file even when > > vacuum-delay is disabled. But I think it's not convenient for users > > that we don't reload the configuration file once the failsafe is > > triggered. I think users might want to change some GUCs such as > > log_autovacuum_min_duration. > > Ah, okay. Attached v7 has this change (it reloads even if failsafe is > active). > > > > And, I was wondering if it was worth trying to split up the part that > > > reloads the config file and all of the autovacuum stuff. The reloading > > > of the config file by itself won't actually result in autovacuum workers > > > having updated cost delays because of them overwriting it with > > > wi_cost_delay, but it will allow VACUUM to have those updated values. > > > > It makes sense to me to have changes for overhauling the rebalance > > mechanism in a separate patch. > > > > Looking back at the original concern you mentioned[1]: > > > > speed up long-running vacuum of a large table by > > decreasing autovacuum_vacuum_cost_delay/vacuum_cost_delay, however the > > config file is only reloaded between tables (for autovacuum) or after > > the statement (for explicit vacuum). > > > > does it make sense to have autovac_balance_cost() update workers' > > wi_cost_delay too? Autovacuum launcher already reloads the config file > > and does the rebalance. So I thought autovac_balance_cost() can update > > the cost_delay as well, and this might be a minimal change to deal > > with your concern. This doesn't have the effect for manual VACUUM but > > since vacuum delay is disabled by default it won't be a big problem. > > As for manual VACUUMs, we would need to reload the config file in > > vacuum_delay_point() as the part of your patch does. Overhauling the > > rebalance mechanism would be another patch to improve it further. > > So, we can't do this without acquiring an access shared lock on every > call to vacuum_delay_point() because cost delay is a double. > > I will work on a patchset with separate commits for reloading the config > file, though (with autovac not benefitting in the first commit). > > On Thu, Mar 23, 2023 at 12:24 PM Daniel Gustafsson <[email protected]> wrote: > > > > +bool VacuumFailsafeActive = false; > > This needs documentation, how it's used and how it relates to failsafe_active > > in LVRelState (which it might replace(?), but until then). > > Thanks! I've removed LVRelState->failsafe_active. > > I've also separated the VacuumFailsafeActive change into its own commit. @@ -492,6 +493,7 @@ vacuum(List *relations, VacuumParams *params, in_vacuum = true; VacuumCostActive = (VacuumCostDelay > 0); + VacuumFailsafeActive = false; VacuumCostBalance = 0; VacuumPageHit = 0; VacuumPageMiss = 0; I think we need to reset VacuumFailsafeActive also in PG_FINALLY() block in vacuum(). One comment on 0002 patch: + /* + * 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 && !analyze_in_outer_xact) + { + ConfigReloadPending = false; + ProcessConfigFile(PGC_SIGHUP); + AutoVacuumUpdateDelay(); + AutoVacuumUpdateLimit(); + } I think we need comments on why we don't reload the config file if we're analyzing a table in a user transaction. > > > I need another few readthroughs to figure out of VacuumFailsafeActive does what > > I think it does, and should be doing, but in general I think this is a good > > idea and a patch in good condition close to being committable. Another approach would be to make VacuumCostActive a ternary value: on, off, and never. When we trigger the failsafe mode we switch it to never, meaning that it never becomes active even after reloading the config file. A good point is that we don't need to add a new global variable, but I'm not sure it's better than the current approach. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Should vacuum process config file reload more often 2023-03-15 05:13 Re: Should vacuum process config file reload more often Masahiko Sawada <[email protected]> 2023-03-18 22:47 ` Re: Should vacuum process config file reload more often Melanie Plageman <[email protected]> 2023-03-23 06:08 ` Re: Should vacuum process config file reload more often Masahiko Sawada <[email protected]> 2023-03-23 16:24 ` Re: Should vacuum process config file reload more often Daniel Gustafsson <[email protected]> 2023-03-24 00:27 ` Re: Should vacuum process config file reload more often Melanie Plageman <[email protected]> 2023-03-24 05:21 ` Re: Should vacuum process config file reload more often Masahiko Sawada <[email protected]> @ 2023-03-24 17:27 ` Melanie Plageman <[email protected]> 2023-03-25 19:03 ` Re: Should vacuum process config file reload more often Melanie Plageman <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Melanie Plageman @ 2023-03-24 17:27 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; pgsql-hackers; Andres Freund <[email protected]>; Amit Kapila <[email protected]> On Fri, Mar 24, 2023 at 1:21 AM Masahiko Sawada <[email protected]> wrote: > > On Fri, Mar 24, 2023 at 9:27 AM Melanie Plageman > <[email protected]> wrote: > > > > On Thu, Mar 23, 2023 at 2:09 AM Masahiko Sawada <[email protected]> wrote: > > > On Sun, Mar 19, 2023 at 7:47 AM Melanie Plageman <[email protected]> wrote: > > > Do we need to calculate the number of workers running with > > > nworkers_for_balance by iterating over the running worker list? I > > > guess autovacuum workers can increment/decrement it at the beginning > > > and end of vacuum. > > > > I don't think we can do that because if a worker crashes, we have no way > > of knowing if it had incremented or decremented the number, so we can't > > adjust for it. > > What kind of crash are you concerned about? If a worker raises an > ERROR, we can catch it in PG_CATCH() block. If it's a FATAL, we can do > that in FreeWorkerInfo(). A PANIC error ends up crashing the entire > server. Yes, but what about a worker that segfaults? Since table AMs can define relation_vacuum(), this seems like a real possibility. I'll address your other code feedback in the next version. I realized nworkers_for_balance should be initialized to 0 and not 1 -- 1 is misleading since there are often 0 autovac workers. We just never want to use nworkers_for_balance when it is 0. But, workers put a floor of 1 on the number when they divide limit/nworkers_for_balance (since they know there must be at least one worker right now since they are a worker). I thought about whether or not they should call autovac_balance_cost() if they find that nworkers_for_balance is 0 when updating their own limit, but I'm not sure. > > > I need another few readthroughs to figure out of VacuumFailsafeActive does what > > > I think it does, and should be doing, but in general I think this is a good > > > idea and a patch in good condition close to being committable. > > Another approach would be to make VacuumCostActive a ternary value: > on, off, and never. When we trigger the failsafe mode we switch it to > never, meaning that it never becomes active even after reloading the > config file. A good point is that we don't need to add a new global > variable, but I'm not sure it's better than the current approach. Hmm, this is interesting. I don't love the word "never" since it kind of implies a duration longer than the current table being vacuumed. But we could find a different word or just document it well. For clarity, we might want to call it failsafe_mode or something. I wonder if the primary drawback to converting LVRelState->failsafe_active to a global VacuumFailsafeActive is just the general rule of limiting scope to the minimum needed. - Melanie ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Should vacuum process config file reload more often 2023-03-15 05:13 Re: Should vacuum process config file reload more often Masahiko Sawada <[email protected]> 2023-03-18 22:47 ` Re: Should vacuum process config file reload more often Melanie Plageman <[email protected]> 2023-03-23 06:08 ` Re: Should vacuum process config file reload more often Masahiko Sawada <[email protected]> 2023-03-23 16:24 ` Re: Should vacuum process config file reload more often Daniel Gustafsson <[email protected]> 2023-03-24 00:27 ` Re: Should vacuum process config file reload more often Melanie Plageman <[email protected]> 2023-03-24 05:21 ` Re: Should vacuum process config file reload more often Masahiko Sawada <[email protected]> 2023-03-24 17:27 ` Re: Should vacuum process config file reload more often Melanie Plageman <[email protected]> @ 2023-03-25 19:03 ` Melanie Plageman <[email protected]> 2023-03-27 18:12 ` Re: Should vacuum process config file reload more often Melanie Plageman <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Melanie Plageman @ 2023-03-25 19:03 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; pgsql-hackers; Andres Freund <[email protected]>; Amit Kapila <[email protected]> On Thu, Mar 23, 2023 at 8:27 PM Melanie Plageman <[email protected]> wrote: > On Thu, Mar 23, 2023 at 2:09 AM Masahiko Sawada <[email protected]> wrote: > > > And, I was wondering if it was worth trying to split up the part that > > > reloads the config file and all of the autovacuum stuff. The reloading > > > of the config file by itself won't actually result in autovacuum workers > > > having updated cost delays because of them overwriting it with > > > wi_cost_delay, but it will allow VACUUM to have those updated values. > > > > It makes sense to me to have changes for overhauling the rebalance > > mechanism in a separate patch. > > > > Looking back at the original concern you mentioned[1]: > > > > speed up long-running vacuum of a large table by > > decreasing autovacuum_vacuum_cost_delay/vacuum_cost_delay, however the > > config file is only reloaded between tables (for autovacuum) or after > > the statement (for explicit vacuum). > > > > does it make sense to have autovac_balance_cost() update workers' > > wi_cost_delay too? Autovacuum launcher already reloads the config file > > and does the rebalance. So I thought autovac_balance_cost() can update > > the cost_delay as well, and this might be a minimal change to deal > > with your concern. This doesn't have the effect for manual VACUUM but > > since vacuum delay is disabled by default it won't be a big problem. > > As for manual VACUUMs, we would need to reload the config file in > > vacuum_delay_point() as the part of your patch does. Overhauling the > > rebalance mechanism would be another patch to improve it further. > > So, we can't do this without acquiring an access shared lock on every > call to vacuum_delay_point() because cost delay is a double. > > I will work on a patchset with separate commits for reloading the config > file, though (with autovac not benefitting in the first commit). So, I realized we could actually do as you say and have autovac workers update their wi_cost_delay and keep the balance changes in a separate commit. I've done this in attached v8. Workers take the exclusive lock to update their wi_cost_delay and wi_cost_limit only when there is a config reload. So, there is one commit that implements this behavior and a separate commit to revise the worker rebalancing. Note that we must have the workers also update wi_cost_limit_base and then call autovac_balance_cost() when they reload the config file (instead of waiting for launcher to call autovac_balance_cost()) to avoid potentially calculating the sleep with a new value of cost delay and an old value of cost limit. In the commit which revises the worker rebalancing, I'm still wondering if wi_dobalance should be an atomic flag -- probably not worth it, right? On Fri, Mar 24, 2023 at 1:27 PM Melanie Plageman <[email protected]> wrote: > I realized nworkers_for_balance should be initialized to 0 and not 1 -- > 1 is misleading since there are often 0 autovac workers. We just never > want to use nworkers_for_balance when it is 0. But, workers put a floor > of 1 on the number when they divide limit/nworkers_for_balance (since > they know there must be at least one worker right now since they are a > worker). I thought about whether or not they should call > autovac_balance_cost() if they find that nworkers_for_balance is 0 when > updating their own limit, but I'm not sure. I've gone ahead and updated this. I haven't made the workers call autovac_balance_cost() if they find that nworkers_for_balance is 0 when they try and use it when updating their limit because I'm not sure if this can happen. I would be interested in input here. I'm also still interested in feedback on the variable name av_nworkers_for_balance. > On Fri, Mar 24, 2023 at 1:21 AM Masahiko Sawada <[email protected]> wrote: > > > > I need another few readthroughs to figure out of VacuumFailsafeActive does what > > > > I think it does, and should be doing, but in general I think this is a good > > > > idea and a patch in good condition close to being committable. > > > > Another approach would be to make VacuumCostActive a ternary value: > > on, off, and never. When we trigger the failsafe mode we switch it to > > never, meaning that it never becomes active even after reloading the > > config file. A good point is that we don't need to add a new global > > variable, but I'm not sure it's better than the current approach. > > Hmm, this is interesting. I don't love the word "never" since it kind of > implies a duration longer than the current table being vacuumed. But we > could find a different word or just document it well. For clarity, we > might want to call it failsafe_mode or something. > > I wonder if the primary drawback to converting > LVRelState->failsafe_active to a global VacuumFailsafeActive is just the > general rule of limiting scope to the minimum needed. Okay, so I've changed my mind about this. I like having a ternary for VacuumCostActive and keeping failsafe_active in LVRelState. What I didn't like was having non-vacuum code have to care about the distinction between failsafe + inactive and just inactive. To handle this, I converted VacuumCostActive to VacuumCostInactive since there are two inactive cases (inactive and failsafe and plain inactive) and only one active case. Then, I defined VacuumCostInactive as an int but use enum values for it in vacuum code to distinguish between failsafe + inactive and just inactive (I call it VACUUM_COST_INACTIVE_AND_LOCKED and VACUUM_COST_INACTIVE_AND_UNLOCKED). Non-vacuum code only needs to check if VacuumCostInactive is 0 like if (!VacuumCostInactive). I'm happy with the result, and I think it employs only well-defined C behavior. - Melanie Attachments: [text/x-patch] v8-0003-auto-vacuum-reloads-config-file-more-often.patch (7.7K, ../../CAAKRu_a-y1eTNLJA3U-SwcjU4SWZ5w1QSXAN6uee3wivcjR6BQ@mail.gmail.com/2-v8-0003-auto-vacuum-reloads-config-file-more-often.patch) download | inline diff: From 8b9fcf7c10353dcacb4ac16515aad0ce34565566 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Fri, 24 Mar 2023 13:38:20 -0400 Subject: [PATCH v8 3/4] [auto]vacuum reloads config file more often Previously, VACUUM and autovacuum workers would reload the configuration file only between vacuuming tables. This precluded user updates to cost-based delay parameters from taking effect while vacuuming a table. Check if a reload is pending roughly once per block now, when checking if we need to delay. Reviewed-by: Masahiko Sawada <[email protected]> Reviewed-by: Daniel Gustafsson <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_buP5wzsho3qNw5o9_R0pF69FRM5hgCmr-mvXmGXwdA7A%40mail.gmail.com#5e6771d4cdca4db6efc2acec2dce0bc7 --- src/backend/commands/vacuum.c | 72 +++++++++++++++++++++++++---- src/backend/postmaster/autovacuum.c | 30 +++++++++++- src/include/postmaster/autovacuum.h | 3 +- 3 files changed, 92 insertions(+), 13 deletions(-) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index eb126f2247..cb32078c19 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" @@ -76,6 +77,7 @@ int vacuum_multixact_failsafe_age; /* A few variables that don't seem worth passing around as parameters */ static MemoryContext vac_context = NULL; static BufferAccessStrategy vac_strategy; +static bool analyze_in_outer_xact = false; /* @@ -314,8 +316,7 @@ vacuum(List *relations, VacuumParams *params, static bool in_vacuum = false; const char *stmttype; - volatile bool in_outer_xact, - use_own_xacts; + volatile bool use_own_xacts; Assert(params != NULL); @@ -332,10 +333,10 @@ vacuum(List *relations, VacuumParams *params, if (params->options & VACOPT_VACUUM) { PreventInTransactionBlock(isTopLevel, stmttype); - in_outer_xact = false; + analyze_in_outer_xact = false; } else - in_outer_xact = IsInTransactionBlock(isTopLevel); + analyze_in_outer_xact = IsInTransactionBlock(isTopLevel); /* * Due to static variables vac_context, anl_context and vac_strategy, @@ -457,7 +458,7 @@ vacuum(List *relations, VacuumParams *params, Assert(params->options & VACOPT_ANALYZE); if (IsAutoVacuumWorkerProcess()) use_own_xacts = true; - else if (in_outer_xact) + else if (analyze_in_outer_xact) use_own_xacts = false; else if (list_length(relations) > 1) use_own_xacts = true; @@ -475,7 +476,7 @@ vacuum(List *relations, VacuumParams *params, */ if (use_own_xacts) { - Assert(!in_outer_xact); + Assert(!analyze_in_outer_xact); /* ActiveSnapshot is not set by autovacuum */ if (ActiveSnapshotSet()) @@ -544,7 +545,7 @@ vacuum(List *relations, VacuumParams *params, } analyze_rel(vrel->oid, vrel->relation, params, - vrel->va_cols, in_outer_xact, vac_strategy); + vrel->va_cols, analyze_in_outer_xact, vac_strategy); if (use_own_xacts) { @@ -568,6 +569,7 @@ vacuum(List *relations, VacuumParams *params, in_vacuum = false; VacuumCostInactive = VACUUM_COST_INACTIVE_AND_UNLOCKED; VacuumCostBalance = 0; + analyze_in_outer_xact = false; } PG_END_TRY(); @@ -2233,7 +2235,52 @@ vacuum_delay_point(void) /* Always check for interrupts */ CHECK_FOR_INTERRUPTS(); - if (VacuumCostInactive || InterruptPending) + if (InterruptPending || + (VacuumCostInactive && !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. Analyze should + * not reload configuration file if it is in an outer transaction, as GUC + * values shouldn't be allowed to refer to some uncommitted state (e.g. + * database objects created in this transaction). + */ + if (ConfigReloadPending && !analyze_in_outer_xact) + { + ConfigReloadPending = false; + ProcessConfigFile(PGC_SIGHUP); + + /* + * Autovacuum workers must restore the correct values of + * VacuumCostLimit and VacuumCostDelay in case they were overwritten + * by reload. + */ + AutoVacuumUpdateCosts(); + AutoVacuumOverrideCosts(); + + /* + * If configuration changes are allowed to impact VacuumCostInactive, + * make sure it is updated. + */ + if (VacuumCostInactive == VACUUM_COST_INACTIVE_AND_LOCKED) + return; + + if (VacuumCostDelay > 0) + VacuumCostInactive = VACUUM_COST_ACTIVE; + else + { + VacuumCostInactive = VACUUM_COST_INACTIVE_AND_UNLOCKED; + VacuumCostBalance = 0; + } + } + + /* + * If we disabled cost-based delays after reloading the config file, + * return. + */ + if (VacuumCostInactive) return; /* @@ -2266,8 +2313,13 @@ vacuum_delay_point(void) VacuumCostBalance = 0; - /* update balance values for workers */ - AutoVacuumUpdateDelay(); + /* + * For autovacuum workers, someone may have called + * autovac_balance_cost() since they last updated their + * VacuumCostLimit above. Do so again now to ensure they have a + * current value. + */ + AutoVacuumOverrideCosts(); /* Might have gotten an interrupt while sleeping */ CHECK_FOR_INTERRUPTS(); diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index c0e2e00a7e..8ac14a44c8 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -1778,7 +1778,7 @@ FreeWorkerInfo(int code, Datum arg) * each a fraction of the total available I/O. */ void -AutoVacuumUpdateDelay(void) +AutoVacuumOverrideCosts(void) { if (MyWorkerInfo) { @@ -1787,6 +1787,29 @@ AutoVacuumUpdateDelay(void) } } +/* + * Caller must not already hold the AutovacuumLock + */ +void +AutoVacuumUpdateCosts(void) +{ + /* + * Even though this autovacuum worker may be vacuuming a table with a cost + * limit table option and not a cost delay table option, we still don't + * refresh the cost delay value. + */ + if (!MyWorkerInfo || !MyWorkerInfo->wi_dobalance) + return; + + LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); + MyWorkerInfo->wi_cost_delay = autovacuum_vac_cost_delay >= 0 ? + autovacuum_vac_cost_delay : VacuumCostDelay; + MyWorkerInfo->wi_cost_limit_base = autovacuum_vac_cost_limit > 0 ? + autovacuum_vac_cost_limit : VacuumCostLimit; + autovac_balance_cost(); + LWLockRelease(AutovacuumLock); +} + /* * autovac_balance_cost * Recalculate the cost limit setting for each active worker. @@ -2320,6 +2343,9 @@ do_autovacuum(void) /* * Check for config changes before processing each collected table. + * Autovacuum workers must update VacuumCostDelay and VacuumCostLimit + * in case they were overridden by the reload. However, we will do + * this as soon as we check table options a bit later. */ if (ConfigReloadPending) { @@ -2437,7 +2463,7 @@ do_autovacuum(void) autovac_balance_cost(); /* set the active cost parameters from the result of that */ - AutoVacuumUpdateDelay(); + AutoVacuumOverrideCosts(); /* done */ LWLockRelease(AutovacuumLock); diff --git a/src/include/postmaster/autovacuum.h b/src/include/postmaster/autovacuum.h index c140371b51..ee48e7123d 100644 --- a/src/include/postmaster/autovacuum.h +++ b/src/include/postmaster/autovacuum.h @@ -64,7 +64,8 @@ extern int StartAutoVacWorker(void); extern void AutoVacWorkerFailed(void); /* autovacuum cost-delay balancer */ -extern void AutoVacuumUpdateDelay(void); +extern void AutoVacuumOverrideCosts(void); +extern void AutoVacuumUpdateCosts(void); #ifdef EXEC_BACKEND extern void AutoVacLauncherMain(int argc, char *argv[]) pg_attribute_noreturn(); -- 2.37.2 [text/x-patch] v8-0001-Zero-out-VacuumCostBalance.patch (793B, ../../CAAKRu_a-y1eTNLJA3U-SwcjU4SWZ5w1QSXAN6uee3wivcjR6BQ@mail.gmail.com/3-v8-0001-Zero-out-VacuumCostBalance.patch) download | inline diff: From 9ade10882c6b2daafb846be667de04225046e157 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Sat, 25 Mar 2023 11:59:33 -0400 Subject: [PATCH v8 1/4] Zero out VacuumCostBalance Though it is unlikely to matter, we should zero out VacuumCostBalance whenever we may be transitioning the state of VacuumCostActive to false. --- src/backend/commands/vacuum.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index c54360a6a0..7d01df7e48 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -550,6 +550,7 @@ vacuum(List *relations, VacuumParams *params, { in_vacuum = false; VacuumCostActive = false; + VacuumCostBalance = 0; } PG_END_TRY(); -- 2.37.2 [text/x-patch] v8-0004-Improve-autovacuum-worker-cost-balancing.patch (17.5K, ../../CAAKRu_a-y1eTNLJA3U-SwcjU4SWZ5w1QSXAN6uee3wivcjR6BQ@mail.gmail.com/4-v8-0004-Improve-autovacuum-worker-cost-balancing.patch) download | inline diff: From 0a546a538b75ee1a736ff9a09a31412c0b323082 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Sat, 25 Mar 2023 14:14:55 -0400 Subject: [PATCH v8 4/4] Improve autovacuum worker cost balancing Before the prior commit, 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 table options 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 table options). 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 table options. --- src/backend/commands/vacuum.c | 18 ++- src/backend/postmaster/autovacuum.c | 237 ++++++++++++---------------- src/include/postmaster/autovacuum.h | 4 +- 3 files changed, 112 insertions(+), 147 deletions(-) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index cb32078c19..54ad76a729 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -2257,12 +2257,13 @@ vacuum_delay_point(void) * VacuumCostLimit and VacuumCostDelay in case they were overwritten * by reload. */ - AutoVacuumUpdateCosts(); - AutoVacuumOverrideCosts(); + AutoVacuumUpdateDelay(); + AutoVacuumUpdateLimit(); /* * If configuration changes are allowed to impact VacuumCostInactive, - * make sure it is updated. + * make sure it is updated. Autovacuum workers will have already done + * this in AutoVacuumUpdateDelay() */ if (VacuumCostInactive == VACUUM_COST_INACTIVE_AND_LOCKED) return; @@ -2314,12 +2315,13 @@ vacuum_delay_point(void) VacuumCostBalance = 0; /* - * For autovacuum workers, someone may have called - * autovac_balance_cost() since they last updated their - * VacuumCostLimit above. Do so again now to ensure they have a - * current value. + * 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. */ - AutoVacuumOverrideCosts(); + AutoVacuumUpdateLimit(); /* Might have gotten an interrupt while sleeping */ CHECK_FOR_INTERRUPTS(); diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index 8ac14a44c8..0c20442fb1 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -139,6 +139,9 @@ int Log_autovacuum_min_duration = 600000; static bool am_autovacuum_launcher = false; static bool am_autovacuum_worker = false; +static double av_table_option_cost_delay = -1; +static int av_table_option_cost_limit = 0; + /* Flags set by signal handlers */ static volatile sig_atomic_t got_SIGUSR2 = false; @@ -189,8 +192,8 @@ typedef struct autovac_table { Oid at_relid; VacuumParams at_params; - double at_vacuum_cost_delay; - int at_vacuum_cost_limit; + double at_table_option_vac_cost_delay; + int at_table_option_vac_cost_limit; bool at_dobalance; bool at_sharedrel; char *at_relname; @@ -209,7 +212,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 @@ -225,9 +228,6 @@ typedef struct WorkerInfoData TimestampTz wi_launchtime; bool 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 +273,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_nworkers_for_balance 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 +288,7 @@ typedef struct dlist_head av_runningWorkers; WorkerInfo av_startingWorker; AutoVacuumWorkItem av_workItems[NUM_WORKITEMS]; + pg_atomic_uint32 av_nworkers_for_balance; } AutoVacuumShmemStruct; static AutoVacuumShmemStruct *AutoVacuumShmem; @@ -820,7 +823,7 @@ HandleAutoVacLauncherInterrupts(void) AutoVacLauncherShutdown(); /* rebalance in case the default cost parameters changed */ - LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); + LWLockAcquire(AutovacuumLock, LW_SHARED); autovac_balance_cost(); LWLockRelease(AutovacuumLock); @@ -1756,9 +1759,6 @@ FreeWorkerInfo(int code, Datum arg) 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; dlist_push_head(&AutoVacuumShmem->av_freeWorkers, &MyWorkerInfo->wi_links); /* not mine anymore */ @@ -1773,123 +1773,114 @@ 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 VacuumCostDelay with the correct value for an autovacuum worker, + * given the value of other relevant cost-based delay parameters. Autovacuum + * workers should call this after every config reload, in case VacuumCostDelay + * was overwritten. */ void -AutoVacuumOverrideCosts(void) +AutoVacuumUpdateDelay(void) { - if (MyWorkerInfo) + if (!am_autovacuum_worker) + return; + + if (av_table_option_cost_delay >= 0) + VacuumCostDelay = av_table_option_cost_delay; + else if (autovacuum_vac_cost_delay >= 0) + VacuumCostDelay = autovacuum_vac_cost_delay; + + /* + * If configuration changes are allowed to impact VacuumCostInactive, make + * sure it is updated. + */ + if (VacuumCostInactive == VACUUM_COST_INACTIVE_AND_LOCKED) + return; + + if (VacuumCostDelay > 0) + VacuumCostInactive = VACUUM_COST_ACTIVE; + else { - VacuumCostDelay = MyWorkerInfo->wi_cost_delay; - VacuumCostLimit = MyWorkerInfo->wi_cost_limit; + VacuumCostInactive = VACUUM_COST_INACTIVE_AND_UNLOCKED; + VacuumCostBalance = 0; } } + /* - * Caller must not already hold the AutovacuumLock + * 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 also must call this after + * every config reload, in case VacuumCostLimit was overwritten. */ void -AutoVacuumUpdateCosts(void) +AutoVacuumUpdateLimit(void) { + if (!am_autovacuum_worker) + return; + /* - * Even though this autovacuum worker may be vacuuming a table with a cost - * limit table option and not a cost delay table option, we still don't - * refresh the cost delay value. + * note: in cost_limit, zero also means use value from elsewhere, because + * zero is not a valid value. */ - if (!MyWorkerInfo || !MyWorkerInfo->wi_dobalance) - return; + if (av_table_option_cost_limit > 0) + VacuumCostLimit = av_table_option_cost_limit; + else + { + /* There is at least 1 autovac worker (this worker). */ + int nworkers_for_balance = Max(pg_atomic_read_u32( + &AutoVacuumShmem->av_nworkers_for_balance), 1); - LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); - MyWorkerInfo->wi_cost_delay = autovacuum_vac_cost_delay >= 0 ? - autovacuum_vac_cost_delay : VacuumCostDelay; - MyWorkerInfo->wi_cost_limit_base = autovacuum_vac_cost_limit > 0 ? + int vac_cost_limit = autovacuum_vac_cost_limit > 0 ? autovacuum_vac_cost_limit : VacuumCostLimit; - autovac_balance_cost(); - LWLockRelease(AutovacuumLock); + + int balanced_cost_limit = vac_cost_limit / nworkers_for_balance; + + VacuumCostLimit = Max(Min(balanced_cost_limit, vac_cost_limit), 1); + } } + /* * autovac_balance_cost - * Recalculate the cost limit setting for each active worker. + * Recalculate the number of workers to consider, given table options and + * the current number of active workers. * - * Caller must hold the AutovacuumLock in exclusive mode. + * Caller must hold the AutovacuumLock in at least shared mode. */ static void autovac_balance_cost(void) { - /* - * 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; + int orig_nworkers_for_balance; + int nworkers_for_balance = 0; - /* not set? nothing to do */ - if (vac_cost_limit <= 0 || vac_cost_delay <= 0) + if (autovacuum_vac_cost_delay == 0 || + (autovacuum_vac_cost_delay == -1 && VacuumCostDelay == 0)) return; - /* calculate the total base cost limit of participating active workers */ - cost_total = 0.0; - 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) - cost_total += - (double) worker->wi_cost_limit_base / worker->wi_cost_delay; - } - - /* there are no cost limits -- nothing to do */ - if (cost_total <= 0) + if (autovacuum_vac_cost_limit <= 0 && VacuumCostLimit <= 0) return; - /* - * 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; + orig_nworkers_for_balance = + pg_atomic_read_u32(&AutoVacuumShmem->av_nworkers_for_balance); + 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 || !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_nworkers_for_balance, + nworkers_for_balance); } /* @@ -2335,8 +2326,6 @@ do_autovacuum(void) autovac_table *tab; bool isshared; bool skipit; - double stdVacuumCostDelay; - int stdVacuumCostLimit; dlist_iter iter; CHECK_FOR_INTERRUPTS(); @@ -2442,32 +2431,18 @@ 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; + av_table_option_cost_limit = tab->at_table_option_vac_cost_limit; + av_table_option_cost_delay = tab->at_table_option_vac_cost_delay; /* Must hold AutovacuumLock while mucking with cost balance info */ LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); - - /* 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; - - /* do a balance */ autovac_balance_cost(); - - /* set the active cost parameters from the result of that */ - AutoVacuumOverrideCosts(); - - /* done */ LWLockRelease(AutovacuumLock); + AutoVacuumUpdateDelay(); + AutoVacuumUpdateLimit(); + /* clean up memory before each iteration */ MemoryContextResetAndDeleteChildren(PortalContext); @@ -2551,19 +2526,15 @@ deleted: /* * 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. + * don't, set wi_dobalance to false on the assumption that we are more + * likely than not to vacuum a table with no table options 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); - - /* restore vacuum cost GUCs for the next iteration */ - VacuumCostDelay = stdVacuumCostDelay; - VacuumCostLimit = stdVacuumCostLimit; } /* @@ -2595,6 +2566,8 @@ deleted: { ConfigReloadPending = false; ProcessConfigFile(PGC_SIGHUP); + AutoVacuumUpdateDelay(); + AutoVacuumUpdateLimit(); } LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); @@ -2827,8 +2800,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; /* @@ -2838,20 +2809,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 @@ -2907,8 +2864,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_table_option_vac_cost_limit = avopts ? + avopts->vacuum_cost_limit : 0; + tab->at_table_option_vac_cost_delay = avopts ? + avopts->vacuum_cost_delay : -1; tab->at_relname = NULL; tab->at_nspname = NULL; tab->at_datname = NULL; @@ -3400,10 +3359,14 @@ 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_u32(&AutoVacuumShmem->av_nworkers_for_balance, 0); + } else Assert(found); diff --git a/src/include/postmaster/autovacuum.h b/src/include/postmaster/autovacuum.h index ee48e7123d..7b462866c9 100644 --- a/src/include/postmaster/autovacuum.h +++ b/src/include/postmaster/autovacuum.h @@ -64,8 +64,8 @@ extern int StartAutoVacWorker(void); extern void AutoVacWorkerFailed(void); /* autovacuum cost-delay balancer */ -extern void AutoVacuumOverrideCosts(void); -extern void AutoVacuumUpdateCosts(void); +extern void AutoVacuumUpdateDelay(void); +extern void AutoVacuumUpdateLimit(void); #ifdef EXEC_BACKEND extern void AutoVacLauncherMain(int argc, char *argv[]) pg_attribute_noreturn(); -- 2.37.2 [text/x-patch] v8-0002-Make-VacuumCostActive-failsafe-aware.patch (7.4K, ../../CAAKRu_a-y1eTNLJA3U-SwcjU4SWZ5w1QSXAN6uee3wivcjR6BQ@mail.gmail.com/5-v8-0002-Make-VacuumCostActive-failsafe-aware.patch) download | inline diff: From 3a9492b1f3eaacafc730aea0d53085046886ecad Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Sat, 25 Mar 2023 12:05:18 -0400 Subject: [PATCH v8 2/4] Make VacuumCostActive failsafe-aware 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, make vacuum cost status more expressive. VacuumCostActive is now VacuumCostInactive, as it can only be active in one way but it can be inactive in two ways. If performing a failsafe vacuum, the vacuum cost status cannot be enabled and is effectively "locked". If performing a non-failsafe vacuum, the vacuum cost status may be active or inactive. To express this, VacuumCostInactive can be one of three statuses: VACUUM_COST_INACTIVE_AND_LOCKED, VACUUM_COST_ACTIVE_AND_LOCKED, and VACUUM_COST_ACTIVE. VacuumCostInactive is defined as an integer because we do not want non-vacuum code concerning itself with the distinction between the three statuses -- only with whether or not VacuumCostInactive == 0 or not. --- src/backend/access/heap/vacuumlazy.c | 2 +- src/backend/commands/vacuum.c | 23 ++++++++++++++++++++--- src/backend/commands/vacuumparallel.c | 8 ++++++-- src/backend/storage/buffer/bufmgr.c | 8 ++++---- src/backend/utils/init/globals.c | 2 +- src/include/commands/vacuum.h | 8 ++++++++ src/include/miscadmin.h | 3 +-- 7 files changed, 41 insertions(+), 13 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 8f14cf85f3..040a4e931b 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -2637,7 +2637,7 @@ lazy_check_wraparound_failsafe(LVRelState *vacrel) "You might also need to consider other ways for VACUUM to keep up with the allocation of transaction IDs."))); /* Stop applying cost limits from this point on */ - VacuumCostActive = false; + VacuumCostInactive = VACUUM_COST_INACTIVE_AND_LOCKED; VacuumCostBalance = 0; return true; diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 7d01df7e48..eb126f2247 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -491,7 +491,6 @@ vacuum(List *relations, VacuumParams *params, ListCell *cur; in_vacuum = true; - VacuumCostActive = (VacuumCostDelay > 0); VacuumCostBalance = 0; VacuumPageHit = 0; VacuumPageMiss = 0; @@ -507,6 +506,24 @@ vacuum(List *relations, VacuumParams *params, { VacuumRelation *vrel = lfirst_node(VacuumRelation, cur); + /* + * failsafe_active is reset per relation, so we must be sure that + * VacuumCostInactive is set to either VACUUM_COST_INACTIVE or + * VACUUM_COST_INACTIVE_AND_UNLOCKED in between vacuuming + * relations. + */ + VacuumCostInactive = VacuumCostDelay > 0 ? VACUUM_COST_ACTIVE : + VACUUM_COST_INACTIVE_AND_UNLOCKED; + + /* + * We should not have transitioned VacuumCostInactive from + * VACUUM_COST_ACTIVE to VACUUM_COST_INACTIVE_AND_UNLOCKED above, + * as that should have happened when we changed the value of + * VacuumCostDelay. + */ + Assert(VacuumCostInactive == VACUUM_COST_ACTIVE || + VacuumCostBalance == 0); + if (params->options & VACOPT_VACUUM) { if (!vacuum_rel(vrel->oid, vrel->relation, params, false)) @@ -549,7 +566,7 @@ vacuum(List *relations, VacuumParams *params, PG_FINALLY(); { in_vacuum = false; - VacuumCostActive = false; + VacuumCostInactive = VACUUM_COST_INACTIVE_AND_UNLOCKED; VacuumCostBalance = 0; } PG_END_TRY(); @@ -2216,7 +2233,7 @@ vacuum_delay_point(void) /* Always check for interrupts */ CHECK_FOR_INTERRUPTS(); - if (!VacuumCostActive || InterruptPending) + if (VacuumCostInactive || InterruptPending) return; /* diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c index bcd40c80a1..266bf6bb4c 100644 --- a/src/backend/commands/vacuumparallel.c +++ b/src/backend/commands/vacuumparallel.c @@ -989,8 +989,12 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc) PARALLEL_VACUUM_KEY_DEAD_ITEMS, false); - /* Set cost-based vacuum delay */ - VacuumCostActive = (VacuumCostDelay > 0); + /* + * Set cost-based vacuum delay Parallel vacuum workers will not execute + * failsafe VACUUM. + */ + VacuumCostInactive = VacuumCostDelay > 0 ? VACUUM_COST_ACTIVE : + VACUUM_COST_INACTIVE_AND_UNLOCKED; VacuumCostBalance = 0; VacuumPageHit = 0; VacuumPageMiss = 0; diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 95212a3941..6d3dd26fc7 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -893,7 +893,7 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum, *hit = true; VacuumPageHit++; - if (VacuumCostActive) + if (!VacuumCostInactive) VacuumCostBalance += VacuumCostPageHit; TRACE_POSTGRESQL_BUFFER_READ_DONE(forkNum, blockNum, @@ -1098,7 +1098,7 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum, } VacuumPageMiss++; - if (VacuumCostActive) + if (!VacuumCostInactive) VacuumCostBalance += VacuumCostPageMiss; TRACE_POSTGRESQL_BUFFER_READ_DONE(forkNum, blockNum, @@ -1672,7 +1672,7 @@ MarkBufferDirty(Buffer buffer) { VacuumPageDirty++; pgBufferUsage.shared_blks_dirtied++; - if (VacuumCostActive) + if (!VacuumCostInactive) VacuumCostBalance += VacuumCostPageDirty; } } @@ -4199,7 +4199,7 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std) { VacuumPageDirty++; pgBufferUsage.shared_blks_dirtied++; - if (VacuumCostActive) + if (!VacuumCostInactive) VacuumCostBalance += VacuumCostPageDirty; } } diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c index 1b1d814254..608ebb9182 100644 --- a/src/backend/utils/init/globals.c +++ b/src/backend/utils/init/globals.c @@ -150,4 +150,4 @@ int64 VacuumPageMiss = 0; int64 VacuumPageDirty = 0; int VacuumCostBalance = 0; /* working state for vacuum */ -bool VacuumCostActive = false; +int VacuumCostInactive = 1; diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h index bdfd96cfec..5c3e250b06 100644 --- a/src/include/commands/vacuum.h +++ b/src/include/commands/vacuum.h @@ -302,6 +302,14 @@ extern PGDLLIMPORT int vacuum_failsafe_age; extern PGDLLIMPORT int vacuum_multixact_failsafe_age; /* Variables for cost-based parallel vacuum */ + +typedef enum VacuumCostStatus +{ + VACUUM_COST_INACTIVE_AND_LOCKED = -1, + VACUUM_COST_ACTIVE = 0, + VACUUM_COST_INACTIVE_AND_UNLOCKED = 1, +} VacuumCostStatus; + extern PGDLLIMPORT pg_atomic_uint32 *VacuumSharedCostBalance; extern PGDLLIMPORT pg_atomic_uint32 *VacuumActiveNWorkers; extern PGDLLIMPORT int VacuumCostBalanceLocal; diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 06a86f9ac1..33e22733ae 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -274,8 +274,7 @@ extern PGDLLIMPORT int64 VacuumPageMiss; extern PGDLLIMPORT int64 VacuumPageDirty; extern PGDLLIMPORT int VacuumCostBalance; -extern PGDLLIMPORT bool VacuumCostActive; - +extern PGDLLIMPORT int VacuumCostInactive; /* in tcop/postgres.c */ -- 2.37.2 ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Should vacuum process config file reload more often 2023-03-15 05:13 Re: Should vacuum process config file reload more often Masahiko Sawada <[email protected]> 2023-03-18 22:47 ` Re: Should vacuum process config file reload more often Melanie Plageman <[email protected]> 2023-03-23 06:08 ` Re: Should vacuum process config file reload more often Masahiko Sawada <[email protected]> 2023-03-23 16:24 ` Re: Should vacuum process config file reload more often Daniel Gustafsson <[email protected]> 2023-03-24 00:27 ` Re: Should vacuum process config file reload more often Melanie Plageman <[email protected]> 2023-03-24 05:21 ` Re: Should vacuum process config file reload more often Masahiko Sawada <[email protected]> 2023-03-24 17:27 ` Re: Should vacuum process config file reload more often Melanie Plageman <[email protected]> 2023-03-25 19:03 ` Re: Should vacuum process config file reload more often Melanie Plageman <[email protected]> @ 2023-03-27 18:12 ` Melanie Plageman <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Melanie Plageman @ 2023-03-27 18:12 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; pgsql-hackers; Andres Freund <[email protected]>; Amit Kapila <[email protected]> On Sat, Mar 25, 2023 at 3:03 PM Melanie Plageman <[email protected]> wrote: > > On Thu, Mar 23, 2023 at 8:27 PM Melanie Plageman > <[email protected]> wrote: > > On Thu, Mar 23, 2023 at 2:09 AM Masahiko Sawada <[email protected]> wrote: > > > > And, I was wondering if it was worth trying to split up the part that > > > > reloads the config file and all of the autovacuum stuff. The reloading > > > > of the config file by itself won't actually result in autovacuum workers > > > > having updated cost delays because of them overwriting it with > > > > wi_cost_delay, but it will allow VACUUM to have those updated values. > > > > > > It makes sense to me to have changes for overhauling the rebalance > > > mechanism in a separate patch. > > > > > > Looking back at the original concern you mentioned[1]: > > > > > > speed up long-running vacuum of a large table by > > > decreasing autovacuum_vacuum_cost_delay/vacuum_cost_delay, however the > > > config file is only reloaded between tables (for autovacuum) or after > > > the statement (for explicit vacuum). > > > > > > does it make sense to have autovac_balance_cost() update workers' > > > wi_cost_delay too? Autovacuum launcher already reloads the config file > > > and does the rebalance. So I thought autovac_balance_cost() can update > > > the cost_delay as well, and this might be a minimal change to deal > > > with your concern. This doesn't have the effect for manual VACUUM but > > > since vacuum delay is disabled by default it won't be a big problem. > > > As for manual VACUUMs, we would need to reload the config file in > > > vacuum_delay_point() as the part of your patch does. Overhauling the > > > rebalance mechanism would be another patch to improve it further. > > > > So, we can't do this without acquiring an access shared lock on every > > call to vacuum_delay_point() because cost delay is a double. > > > > I will work on a patchset with separate commits for reloading the config > > file, though (with autovac not benefitting in the first commit). > > So, I realized we could actually do as you say and have autovac workers > update their wi_cost_delay and keep the balance changes in a separate > commit. I've done this in attached v8. > > Workers take the exclusive lock to update their wi_cost_delay and > wi_cost_limit only when there is a config reload. So, there is one > commit that implements this behavior and a separate commit to revise the > worker rebalancing. So, I've attached an alternate version of the patchset which takes the approach of having one commit which only enables cost-based delay GUC refresh for VACUUM and another commit which enables it for autovacuum and makes the changes to balancing variables. I still think the commit which has workers updating their own wi_cost_delay in vacuum_delay_point() is a bit weird. It relies on no one else emulating our bad behavior and reading from wi_cost_delay without a lock and on no one else deciding to ever write to wi_cost_delay (even though it is in shared memory [this is the same as master]). It is only safe because our process is the only one (right now) writing to wi_cost_delay, so when we read from it without a lock, we know it isn't being written to. And everyone else takes a lock when reading from wi_cost_delay right now. So, it seems...not great. This approach also introduces a function that is only around for one commit until the next commit obsoletes it, which seems a bit silly. Basically, I think it is probably better to just have one commit enabling guc refresh for VACUUM and then another which correctly implements what is needed for autovacuum to do the same. Attached v9 does this. I've provided both complete versions of both approaches (v9 and v8). - Melanie Attachments: [text/x-patch] v9-0001-Zero-out-VacuumCostBalance.patch (793B, ../../CAAKRu_afNSU-4AtT847ms22z1w20Ldjwx4v78te_qnzE6KWYMw@mail.gmail.com/2-v9-0001-Zero-out-VacuumCostBalance.patch) download | inline diff: From 94c08c1b764619ad6cc3a0c75295f416e1863b26 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Sat, 25 Mar 2023 11:59:33 -0400 Subject: [PATCH v9 1/4] Zero out VacuumCostBalance Though it is unlikely to matter, we should zero out VacuumCostBalance whenever we may be transitioning the state of VacuumCostActive to false. --- src/backend/commands/vacuum.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index c54360a6a0..7d01df7e48 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -550,6 +550,7 @@ vacuum(List *relations, VacuumParams *params, { in_vacuum = false; VacuumCostActive = false; + VacuumCostBalance = 0; } PG_END_TRY(); -- 2.37.2 [text/x-patch] v9-0004-Autovacuum-refreshes-cost-based-delay-params-more.patch (17.7K, ../../CAAKRu_afNSU-4AtT847ms22z1w20Ldjwx4v78te_qnzE6KWYMw@mail.gmail.com/3-v9-0004-Autovacuum-refreshes-cost-based-delay-params-more.patch) download | inline diff: From 3bdf9414c5840bcc94a9c0292f25ec41d2e95b69 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Sat, 25 Mar 2023 14:14:55 -0400 Subject: [PATCH v9 4/4] Autovacuum refreshes cost-based delay params more often The previous commit allowed VACUUM to reload the config file more often so that cost-based delay parameters could take effect while VACUUMing a relation. Autovacuum, however did not benefit from this change. 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 table options 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 table options). 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 table options. --- src/backend/commands/vacuum.c | 24 ++- src/backend/postmaster/autovacuum.c | 228 +++++++++++++--------------- src/include/postmaster/autovacuum.h | 1 + 3 files changed, 124 insertions(+), 129 deletions(-) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 0e686c94b2..8e39a13285 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -2241,11 +2241,11 @@ vacuum_delay_point(void) /* * Reload the configuration file if requested. This allows changes to - * vacuum_cost_limit and vacuum_cost_delay to take effect while a table is - * being vacuumed or analyzed. Analyze should not reload configuration - * file if it is in an outer transaction, as GUC values shouldn't be - * allowed to refer to some uncommitted state (e.g. database objects - * created in this transaction). + * [autovacuum]_vacuum_cost_limit and [autovacuum]_vacuum_cost_delay to + * take effect while a table is being vacuumed or analyzed. Analyze should + * not reload configuration file if it is in an outer transaction, as GUC + * values shouldn't be allowed to refer to some uncommitted state (e.g. + * database objects created in this transaction). */ if (ConfigReloadPending && !analyze_in_outer_xact) { @@ -2258,10 +2258,12 @@ vacuum_delay_point(void) * by reload. */ AutoVacuumUpdateDelay(); + AutoVacuumUpdateLimit(); /* * If configuration changes are allowed to impact VacuumCostInactive, - * make sure it is updated. + * make sure it is updated. Autovacuum workers will have already done + * this in AutoVacuumUpdateDelay() */ if (VacuumCostInactive == VACUUM_COST_INACTIVE_AND_LOCKED) return; @@ -2312,8 +2314,14 @@ vacuum_delay_point(void) VacuumCostBalance = 0; - /* update balance values for workers */ - AutoVacuumUpdateDelay(); + /* + * 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. + */ + AutoVacuumUpdateLimit(); /* Might have gotten an interrupt while sleeping */ CHECK_FOR_INTERRUPTS(); diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index 585d28148c..6fe16aca3a 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -139,6 +139,9 @@ int Log_autovacuum_min_duration = 600000; static bool am_autovacuum_launcher = false; static bool am_autovacuum_worker = false; +static double av_table_option_cost_delay = -1; +static int av_table_option_cost_limit = 0; + /* Flags set by signal handlers */ static volatile sig_atomic_t got_SIGUSR2 = false; @@ -189,8 +192,8 @@ typedef struct autovac_table { Oid at_relid; VacuumParams at_params; - double at_vacuum_cost_delay; - int at_vacuum_cost_limit; + double at_table_option_vac_cost_delay; + int at_table_option_vac_cost_limit; bool at_dobalance; bool at_sharedrel; char *at_relname; @@ -209,7 +212,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 @@ -225,9 +228,6 @@ typedef struct WorkerInfoData TimestampTz wi_launchtime; bool 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 +273,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_nworkers_for_balance 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 +288,7 @@ typedef struct dlist_head av_runningWorkers; WorkerInfo av_startingWorker; AutoVacuumWorkItem av_workItems[NUM_WORKITEMS]; + pg_atomic_uint32 av_nworkers_for_balance; } AutoVacuumShmemStruct; static AutoVacuumShmemStruct *AutoVacuumShmem; @@ -820,7 +823,7 @@ HandleAutoVacLauncherInterrupts(void) AutoVacLauncherShutdown(); /* rebalance in case the default cost parameters changed */ - LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); + LWLockAcquire(AutovacuumLock, LW_SHARED); autovac_balance_cost(); LWLockRelease(AutovacuumLock); @@ -1756,9 +1759,6 @@ FreeWorkerInfo(int code, Datum arg) 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; dlist_push_head(&AutoVacuumShmem->av_freeWorkers, &MyWorkerInfo->wi_links); /* not mine anymore */ @@ -1773,100 +1773,114 @@ 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 VacuumCostDelay with the correct value for an autovacuum worker, + * given the value of other relevant cost-based delay parameters. Autovacuum + * workers should call this after every config reload, in case VacuumCostDelay + * was overwritten. */ void AutoVacuumUpdateDelay(void) { - if (MyWorkerInfo) + if (!am_autovacuum_worker) + return; + + if (av_table_option_cost_delay >= 0) + VacuumCostDelay = av_table_option_cost_delay; + else if (autovacuum_vac_cost_delay >= 0) + VacuumCostDelay = autovacuum_vac_cost_delay; + + /* + * If configuration changes are allowed to impact VacuumCostInactive, make + * sure it is updated. + */ + if (VacuumCostInactive == VACUUM_COST_INACTIVE_AND_LOCKED) + return; + + if (VacuumCostDelay > 0) + VacuumCostInactive = VACUUM_COST_ACTIVE; + else { - VacuumCostDelay = MyWorkerInfo->wi_cost_delay; - VacuumCostLimit = MyWorkerInfo->wi_cost_limit; + VacuumCostInactive = VACUUM_COST_INACTIVE_AND_UNLOCKED; + VacuumCostBalance = 0; } } + /* - * 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 also must call this after + * every config reload, in case VacuumCostLimit was overwritten. */ -static void -autovac_balance_cost(void) +void +AutoVacuumUpdateLimit(void) { + if (!am_autovacuum_worker) + 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; + if (av_table_option_cost_limit > 0) + VacuumCostLimit = av_table_option_cost_limit; + else + { + /* There is at least 1 autovac worker (this worker). */ + int nworkers_for_balance = Max(pg_atomic_read_u32( + &AutoVacuumShmem->av_nworkers_for_balance), 1); - /* not set? nothing to do */ - if (vac_cost_limit <= 0 || vac_cost_delay <= 0) - return; + int vac_cost_limit = autovacuum_vac_cost_limit > 0 ? + autovacuum_vac_cost_limit : VacuumCostLimit; - /* calculate the total base cost limit of participating active workers */ - cost_total = 0.0; - dlist_foreach(iter, &AutoVacuumShmem->av_runningWorkers) - { - WorkerInfo worker = dlist_container(WorkerInfoData, wi_links, iter.cur); + int balanced_cost_limit = vac_cost_limit / nworkers_for_balance; - 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; + VacuumCostLimit = Max(Min(balanced_cost_limit, vac_cost_limit), 1); } +} - /* there are no cost limits -- nothing to do */ - if (cost_total <= 0) + +/* + * autovac_balance_cost + * Recalculate the number of workers to consider, given table options and + * the current number of active workers. + * + * Caller must hold the AutovacuumLock in at least shared mode. + */ +static void +autovac_balance_cost(void) +{ + dlist_iter iter; + int orig_nworkers_for_balance; + int nworkers_for_balance = 0; + + if (autovacuum_vac_cost_delay == 0 || + (autovacuum_vac_cost_delay == -1 && VacuumCostDelay == 0)) return; - /* - * 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; + if (autovacuum_vac_cost_limit <= 0 && VacuumCostLimit <= 0) + return; + + orig_nworkers_for_balance = + pg_atomic_read_u32(&AutoVacuumShmem->av_nworkers_for_balance); + 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 || !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_nworkers_for_balance, + nworkers_for_balance); } /* @@ -2312,8 +2326,6 @@ do_autovacuum(void) autovac_table *tab; bool isshared; bool skipit; - double stdVacuumCostDelay; - int stdVacuumCostLimit; dlist_iter iter; CHECK_FOR_INTERRUPTS(); @@ -2416,31 +2428,17 @@ 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; + av_table_option_cost_limit = tab->at_table_option_vac_cost_limit; + av_table_option_cost_delay = tab->at_table_option_vac_cost_delay; /* Must hold AutovacuumLock while mucking with cost balance info */ LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); - - /* 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; - - /* do a balance */ autovac_balance_cost(); + LWLockRelease(AutovacuumLock); - /* set the active cost parameters from the result of that */ AutoVacuumUpdateDelay(); - - /* done */ - LWLockRelease(AutovacuumLock); + AutoVacuumUpdateLimit(); /* clean up memory before each iteration */ MemoryContextResetAndDeleteChildren(PortalContext); @@ -2525,19 +2523,15 @@ deleted: /* * 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. + * don't, set wi_dobalance to false on the assumption that we are more + * likely than not to vacuum a table with no table options 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); - - /* restore vacuum cost GUCs for the next iteration */ - VacuumCostDelay = stdVacuumCostDelay; - VacuumCostLimit = stdVacuumCostLimit; } /* @@ -2569,6 +2563,8 @@ deleted: { ConfigReloadPending = false; ProcessConfigFile(PGC_SIGHUP); + AutoVacuumUpdateDelay(); + AutoVacuumUpdateLimit(); } LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); @@ -2804,8 +2800,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; /* @@ -2815,20 +2809,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 @@ -2884,8 +2864,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_table_option_vac_cost_limit = avopts ? + avopts->vacuum_cost_limit : 0; + tab->at_table_option_vac_cost_delay = avopts ? + avopts->vacuum_cost_delay : -1; tab->at_relname = NULL; tab->at_nspname = NULL; tab->at_datname = NULL; @@ -3377,10 +3359,14 @@ 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_u32(&AutoVacuumShmem->av_nworkers_for_balance, 0); + } else Assert(found); diff --git a/src/include/postmaster/autovacuum.h b/src/include/postmaster/autovacuum.h index c140371b51..7b462866c9 100644 --- a/src/include/postmaster/autovacuum.h +++ b/src/include/postmaster/autovacuum.h @@ -65,6 +65,7 @@ extern void AutoVacWorkerFailed(void); /* autovacuum cost-delay balancer */ extern void AutoVacuumUpdateDelay(void); +extern void AutoVacuumUpdateLimit(void); #ifdef EXEC_BACKEND extern void AutoVacLauncherMain(int argc, char *argv[]) pg_attribute_noreturn(); -- 2.37.2 [text/x-patch] v9-0003-VACUUM-reloads-config-file-more-often.patch (5.0K, ../../CAAKRu_afNSU-4AtT847ms22z1w20Ldjwx4v78te_qnzE6KWYMw@mail.gmail.com/4-v9-0003-VACUUM-reloads-config-file-more-often.patch) download | inline diff: From 28d33a9d5cab0d384a18dbc2bb4ad95da61741c3 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Mon, 27 Mar 2023 13:33:19 -0400 Subject: [PATCH v9 3/4] VACUUM reloads config file more often Previously, VACUUM would not reload the configuration file. So, changes to cost-based delay parameters could only take effect on the next invocation of VACUUM. Check if a reload is pending roughly once per block now, when checking if we need to delay. Note that autovacuum is unaffected by this change. Autovacuum workers overwrite the value of VacuumCostLimit and VacuumCostDelay with their own WorkerInfo->wi_cost_limit and wi_cost_delay. Writing to their wi_cost_delay more often makes reading wi_cost_delay without a lock to update VacuumCostDelay an even worse idea. Reviewed-by: Masahiko Sawada <[email protected]> Reviewed-by: Daniel Gustafsson <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_buP5wzsho3qNw5o9_R0pF69FRM5hgCmr-mvXmGXwdA7A%40mail.gmail.com#5e6771d4cdca4db6efc2acec2dce0bc7 --- src/backend/commands/vacuum.c | 62 ++++++++++++++++++++++++++++++----- 1 file changed, 54 insertions(+), 8 deletions(-) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index eb126f2247..0e686c94b2 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" @@ -76,6 +77,7 @@ int vacuum_multixact_failsafe_age; /* A few variables that don't seem worth passing around as parameters */ static MemoryContext vac_context = NULL; static BufferAccessStrategy vac_strategy; +static bool analyze_in_outer_xact = false; /* @@ -314,8 +316,7 @@ vacuum(List *relations, VacuumParams *params, static bool in_vacuum = false; const char *stmttype; - volatile bool in_outer_xact, - use_own_xacts; + volatile bool use_own_xacts; Assert(params != NULL); @@ -332,10 +333,10 @@ vacuum(List *relations, VacuumParams *params, if (params->options & VACOPT_VACUUM) { PreventInTransactionBlock(isTopLevel, stmttype); - in_outer_xact = false; + analyze_in_outer_xact = false; } else - in_outer_xact = IsInTransactionBlock(isTopLevel); + analyze_in_outer_xact = IsInTransactionBlock(isTopLevel); /* * Due to static variables vac_context, anl_context and vac_strategy, @@ -457,7 +458,7 @@ vacuum(List *relations, VacuumParams *params, Assert(params->options & VACOPT_ANALYZE); if (IsAutoVacuumWorkerProcess()) use_own_xacts = true; - else if (in_outer_xact) + else if (analyze_in_outer_xact) use_own_xacts = false; else if (list_length(relations) > 1) use_own_xacts = true; @@ -475,7 +476,7 @@ vacuum(List *relations, VacuumParams *params, */ if (use_own_xacts) { - Assert(!in_outer_xact); + Assert(!analyze_in_outer_xact); /* ActiveSnapshot is not set by autovacuum */ if (ActiveSnapshotSet()) @@ -544,7 +545,7 @@ vacuum(List *relations, VacuumParams *params, } analyze_rel(vrel->oid, vrel->relation, params, - vrel->va_cols, in_outer_xact, vac_strategy); + vrel->va_cols, analyze_in_outer_xact, vac_strategy); if (use_own_xacts) { @@ -568,6 +569,7 @@ vacuum(List *relations, VacuumParams *params, in_vacuum = false; VacuumCostInactive = VACUUM_COST_INACTIVE_AND_UNLOCKED; VacuumCostBalance = 0; + analyze_in_outer_xact = false; } PG_END_TRY(); @@ -2233,7 +2235,51 @@ vacuum_delay_point(void) /* Always check for interrupts */ CHECK_FOR_INTERRUPTS(); - if (VacuumCostInactive || InterruptPending) + if (InterruptPending || + (VacuumCostInactive && !ConfigReloadPending)) + return; + + /* + * Reload the configuration file if requested. This allows changes to + * vacuum_cost_limit and vacuum_cost_delay to take effect while a table is + * being vacuumed or analyzed. Analyze should not reload configuration + * file if it is in an outer transaction, as GUC values shouldn't be + * allowed to refer to some uncommitted state (e.g. database objects + * created in this transaction). + */ + if (ConfigReloadPending && !analyze_in_outer_xact) + { + ConfigReloadPending = false; + ProcessConfigFile(PGC_SIGHUP); + + /* + * Autovacuum workers must restore the correct values of + * VacuumCostLimit and VacuumCostDelay in case they were overwritten + * by reload. + */ + AutoVacuumUpdateDelay(); + + /* + * If configuration changes are allowed to impact VacuumCostInactive, + * make sure it is updated. + */ + if (VacuumCostInactive == VACUUM_COST_INACTIVE_AND_LOCKED) + return; + + if (VacuumCostDelay > 0) + VacuumCostInactive = VACUUM_COST_ACTIVE; + else + { + VacuumCostInactive = VACUUM_COST_INACTIVE_AND_UNLOCKED; + VacuumCostBalance = 0; + } + } + + /* + * If we disabled cost-based delays after reloading the config file, + * return. + */ + if (VacuumCostInactive) return; /* -- 2.37.2 [text/x-patch] v9-0002-Make-VacuumCostActive-failsafe-aware.patch (7.4K, ../../CAAKRu_afNSU-4AtT847ms22z1w20Ldjwx4v78te_qnzE6KWYMw@mail.gmail.com/5-v9-0002-Make-VacuumCostActive-failsafe-aware.patch) download | inline diff: From 57a5284abc43fa5531574f998ac2ab8d038264d1 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Sat, 25 Mar 2023 12:05:18 -0400 Subject: [PATCH v9 2/4] Make VacuumCostActive failsafe-aware 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, make vacuum cost status more expressive. VacuumCostActive is now VacuumCostInactive, as it can only be active in one way but it can be inactive in two ways. If performing a failsafe vacuum, the vacuum cost status cannot be enabled and is effectively "locked". If performing a non-failsafe vacuum, the vacuum cost status may be active or inactive. To express this, VacuumCostInactive can be one of three statuses: VACUUM_COST_INACTIVE_AND_LOCKED, VACUUM_COST_ACTIVE_AND_LOCKED, and VACUUM_COST_ACTIVE. VacuumCostInactive is defined as an integer because we do not want non-vacuum code concerning itself with the distinction between the three statuses -- only with whether or not VacuumCostInactive == 0 or not. --- src/backend/access/heap/vacuumlazy.c | 2 +- src/backend/commands/vacuum.c | 23 ++++++++++++++++++++--- src/backend/commands/vacuumparallel.c | 8 ++++++-- src/backend/storage/buffer/bufmgr.c | 8 ++++---- src/backend/utils/init/globals.c | 2 +- src/include/commands/vacuum.h | 8 ++++++++ src/include/miscadmin.h | 3 +-- 7 files changed, 41 insertions(+), 13 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 8f14cf85f3..040a4e931b 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -2637,7 +2637,7 @@ lazy_check_wraparound_failsafe(LVRelState *vacrel) "You might also need to consider other ways for VACUUM to keep up with the allocation of transaction IDs."))); /* Stop applying cost limits from this point on */ - VacuumCostActive = false; + VacuumCostInactive = VACUUM_COST_INACTIVE_AND_LOCKED; VacuumCostBalance = 0; return true; diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 7d01df7e48..eb126f2247 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -491,7 +491,6 @@ vacuum(List *relations, VacuumParams *params, ListCell *cur; in_vacuum = true; - VacuumCostActive = (VacuumCostDelay > 0); VacuumCostBalance = 0; VacuumPageHit = 0; VacuumPageMiss = 0; @@ -507,6 +506,24 @@ vacuum(List *relations, VacuumParams *params, { VacuumRelation *vrel = lfirst_node(VacuumRelation, cur); + /* + * failsafe_active is reset per relation, so we must be sure that + * VacuumCostInactive is set to either VACUUM_COST_INACTIVE or + * VACUUM_COST_INACTIVE_AND_UNLOCKED in between vacuuming + * relations. + */ + VacuumCostInactive = VacuumCostDelay > 0 ? VACUUM_COST_ACTIVE : + VACUUM_COST_INACTIVE_AND_UNLOCKED; + + /* + * We should not have transitioned VacuumCostInactive from + * VACUUM_COST_ACTIVE to VACUUM_COST_INACTIVE_AND_UNLOCKED above, + * as that should have happened when we changed the value of + * VacuumCostDelay. + */ + Assert(VacuumCostInactive == VACUUM_COST_ACTIVE || + VacuumCostBalance == 0); + if (params->options & VACOPT_VACUUM) { if (!vacuum_rel(vrel->oid, vrel->relation, params, false)) @@ -549,7 +566,7 @@ vacuum(List *relations, VacuumParams *params, PG_FINALLY(); { in_vacuum = false; - VacuumCostActive = false; + VacuumCostInactive = VACUUM_COST_INACTIVE_AND_UNLOCKED; VacuumCostBalance = 0; } PG_END_TRY(); @@ -2216,7 +2233,7 @@ vacuum_delay_point(void) /* Always check for interrupts */ CHECK_FOR_INTERRUPTS(); - if (!VacuumCostActive || InterruptPending) + if (VacuumCostInactive || InterruptPending) return; /* diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c index bcd40c80a1..266bf6bb4c 100644 --- a/src/backend/commands/vacuumparallel.c +++ b/src/backend/commands/vacuumparallel.c @@ -989,8 +989,12 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc) PARALLEL_VACUUM_KEY_DEAD_ITEMS, false); - /* Set cost-based vacuum delay */ - VacuumCostActive = (VacuumCostDelay > 0); + /* + * Set cost-based vacuum delay Parallel vacuum workers will not execute + * failsafe VACUUM. + */ + VacuumCostInactive = VacuumCostDelay > 0 ? VACUUM_COST_ACTIVE : + VACUUM_COST_INACTIVE_AND_UNLOCKED; VacuumCostBalance = 0; VacuumPageHit = 0; VacuumPageMiss = 0; diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 95212a3941..6d3dd26fc7 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -893,7 +893,7 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum, *hit = true; VacuumPageHit++; - if (VacuumCostActive) + if (!VacuumCostInactive) VacuumCostBalance += VacuumCostPageHit; TRACE_POSTGRESQL_BUFFER_READ_DONE(forkNum, blockNum, @@ -1098,7 +1098,7 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum, } VacuumPageMiss++; - if (VacuumCostActive) + if (!VacuumCostInactive) VacuumCostBalance += VacuumCostPageMiss; TRACE_POSTGRESQL_BUFFER_READ_DONE(forkNum, blockNum, @@ -1672,7 +1672,7 @@ MarkBufferDirty(Buffer buffer) { VacuumPageDirty++; pgBufferUsage.shared_blks_dirtied++; - if (VacuumCostActive) + if (!VacuumCostInactive) VacuumCostBalance += VacuumCostPageDirty; } } @@ -4199,7 +4199,7 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std) { VacuumPageDirty++; pgBufferUsage.shared_blks_dirtied++; - if (VacuumCostActive) + if (!VacuumCostInactive) VacuumCostBalance += VacuumCostPageDirty; } } diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c index 1b1d814254..608ebb9182 100644 --- a/src/backend/utils/init/globals.c +++ b/src/backend/utils/init/globals.c @@ -150,4 +150,4 @@ int64 VacuumPageMiss = 0; int64 VacuumPageDirty = 0; int VacuumCostBalance = 0; /* working state for vacuum */ -bool VacuumCostActive = false; +int VacuumCostInactive = 1; diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h index bdfd96cfec..5c3e250b06 100644 --- a/src/include/commands/vacuum.h +++ b/src/include/commands/vacuum.h @@ -302,6 +302,14 @@ extern PGDLLIMPORT int vacuum_failsafe_age; extern PGDLLIMPORT int vacuum_multixact_failsafe_age; /* Variables for cost-based parallel vacuum */ + +typedef enum VacuumCostStatus +{ + VACUUM_COST_INACTIVE_AND_LOCKED = -1, + VACUUM_COST_ACTIVE = 0, + VACUUM_COST_INACTIVE_AND_UNLOCKED = 1, +} VacuumCostStatus; + extern PGDLLIMPORT pg_atomic_uint32 *VacuumSharedCostBalance; extern PGDLLIMPORT pg_atomic_uint32 *VacuumActiveNWorkers; extern PGDLLIMPORT int VacuumCostBalanceLocal; diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 06a86f9ac1..33e22733ae 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -274,8 +274,7 @@ extern PGDLLIMPORT int64 VacuumPageMiss; extern PGDLLIMPORT int64 VacuumPageDirty; extern PGDLLIMPORT int VacuumCostBalance; -extern PGDLLIMPORT bool VacuumCostActive; - +extern PGDLLIMPORT int VacuumCostInactive; /* in tcop/postgres.c */ -- 2.37.2 ^ permalink raw reply [nested|flat] 11+ messages in thread
* [PATCH v6 1/1] Introduce macros for protocol characters. @ 2023-08-16 19:08 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Nathan Bossart @ 2023-08-16 19:08 UTC (permalink / raw) Author: Dave Cramer Reviewed-by: Alvaro Herrera, Tatsuo Ishii, Peter Smith, Robert Haas, Tom Lane, Peter Eisentraut Discussion: https://postgr.es/m/CADK3HHKbBmK-PKf1bPNFoMC%2BoBt%2BpD9PH8h5nvmBQskEHm-Ehw%40mail.gmail.com --- src/backend/access/common/printsimple.c | 5 +- src/backend/access/transam/parallel.c | 14 ++-- src/backend/backup/basebackup_copy.c | 16 ++--- src/backend/commands/async.c | 2 +- src/backend/commands/copyfromparse.c | 22 +++---- src/backend/commands/copyto.c | 6 +- src/backend/libpq/auth-sasl.c | 2 +- src/backend/libpq/auth.c | 8 +-- src/backend/postmaster/postmaster.c | 2 +- src/backend/replication/walsender.c | 18 +++--- src/backend/tcop/dest.c | 8 +-- src/backend/tcop/fastpath.c | 2 +- src/backend/tcop/postgres.c | 68 ++++++++++---------- src/backend/utils/error/elog.c | 5 +- src/backend/utils/misc/guc.c | 2 +- src/include/Makefile | 3 +- src/include/libpq/pqcomm.h | 23 ++----- src/include/libpq/protocol.h | 85 +++++++++++++++++++++++++ src/include/meson.build | 1 + src/interfaces/libpq/fe-auth.c | 2 +- src/interfaces/libpq/fe-connect.c | 19 ++++-- src/interfaces/libpq/fe-exec.c | 50 +++++++-------- src/interfaces/libpq/fe-protocol3.c | 70 ++++++++++---------- src/interfaces/libpq/fe-trace.c | 70 +++++++++++--------- 24 files changed, 301 insertions(+), 202 deletions(-) create mode 100644 src/include/libpq/protocol.h diff --git a/src/backend/access/common/printsimple.c b/src/backend/access/common/printsimple.c index ef818228ac..675b744db2 100644 --- a/src/backend/access/common/printsimple.c +++ b/src/backend/access/common/printsimple.c @@ -20,6 +20,7 @@ #include "access/printsimple.h" #include "catalog/pg_type.h" +#include "libpq/protocol.h" #include "libpq/pqformat.h" #include "utils/builtins.h" @@ -32,7 +33,7 @@ printsimple_startup(DestReceiver *self, int operation, TupleDesc tupdesc) StringInfoData buf; int i; - pq_beginmessage(&buf, 'T'); /* RowDescription */ + pq_beginmessage(&buf, PqMsg_RowDescription); pq_sendint16(&buf, tupdesc->natts); for (i = 0; i < tupdesc->natts; ++i) @@ -65,7 +66,7 @@ printsimple(TupleTableSlot *slot, DestReceiver *self) slot_getallattrs(slot); /* Prepare and send message */ - pq_beginmessage(&buf, 'D'); + pq_beginmessage(&buf, PqMsg_DataRow); pq_sendint16(&buf, tupdesc->natts); for (i = 0; i < tupdesc->natts; ++i) diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c index 1738aecf1f..194a1207be 100644 --- a/src/backend/access/transam/parallel.c +++ b/src/backend/access/transam/parallel.c @@ -1127,7 +1127,7 @@ HandleParallelMessage(ParallelContext *pcxt, int i, StringInfo msg) switch (msgtype) { - case 'K': /* BackendKeyData */ + case PqMsg_BackendKeyData: { int32 pid = pq_getmsgint(msg, 4); @@ -1137,8 +1137,8 @@ HandleParallelMessage(ParallelContext *pcxt, int i, StringInfo msg) break; } - case 'E': /* ErrorResponse */ - case 'N': /* NoticeResponse */ + case PqMsg_ErrorResponse: + case PqMsg_NoticeResponse: { ErrorData edata; ErrorContextCallback *save_error_context_stack; @@ -1183,7 +1183,7 @@ HandleParallelMessage(ParallelContext *pcxt, int i, StringInfo msg) break; } - case 'A': /* NotifyResponse */ + case PqMsg_NotificationResponse: { /* Propagate NotifyResponse. */ int32 pid; @@ -1217,7 +1217,7 @@ HandleParallelMessage(ParallelContext *pcxt, int i, StringInfo msg) break; } - case 'X': /* Terminate, indicating clean exit */ + case PqMsg_Terminate: { shm_mq_detach(pcxt->worker[i].error_mqh); pcxt->worker[i].error_mqh = NULL; @@ -1372,7 +1372,7 @@ ParallelWorkerMain(Datum main_arg) * protocol message is defined, but it won't actually be used for anything * in this case. */ - pq_beginmessage(&msgbuf, 'K'); + pq_beginmessage(&msgbuf, PqMsg_BackendKeyData); pq_sendint32(&msgbuf, (int32) MyProcPid); pq_sendint32(&msgbuf, (int32) MyCancelKey); pq_endmessage(&msgbuf); @@ -1550,7 +1550,7 @@ ParallelWorkerMain(Datum main_arg) DetachSession(); /* Report success. */ - pq_putmessage('X', NULL, 0); + pq_putmessage(PqMsg_Terminate, NULL, 0); } /* diff --git a/src/backend/backup/basebackup_copy.c b/src/backend/backup/basebackup_copy.c index 1db80cde1b..fee30c21e1 100644 --- a/src/backend/backup/basebackup_copy.c +++ b/src/backend/backup/basebackup_copy.c @@ -152,7 +152,7 @@ bbsink_copystream_begin_backup(bbsink *sink) SendTablespaceList(state->tablespaces); /* Send a CommandComplete message */ - pq_puttextmessage('C', "SELECT"); + pq_puttextmessage(PqMsg_CommandComplete, "SELECT"); /* Begin COPY stream. This will be used for all archives + manifest. */ SendCopyOutResponse(); @@ -169,7 +169,7 @@ bbsink_copystream_begin_archive(bbsink *sink, const char *archive_name) StringInfoData buf; ti = list_nth(state->tablespaces, state->tablespace_num); - pq_beginmessage(&buf, 'd'); /* CopyData */ + pq_beginmessage(&buf, PqMsg_CopyData); pq_sendbyte(&buf, 'n'); /* New archive */ pq_sendstring(&buf, archive_name); pq_sendstring(&buf, ti->path == NULL ? "" : ti->path); @@ -220,7 +220,7 @@ bbsink_copystream_archive_contents(bbsink *sink, size_t len) { mysink->last_progress_report_time = now; - pq_beginmessage(&buf, 'd'); /* CopyData */ + pq_beginmessage(&buf, PqMsg_CopyData); pq_sendbyte(&buf, 'p'); /* Progress report */ pq_sendint64(&buf, state->bytes_done); pq_endmessage(&buf); @@ -246,7 +246,7 @@ bbsink_copystream_end_archive(bbsink *sink) mysink->bytes_done_at_last_time_check = state->bytes_done; mysink->last_progress_report_time = GetCurrentTimestamp(); - pq_beginmessage(&buf, 'd'); /* CopyData */ + pq_beginmessage(&buf, PqMsg_CopyData); pq_sendbyte(&buf, 'p'); /* Progress report */ pq_sendint64(&buf, state->bytes_done); pq_endmessage(&buf); @@ -261,7 +261,7 @@ bbsink_copystream_begin_manifest(bbsink *sink) { StringInfoData buf; - pq_beginmessage(&buf, 'd'); /* CopyData */ + pq_beginmessage(&buf, PqMsg_CopyData); pq_sendbyte(&buf, 'm'); /* Manifest */ pq_endmessage(&buf); } @@ -318,7 +318,7 @@ SendCopyOutResponse(void) { StringInfoData buf; - pq_beginmessage(&buf, 'H'); + pq_beginmessage(&buf, PqMsg_CopyOutResponse); pq_sendbyte(&buf, 0); /* overall format */ pq_sendint16(&buf, 0); /* natts */ pq_endmessage(&buf); @@ -330,7 +330,7 @@ SendCopyOutResponse(void) static void SendCopyDone(void) { - pq_putemptymessage('c'); + pq_putemptymessage(PqMsg_CopyDone); } /* @@ -368,7 +368,7 @@ SendXlogRecPtrResult(XLogRecPtr ptr, TimeLineID tli) end_tup_output(tstate); /* Send a CommandComplete message */ - pq_puttextmessage('C', "SELECT"); + pq_puttextmessage(PqMsg_CommandComplete, "SELECT"); } /* diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c index ef909cf4e0..d148d10850 100644 --- a/src/backend/commands/async.c +++ b/src/backend/commands/async.c @@ -2281,7 +2281,7 @@ NotifyMyFrontEnd(const char *channel, const char *payload, int32 srcPid) { StringInfoData buf; - pq_beginmessage(&buf, 'A'); + pq_beginmessage(&buf, PqMsg_NotificationResponse); pq_sendint32(&buf, srcPid); pq_sendstring(&buf, channel); pq_sendstring(&buf, payload); diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c index 232768a6e1..f553734582 100644 --- a/src/backend/commands/copyfromparse.c +++ b/src/backend/commands/copyfromparse.c @@ -174,7 +174,7 @@ ReceiveCopyBegin(CopyFromState cstate) int16 format = (cstate->opts.binary ? 1 : 0); int i; - pq_beginmessage(&buf, 'G'); + pq_beginmessage(&buf, PqMsg_CopyInResponse); pq_sendbyte(&buf, format); /* overall format */ pq_sendint16(&buf, natts); for (i = 0; i < natts; i++) @@ -279,13 +279,13 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread) /* Validate message type and set packet size limit */ switch (mtype) { - case 'd': /* CopyData */ + case PqMsg_CopyData: maxmsglen = PQ_LARGE_MESSAGE_LIMIT; break; - case 'c': /* CopyDone */ - case 'f': /* CopyFail */ - case 'H': /* Flush */ - case 'S': /* Sync */ + case PqMsg_CopyDone: + case PqMsg_CopyFail: + case PqMsg_Flush: + case PqMsg_Sync: maxmsglen = PQ_SMALL_MESSAGE_LIMIT; break; default: @@ -305,20 +305,20 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread) /* ... and process it */ switch (mtype) { - case 'd': /* CopyData */ + case PqMsg_CopyData: break; - case 'c': /* CopyDone */ + case PqMsg_CopyDone: /* COPY IN correctly terminated by frontend */ cstate->raw_reached_eof = true; return bytesread; - case 'f': /* CopyFail */ + case PqMsg_CopyFail: ereport(ERROR, (errcode(ERRCODE_QUERY_CANCELED), errmsg("COPY from stdin failed: %s", pq_getmsgstring(cstate->fe_msgbuf)))); break; - case 'H': /* Flush */ - case 'S': /* Sync */ + case PqMsg_Flush: + case PqMsg_Sync: /* * Ignore Flush/Sync for the convenience of client diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c index 9e4b2437a5..eaa3172793 100644 --- a/src/backend/commands/copyto.c +++ b/src/backend/commands/copyto.c @@ -144,7 +144,7 @@ SendCopyBegin(CopyToState cstate) int16 format = (cstate->opts.binary ? 1 : 0); int i; - pq_beginmessage(&buf, 'H'); + pq_beginmessage(&buf, PqMsg_CopyOutResponse); pq_sendbyte(&buf, format); /* overall format */ pq_sendint16(&buf, natts); for (i = 0; i < natts; i++) @@ -159,7 +159,7 @@ SendCopyEnd(CopyToState cstate) /* Shouldn't have any unsent data */ Assert(cstate->fe_msgbuf->len == 0); /* Send Copy Done message */ - pq_putemptymessage('c'); + pq_putemptymessage(PqMsg_CopyDone); } /*---------- @@ -247,7 +247,7 @@ CopySendEndOfRow(CopyToState cstate) CopySendChar(cstate, '\n'); /* Dump the accumulated row as one CopyData message */ - (void) pq_putmessage('d', fe_msgbuf->data, fe_msgbuf->len); + (void) pq_putmessage(PqMsg_CopyData, fe_msgbuf->data, fe_msgbuf->len); break; case COPY_CALLBACK: cstate->data_dest_cb(fe_msgbuf->data, fe_msgbuf->len); diff --git a/src/backend/libpq/auth-sasl.c b/src/backend/libpq/auth-sasl.c index 684680897b..c535bc5383 100644 --- a/src/backend/libpq/auth-sasl.c +++ b/src/backend/libpq/auth-sasl.c @@ -87,7 +87,7 @@ CheckSASLAuth(const pg_be_sasl_mech *mech, Port *port, char *shadow_pass, { pq_startmsgread(); mtype = pq_getbyte(); - if (mtype != 'p') + if (mtype != PqMsg_SASLResponse) { /* Only log error if client didn't disconnect. */ if (mtype != EOF) diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c index 315a24bb3f..0356fe3e45 100644 --- a/src/backend/libpq/auth.c +++ b/src/backend/libpq/auth.c @@ -665,7 +665,7 @@ sendAuthRequest(Port *port, AuthRequest areq, const char *extradata, int extrale CHECK_FOR_INTERRUPTS(); - pq_beginmessage(&buf, 'R'); + pq_beginmessage(&buf, PqMsg_AuthenticationRequest); pq_sendint32(&buf, (int32) areq); if (extralen > 0) pq_sendbytes(&buf, extradata, extralen); @@ -698,7 +698,7 @@ recv_password_packet(Port *port) /* Expect 'p' message type */ mtype = pq_getbyte(); - if (mtype != 'p') + if (mtype != PqMsg_PasswordMessage) { /* * If the client just disconnects without offering a password, don't @@ -961,7 +961,7 @@ pg_GSS_recvauth(Port *port) CHECK_FOR_INTERRUPTS(); mtype = pq_getbyte(); - if (mtype != 'p') + if (mtype != PqMsg_GSSResponse) { /* Only log error if client didn't disconnect. */ if (mtype != EOF) @@ -1232,7 +1232,7 @@ pg_SSPI_recvauth(Port *port) { pq_startmsgread(); mtype = pq_getbyte(); - if (mtype != 'p') + if (mtype != PqMsg_GSSResponse) { if (sspictx != NULL) { diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 9c8ec779f9..07d376d77e 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -2357,7 +2357,7 @@ SendNegotiateProtocolVersion(List *unrecognized_protocol_options) StringInfoData buf; ListCell *lc; - pq_beginmessage(&buf, 'v'); /* NegotiateProtocolVersion */ + pq_beginmessage(&buf, PqMsg_NegotiateProtocolVersion); pq_sendint32(&buf, PG_PROTOCOL_LATEST); pq_sendint32(&buf, list_length(unrecognized_protocol_options)); foreach(lc, unrecognized_protocol_options) diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c index d27ef2985d..80374c55be 100644 --- a/src/backend/replication/walsender.c +++ b/src/backend/replication/walsender.c @@ -603,7 +603,7 @@ SendTimeLineHistory(TimeLineHistoryCmd *cmd) dest->rStartup(dest, CMD_SELECT, tupdesc); /* Send a DataRow message */ - pq_beginmessage(&buf, 'D'); + pq_beginmessage(&buf, PqMsg_DataRow); pq_sendint16(&buf, 2); /* # of columns */ len = strlen(histfname); pq_sendint32(&buf, len); /* col1 len */ @@ -801,7 +801,7 @@ StartReplication(StartReplicationCmd *cmd) WalSndSetState(WALSNDSTATE_CATCHUP); /* Send a CopyBothResponse message, and start streaming */ - pq_beginmessage(&buf, 'W'); + pq_beginmessage(&buf, PqMsg_CopyBothResponse); pq_sendbyte(&buf, 0); pq_sendint16(&buf, 0); pq_endmessage(&buf); @@ -1294,7 +1294,7 @@ StartLogicalReplication(StartReplicationCmd *cmd) WalSndSetState(WALSNDSTATE_CATCHUP); /* Send a CopyBothResponse message, and start streaming */ - pq_beginmessage(&buf, 'W'); + pq_beginmessage(&buf, PqMsg_CopyBothResponse); pq_sendbyte(&buf, 0); pq_sendint16(&buf, 0); pq_endmessage(&buf); @@ -1923,11 +1923,11 @@ ProcessRepliesIfAny(void) /* Validate message type and set packet size limit */ switch (firstchar) { - case 'd': + case PqMsg_CopyData: maxmsglen = PQ_LARGE_MESSAGE_LIMIT; break; - case 'c': - case 'X': + case PqMsg_CopyDone: + case PqMsg_Terminate: maxmsglen = PQ_SMALL_MESSAGE_LIMIT; break; default: @@ -1955,7 +1955,7 @@ ProcessRepliesIfAny(void) /* * 'd' means a standby reply wrapped in a CopyData packet. */ - case 'd': + case PqMsg_CopyData: ProcessStandbyMessage(); received = true; break; @@ -1964,7 +1964,7 @@ ProcessRepliesIfAny(void) * CopyDone means the standby requested to finish streaming. * Reply with CopyDone, if we had not sent that already. */ - case 'c': + case PqMsg_CopyDone: if (!streamingDoneSending) { pq_putmessage_noblock('c', NULL, 0); @@ -1978,7 +1978,7 @@ ProcessRepliesIfAny(void) /* * 'X' means that the standby is closing down the socket. */ - case 'X': + case PqMsg_Terminate: proc_exit(0); default: diff --git a/src/backend/tcop/dest.c b/src/backend/tcop/dest.c index c0406e2ee5..06d1872b9a 100644 --- a/src/backend/tcop/dest.c +++ b/src/backend/tcop/dest.c @@ -176,7 +176,7 @@ EndCommand(const QueryCompletion *qc, CommandDest dest, bool force_undecorated_o len = BuildQueryCompletionString(completionTag, qc, force_undecorated_output); - pq_putmessage('C', completionTag, len + 1); + pq_putmessage(PqMsg_Close, completionTag, len + 1); case DestNone: case DestDebug: @@ -200,7 +200,7 @@ EndCommand(const QueryCompletion *qc, CommandDest dest, bool force_undecorated_o void EndReplicationCommand(const char *commandTag) { - pq_putmessage('C', commandTag, strlen(commandTag) + 1); + pq_putmessage(PqMsg_Close, commandTag, strlen(commandTag) + 1); } /* ---------------- @@ -220,7 +220,7 @@ NullCommand(CommandDest dest) case DestRemoteSimple: /* Tell the FE that we saw an empty query string */ - pq_putemptymessage('I'); + pq_putemptymessage(PqMsg_EmptyQueryResponse); break; case DestNone: @@ -258,7 +258,7 @@ ReadyForQuery(CommandDest dest) { StringInfoData buf; - pq_beginmessage(&buf, 'Z'); + pq_beginmessage(&buf, PqMsg_ReadyForQuery); pq_sendbyte(&buf, TransactionBlockStatusCode()); pq_endmessage(&buf); } diff --git a/src/backend/tcop/fastpath.c b/src/backend/tcop/fastpath.c index 2f70ebd5fa..71f161dbe2 100644 --- a/src/backend/tcop/fastpath.c +++ b/src/backend/tcop/fastpath.c @@ -69,7 +69,7 @@ SendFunctionResult(Datum retval, bool isnull, Oid rettype, int16 format) { StringInfoData buf; - pq_beginmessage(&buf, 'V'); + pq_beginmessage(&buf, PqMsg_FunctionCallResponse); if (isnull) { diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 36cc99ec9c..e4756f8be2 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -402,37 +402,37 @@ SocketBackend(StringInfo inBuf) */ switch (qtype) { - case 'Q': /* simple query */ + case PqMsg_Query: maxmsglen = PQ_LARGE_MESSAGE_LIMIT; doing_extended_query_message = false; break; - case 'F': /* fastpath function call */ + case PqMsg_FunctionCall: maxmsglen = PQ_LARGE_MESSAGE_LIMIT; doing_extended_query_message = false; break; - case 'X': /* terminate */ + case PqMsg_Terminate: maxmsglen = PQ_SMALL_MESSAGE_LIMIT; doing_extended_query_message = false; ignore_till_sync = false; break; - case 'B': /* bind */ - case 'P': /* parse */ + case PqMsg_Bind: + case PqMsg_Parse: maxmsglen = PQ_LARGE_MESSAGE_LIMIT; doing_extended_query_message = true; break; - case 'C': /* close */ - case 'D': /* describe */ - case 'E': /* execute */ - case 'H': /* flush */ + case PqMsg_Close: + case PqMsg_Describe: + case PqMsg_Execute: + case PqMsg_Flush: maxmsglen = PQ_SMALL_MESSAGE_LIMIT; doing_extended_query_message = true; break; - case 'S': /* sync */ + case PqMsg_Sync: maxmsglen = PQ_SMALL_MESSAGE_LIMIT; /* stop any active skip-till-Sync */ ignore_till_sync = false; @@ -440,13 +440,13 @@ SocketBackend(StringInfo inBuf) doing_extended_query_message = false; break; - case 'd': /* copy data */ + case PqMsg_CopyData: maxmsglen = PQ_LARGE_MESSAGE_LIMIT; doing_extended_query_message = false; break; - case 'c': /* copy done */ - case 'f': /* copy fail */ + case PqMsg_CopyDone: + case PqMsg_CopyFail: maxmsglen = PQ_SMALL_MESSAGE_LIMIT; doing_extended_query_message = false; break; @@ -1589,7 +1589,7 @@ exec_parse_message(const char *query_string, /* string to execute */ * Send ParseComplete. */ if (whereToSendOutput == DestRemote) - pq_putemptymessage('1'); + pq_putemptymessage(PqMsg_ParseComplete); /* * Emit duration logging if appropriate. @@ -2047,7 +2047,7 @@ exec_bind_message(StringInfo input_message) * Send BindComplete. */ if (whereToSendOutput == DestRemote) - pq_putemptymessage('2'); + pq_putemptymessage(PqMsg_BindComplete); /* * Emit duration logging if appropriate. @@ -2290,7 +2290,7 @@ exec_execute_message(const char *portal_name, long max_rows) { /* Portal run not complete, so send PortalSuspended */ if (whereToSendOutput == DestRemote) - pq_putemptymessage('s'); + pq_putemptymessage(PqMsg_PortalSuspended); /* * Set XACT_FLAGS_PIPELINING whenever we suspend an Execute message, @@ -2683,7 +2683,7 @@ exec_describe_statement_message(const char *stmt_name) NULL); } else - pq_putemptymessage('n'); /* NoData */ + pq_putemptymessage(PqMsg_NoData); } /* @@ -2736,7 +2736,7 @@ exec_describe_portal_message(const char *portal_name) FetchPortalTargetList(portal), portal->formats); else - pq_putemptymessage('n'); /* NoData */ + pq_putemptymessage(PqMsg_NoData); } @@ -4239,7 +4239,7 @@ PostgresMain(const char *dbname, const char *username) { StringInfoData buf; - pq_beginmessage(&buf, 'K'); + pq_beginmessage(&buf, PqMsg_BackendKeyData); pq_sendint32(&buf, (int32) MyProcPid); pq_sendint32(&buf, (int32) MyCancelKey); pq_endmessage(&buf); @@ -4618,7 +4618,7 @@ PostgresMain(const char *dbname, const char *username) switch (firstchar) { - case 'Q': /* simple query */ + case PqMsg_Query: { const char *query_string; @@ -4642,7 +4642,7 @@ PostgresMain(const char *dbname, const char *username) } break; - case 'P': /* parse */ + case PqMsg_Parse: { const char *stmt_name; const char *query_string; @@ -4672,7 +4672,7 @@ PostgresMain(const char *dbname, const char *username) } break; - case 'B': /* bind */ + case PqMsg_Bind: forbidden_in_wal_sender(firstchar); /* Set statement_timestamp() */ @@ -4687,7 +4687,7 @@ PostgresMain(const char *dbname, const char *username) /* exec_bind_message does valgrind_report_error_query */ break; - case 'E': /* execute */ + case PqMsg_Execute: { const char *portal_name; int max_rows; @@ -4707,7 +4707,7 @@ PostgresMain(const char *dbname, const char *username) } break; - case 'F': /* fastpath function call */ + case PqMsg_FunctionCall: forbidden_in_wal_sender(firstchar); /* Set statement_timestamp() */ @@ -4742,7 +4742,7 @@ PostgresMain(const char *dbname, const char *username) send_ready_for_query = true; break; - case 'C': /* close */ + case PqMsg_Close: { int close_type; const char *close_target; @@ -4782,13 +4782,13 @@ PostgresMain(const char *dbname, const char *username) } if (whereToSendOutput == DestRemote) - pq_putemptymessage('3'); /* CloseComplete */ + pq_putemptymessage(PqMsg_CloseComplete); valgrind_report_error_query("CLOSE message"); } break; - case 'D': /* describe */ + case PqMsg_Describe: { int describe_type; const char *describe_target; @@ -4822,13 +4822,13 @@ PostgresMain(const char *dbname, const char *username) } break; - case 'H': /* flush */ + case PqMsg_Flush: pq_getmsgend(&input_message); if (whereToSendOutput == DestRemote) pq_flush(); break; - case 'S': /* sync */ + case PqMsg_Sync: pq_getmsgend(&input_message); finish_xact_command(); valgrind_report_error_query("SYNC message"); @@ -4847,7 +4847,7 @@ PostgresMain(const char *dbname, const char *username) /* FALLTHROUGH */ - case 'X': + case PqMsg_Terminate: /* * Reset whereToSendOutput to prevent ereport from attempting @@ -4865,9 +4865,9 @@ PostgresMain(const char *dbname, const char *username) */ proc_exit(0); - case 'd': /* copy data */ - case 'c': /* copy done */ - case 'f': /* copy fail */ + case PqMsg_CopyData: + case PqMsg_CopyDone: + case PqMsg_CopyFail: /* * Accept but ignore these messages, per protocol spec; we @@ -4897,7 +4897,7 @@ forbidden_in_wal_sender(char firstchar) { if (am_walsender) { - if (firstchar == 'F') + if (firstchar == PqMsg_FunctionCall) ereport(ERROR, (errcode(ERRCODE_PROTOCOL_VIOLATION), errmsg("fastpath function calls not supported in a replication connection"))); diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c index 5898100acb..8e1f3e8521 100644 --- a/src/backend/utils/error/elog.c +++ b/src/backend/utils/error/elog.c @@ -3465,7 +3465,10 @@ send_message_to_frontend(ErrorData *edata) char tbuf[12]; /* 'N' (Notice) is for nonfatal conditions, 'E' is for errors */ - pq_beginmessage(&msgbuf, (edata->elevel < ERROR) ? 'N' : 'E'); + if (edata->elevel < ERROR) + pq_beginmessage(&msgbuf, PqMsg_NoticeResponse); + else + pq_beginmessage(&msgbuf, PqMsg_ErrorResponse); sev = error_severity(edata->elevel); pq_sendbyte(&msgbuf, PG_DIAG_SEVERITY); diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 99bb2fdd19..84e7ad4d90 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -2593,7 +2593,7 @@ ReportGUCOption(struct config_generic *record) { StringInfoData msgbuf; - pq_beginmessage(&msgbuf, 'S'); + pq_beginmessage(&msgbuf, PqMsg_ParameterStatus); pq_sendstring(&msgbuf, record->name); pq_sendstring(&msgbuf, val); pq_endmessage(&msgbuf); diff --git a/src/include/Makefile b/src/include/Makefile index 5d213187e2..2d5242561c 100644 --- a/src/include/Makefile +++ b/src/include/Makefile @@ -40,6 +40,7 @@ install: all installdirs $(INSTALL_DATA) $(srcdir)/port.h '$(DESTDIR)$(includedir_internal)' $(INSTALL_DATA) $(srcdir)/postgres_fe.h '$(DESTDIR)$(includedir_internal)' $(INSTALL_DATA) $(srcdir)/libpq/pqcomm.h '$(DESTDIR)$(includedir_internal)/libpq' + $(INSTALL_DATA) $(srcdir)/libpq/protocol.h '$(DESTDIR)$(includedir_internal)/libpq' # These headers are needed for server-side development $(INSTALL_DATA) pg_config.h '$(DESTDIR)$(includedir_server)' $(INSTALL_DATA) pg_config_ext.h '$(DESTDIR)$(includedir_server)' @@ -65,7 +66,7 @@ installdirs: uninstall: rm -f $(addprefix '$(DESTDIR)$(includedir)'/, pg_config.h pg_config_ext.h pg_config_os.h pg_config_manual.h postgres_ext.h libpq/libpq-fs.h) - rm -f $(addprefix '$(DESTDIR)$(includedir_internal)'/, c.h port.h postgres_fe.h libpq/pqcomm.h) + rm -f $(addprefix '$(DESTDIR)$(includedir_internal)'/, c.h port.h postgres_fe.h libpq/pqcomm.h libpq/protocol.h) # heuristic... rm -rf $(addprefix '$(DESTDIR)$(includedir_server)'/, $(SUBDIRS) *.h) diff --git a/src/include/libpq/pqcomm.h b/src/include/libpq/pqcomm.h index 3da00f7983..46a0946b8b 100644 --- a/src/include/libpq/pqcomm.h +++ b/src/include/libpq/pqcomm.h @@ -21,6 +21,12 @@ #include <netdb.h> #include <netinet/in.h> +/* + * The definitions for the request/response codes are kept in a separate file + * for ease of use in third party programs. + */ +#include "libpq/protocol.h" + typedef struct { struct sockaddr_storage addr; @@ -112,23 +118,6 @@ typedef uint32 PacketLen; #define MAX_STARTUP_PACKET_LENGTH 10000 -/* These are the authentication request codes sent by the backend. */ - -#define AUTH_REQ_OK 0 /* User is authenticated */ -#define AUTH_REQ_KRB4 1 /* Kerberos V4. Not supported any more. */ -#define AUTH_REQ_KRB5 2 /* Kerberos V5. Not supported any more. */ -#define AUTH_REQ_PASSWORD 3 /* Password */ -#define AUTH_REQ_CRYPT 4 /* crypt password. Not supported any more. */ -#define AUTH_REQ_MD5 5 /* md5 password */ -/* 6 is available. It was used for SCM creds, not supported any more. */ -#define AUTH_REQ_GSS 7 /* GSSAPI without wrap() */ -#define AUTH_REQ_GSS_CONT 8 /* Continue GSS exchanges */ -#define AUTH_REQ_SSPI 9 /* SSPI negotiate without wrap() */ -#define AUTH_REQ_SASL 10 /* Begin SASL authentication */ -#define AUTH_REQ_SASL_CONT 11 /* Continue SASL authentication */ -#define AUTH_REQ_SASL_FIN 12 /* Final SASL message */ -#define AUTH_REQ_MAX AUTH_REQ_SASL_FIN /* maximum AUTH_REQ_* value */ - typedef uint32 AuthRequest; diff --git a/src/include/libpq/protocol.h b/src/include/libpq/protocol.h new file mode 100644 index 0000000000..cc46f4b586 --- /dev/null +++ b/src/include/libpq/protocol.h @@ -0,0 +1,85 @@ +/*------------------------------------------------------------------------- + * + * protocol.h + * Definitions of the request/response codes for the wire protocol. + * + * + * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * src/include/libpq/protocol.h + * + *------------------------------------------------------------------------- + */ +#ifndef PROTOCOL_H +#define PROTOCOL_H + +/* These are the request codes sent by the frontend. */ + +#define PqMsg_Bind 'B' +#define PqMsg_Close 'C' +#define PqMsg_Describe 'D' +#define PqMsg_Execute 'E' +#define PqMsg_FunctionCall 'F' +#define PqMsg_Flush 'H' +#define PqMsg_Parse 'P' +#define PqMsg_Query 'Q' +#define PqMsg_Sync 'S' +#define PqMsg_Terminate 'X' +#define PqMsg_CopyFail 'f' +#define PqMsg_GSSResponse 'p' +#define PqMsg_PasswordMessage 'p' +#define PqMsg_SASLInitialResponse 'p' +#define PqMsg_SASLResponse 'p' + + +/* These are the response codes sent by the backend. */ + +#define PqMsg_ParseComplete '1' +#define PqMsg_BindComplete '2' +#define PqMsg_CloseComplete '3' +#define PqMsg_NotificationResponse 'A' +#define PqMsg_CommandComplete 'C' +#define PqMsg_DataRow 'D' +#define PqMsg_ErrorResponse 'E' +#define PqMsg_CopyInResponse 'G' +#define PqMsg_CopyOutResponse 'H' +#define PqMsg_EmptyQueryResponse 'I' +#define PqMsg_BackendKeyData 'K' +#define PqMsg_NoticeResponse 'N' +#define PqMsg_AuthenticationRequest 'R' +#define PqMsg_ParameterStatus 'S' +#define PqMsg_RowDescription 'T' +#define PqMsg_FunctionCallResponse 'V' +#define PqMsg_CopyBothResponse 'W' +#define PqMsg_ReadyForQuery 'Z' +#define PqMsg_NoData 'n' +#define PqMsg_PortalSuspended 's' +#define PqMsg_ParameterDescription 't' +#define PqMsg_NegotiateProtocolVersion 'v' + + +/* These are the codes sent by both the frontend and backend. */ + +#define PqMsg_CopyDone 'c' +#define PqMsg_CopyData 'd' + + +/* These are the authentication request codes sent by the backend. */ + +#define AUTH_REQ_OK 0 /* User is authenticated */ +#define AUTH_REQ_KRB4 1 /* Kerberos V4. Not supported any more. */ +#define AUTH_REQ_KRB5 2 /* Kerberos V5. Not supported any more. */ +#define AUTH_REQ_PASSWORD 3 /* Password */ +#define AUTH_REQ_CRYPT 4 /* crypt password. Not supported any more. */ +#define AUTH_REQ_MD5 5 /* md5 password */ +/* 6 is available. It was used for SCM creds, not supported any more. */ +#define AUTH_REQ_GSS 7 /* GSSAPI without wrap() */ +#define AUTH_REQ_GSS_CONT 8 /* Continue GSS exchanges */ +#define AUTH_REQ_SSPI 9 /* SSPI negotiate without wrap() */ +#define AUTH_REQ_SASL 10 /* Begin SASL authentication */ +#define AUTH_REQ_SASL_CONT 11 /* Continue SASL authentication */ +#define AUTH_REQ_SASL_FIN 12 /* Final SASL message */ +#define AUTH_REQ_MAX AUTH_REQ_SASL_FIN /* maximum AUTH_REQ_* value */ + +#endif /* PROTOCOL_H */ diff --git a/src/include/meson.build b/src/include/meson.build index d7e1ecd4c9..d50897c9fd 100644 --- a/src/include/meson.build +++ b/src/include/meson.build @@ -94,6 +94,7 @@ install_headers( install_headers( 'libpq/pqcomm.h', + 'libpq/protocol.h', install_dir: dir_include_internal / 'libpq', ) diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c index 887ca5e9e1..912aa14821 100644 --- a/src/interfaces/libpq/fe-auth.c +++ b/src/interfaces/libpq/fe-auth.c @@ -586,7 +586,7 @@ pg_SASL_init(PGconn *conn, int payloadlen) /* * Build a SASLInitialResponse message, and send it. */ - if (pqPutMsgStart('p', conn)) + if (pqPutMsgStart(PqMsg_SASLInitialResponse, conn)) goto error; if (pqPuts(selected_mechanism, conn)) goto error; diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index 837c5321aa..bf83a9b569 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -3591,7 +3591,9 @@ keep_going: /* We will come back to here until there is * Anything else probably means it's not Postgres on the other * end at all. */ - if (!(beresp == 'R' || beresp == 'v' || beresp == 'E')) + if (beresp != PqMsg_AuthenticationRequest && + beresp != PqMsg_ErrorResponse && + beresp != PqMsg_NegotiateProtocolVersion) { libpq_append_conn_error(conn, "expected authentication request from server, but received %c", beresp); @@ -3618,19 +3620,22 @@ keep_going: /* We will come back to here until there is * version 14, the server also used the old protocol for * errors that happened before processing the startup packet.) */ - if (beresp == 'R' && (msgLength < 8 || msgLength > 2000)) + if (beresp == PqMsg_AuthenticationRequest && + (msgLength < 8 || msgLength > 2000)) { libpq_append_conn_error(conn, "received invalid authentication request"); goto error_return; } - if (beresp == 'v' && (msgLength < 8 || msgLength > 2000)) + if (beresp == PqMsg_NegotiateProtocolVersion && + (msgLength < 8 || msgLength > 2000)) { libpq_append_conn_error(conn, "received invalid protocol negotiation message"); goto error_return; } #define MAX_ERRLEN 30000 - if (beresp == 'E' && (msgLength < 8 || msgLength > MAX_ERRLEN)) + if (beresp == PqMsg_ErrorResponse && + (msgLength < 8 || msgLength > MAX_ERRLEN)) { /* Handle error from a pre-3.0 server */ conn->inCursor = conn->inStart + 1; /* reread data */ @@ -3693,7 +3698,7 @@ keep_going: /* We will come back to here until there is } /* Handle errors. */ - if (beresp == 'E') + if (beresp == PqMsg_ErrorResponse) { if (pqGetErrorNotice3(conn, true)) { @@ -3770,7 +3775,7 @@ keep_going: /* We will come back to here until there is goto error_return; } - else if (beresp == 'v') + else if (beresp == PqMsg_NegotiateProtocolVersion) { if (pqGetNegotiateProtocolVersion3(conn)) { @@ -4540,7 +4545,7 @@ sendTerminateConn(PGconn *conn) * Try to send "close connection" message to backend. Ignore any * error. */ - pqPutMsgStart('X', conn); + pqPutMsgStart(PqMsg_Terminate, conn); pqPutMsgEnd(conn); (void) pqFlush(conn); } diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c index a868284ff8..fdb7994779 100644 --- a/src/interfaces/libpq/fe-exec.c +++ b/src/interfaces/libpq/fe-exec.c @@ -1458,7 +1458,7 @@ PQsendQueryInternal(PGconn *conn, const char *query, bool newQuery) /* Send the query message(s) */ /* construct the outgoing Query message */ - if (pqPutMsgStart('Q', conn) < 0 || + if (pqPutMsgStart(PqMsg_Query, conn) < 0 || pqPuts(query, conn) < 0 || pqPutMsgEnd(conn) < 0) { @@ -1571,7 +1571,7 @@ PQsendPrepare(PGconn *conn, return 0; /* error msg already set */ /* construct the Parse message */ - if (pqPutMsgStart('P', conn) < 0 || + if (pqPutMsgStart(PqMsg_Parse, conn) < 0 || pqPuts(stmtName, conn) < 0 || pqPuts(query, conn) < 0) goto sendFailed; @@ -1599,7 +1599,7 @@ PQsendPrepare(PGconn *conn, /* Add a Sync, unless in pipeline mode. */ if (conn->pipelineStatus == PQ_PIPELINE_OFF) { - if (pqPutMsgStart('S', conn) < 0 || + if (pqPutMsgStart(PqMsg_Sync, conn) < 0 || pqPutMsgEnd(conn) < 0) goto sendFailed; } @@ -1784,7 +1784,7 @@ PQsendQueryGuts(PGconn *conn, if (command) { /* construct the Parse message */ - if (pqPutMsgStart('P', conn) < 0 || + if (pqPutMsgStart(PqMsg_Parse, conn) < 0 || pqPuts(stmtName, conn) < 0 || pqPuts(command, conn) < 0) goto sendFailed; @@ -1808,7 +1808,7 @@ PQsendQueryGuts(PGconn *conn, } /* Construct the Bind message */ - if (pqPutMsgStart('B', conn) < 0 || + if (pqPutMsgStart(PqMsg_Bind, conn) < 0 || pqPuts("", conn) < 0 || pqPuts(stmtName, conn) < 0) goto sendFailed; @@ -1874,14 +1874,14 @@ PQsendQueryGuts(PGconn *conn, goto sendFailed; /* construct the Describe Portal message */ - if (pqPutMsgStart('D', conn) < 0 || + if (pqPutMsgStart(PqMsg_Describe, conn) < 0 || pqPutc('P', conn) < 0 || pqPuts("", conn) < 0 || pqPutMsgEnd(conn) < 0) goto sendFailed; /* construct the Execute message */ - if (pqPutMsgStart('E', conn) < 0 || + if (pqPutMsgStart(PqMsg_Execute, conn) < 0 || pqPuts("", conn) < 0 || pqPutInt(0, 4, conn) < 0 || pqPutMsgEnd(conn) < 0) @@ -1890,7 +1890,7 @@ PQsendQueryGuts(PGconn *conn, /* construct the Sync message if not in pipeline mode */ if (conn->pipelineStatus == PQ_PIPELINE_OFF) { - if (pqPutMsgStart('S', conn) < 0 || + if (pqPutMsgStart(PqMsg_Sync, conn) < 0 || pqPutMsgEnd(conn) < 0) goto sendFailed; } @@ -2422,7 +2422,7 @@ PQdescribePrepared(PGconn *conn, const char *stmt) { if (!PQexecStart(conn)) return NULL; - if (!PQsendTypedCommand(conn, 'D', 'S', stmt)) + if (!PQsendTypedCommand(conn, PqMsg_Describe, 'S', stmt)) return NULL; return PQexecFinish(conn); } @@ -2441,7 +2441,7 @@ PQdescribePortal(PGconn *conn, const char *portal) { if (!PQexecStart(conn)) return NULL; - if (!PQsendTypedCommand(conn, 'D', 'P', portal)) + if (!PQsendTypedCommand(conn, PqMsg_Describe, 'P', portal)) return NULL; return PQexecFinish(conn); } @@ -2456,7 +2456,7 @@ PQdescribePortal(PGconn *conn, const char *portal) int PQsendDescribePrepared(PGconn *conn, const char *stmt) { - return PQsendTypedCommand(conn, 'D', 'S', stmt); + return PQsendTypedCommand(conn, PqMsg_Describe, 'S', stmt); } /* @@ -2469,7 +2469,7 @@ PQsendDescribePrepared(PGconn *conn, const char *stmt) int PQsendDescribePortal(PGconn *conn, const char *portal) { - return PQsendTypedCommand(conn, 'D', 'P', portal); + return PQsendTypedCommand(conn, PqMsg_Describe, 'P', portal); } /* @@ -2488,7 +2488,7 @@ PQclosePrepared(PGconn *conn, const char *stmt) { if (!PQexecStart(conn)) return NULL; - if (!PQsendTypedCommand(conn, 'C', 'S', stmt)) + if (!PQsendTypedCommand(conn, PqMsg_Close, 'S', stmt)) return NULL; return PQexecFinish(conn); } @@ -2506,7 +2506,7 @@ PQclosePortal(PGconn *conn, const char *portal) { if (!PQexecStart(conn)) return NULL; - if (!PQsendTypedCommand(conn, 'C', 'P', portal)) + if (!PQsendTypedCommand(conn, PqMsg_Close, 'P', portal)) return NULL; return PQexecFinish(conn); } @@ -2521,7 +2521,7 @@ PQclosePortal(PGconn *conn, const char *portal) int PQsendClosePrepared(PGconn *conn, const char *stmt) { - return PQsendTypedCommand(conn, 'C', 'S', stmt); + return PQsendTypedCommand(conn, PqMsg_Close, 'S', stmt); } /* @@ -2534,7 +2534,7 @@ PQsendClosePrepared(PGconn *conn, const char *stmt) int PQsendClosePortal(PGconn *conn, const char *portal) { - return PQsendTypedCommand(conn, 'C', 'P', portal); + return PQsendTypedCommand(conn, PqMsg_Close, 'P', portal); } /* @@ -2577,17 +2577,17 @@ PQsendTypedCommand(PGconn *conn, char command, char type, const char *target) /* construct the Sync message */ if (conn->pipelineStatus == PQ_PIPELINE_OFF) { - if (pqPutMsgStart('S', conn) < 0 || + if (pqPutMsgStart(PqMsg_Sync, conn) < 0 || pqPutMsgEnd(conn) < 0) goto sendFailed; } /* remember if we are doing a Close or a Describe */ - if (command == 'C') + if (command == PqMsg_Close) { entry->queryclass = PGQUERY_CLOSE; } - else if (command == 'D') + else if (command == PqMsg_Describe) { entry->queryclass = PGQUERY_DESCRIBE; } @@ -2696,7 +2696,7 @@ PQputCopyData(PGconn *conn, const char *buffer, int nbytes) return pqIsnonblocking(conn) ? 0 : -1; } /* Send the data (too simple to delegate to fe-protocol files) */ - if (pqPutMsgStart('d', conn) < 0 || + if (pqPutMsgStart(PqMsg_CopyData, conn) < 0 || pqPutnchar(buffer, nbytes, conn) < 0 || pqPutMsgEnd(conn) < 0) return -1; @@ -2731,7 +2731,7 @@ PQputCopyEnd(PGconn *conn, const char *errormsg) if (errormsg) { /* Send COPY FAIL */ - if (pqPutMsgStart('f', conn) < 0 || + if (pqPutMsgStart(PqMsg_CopyFail, conn) < 0 || pqPuts(errormsg, conn) < 0 || pqPutMsgEnd(conn) < 0) return -1; @@ -2739,7 +2739,7 @@ PQputCopyEnd(PGconn *conn, const char *errormsg) else { /* Send COPY DONE */ - if (pqPutMsgStart('c', conn) < 0 || + if (pqPutMsgStart(PqMsg_CopyDone, conn) < 0 || pqPutMsgEnd(conn) < 0) return -1; } @@ -2751,7 +2751,7 @@ PQputCopyEnd(PGconn *conn, const char *errormsg) if (conn->cmd_queue_head && conn->cmd_queue_head->queryclass != PGQUERY_SIMPLE) { - if (pqPutMsgStart('S', conn) < 0 || + if (pqPutMsgStart(PqMsg_Sync, conn) < 0 || pqPutMsgEnd(conn) < 0) return -1; } @@ -3263,7 +3263,7 @@ PQpipelineSync(PGconn *conn) entry->query = NULL; /* construct the Sync message */ - if (pqPutMsgStart('S', conn) < 0 || + if (pqPutMsgStart(PqMsg_Sync, conn) < 0 || pqPutMsgEnd(conn) < 0) goto sendFailed; @@ -3311,7 +3311,7 @@ PQsendFlushRequest(PGconn *conn) return 0; } - if (pqPutMsgStart('H', conn) < 0 || + if (pqPutMsgStart(PqMsg_Flush, conn) < 0 || pqPutMsgEnd(conn) < 0) { return 0; diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c index 7bc6355d17..5613c56b14 100644 --- a/src/interfaces/libpq/fe-protocol3.c +++ b/src/interfaces/libpq/fe-protocol3.c @@ -34,8 +34,13 @@ * than a couple of kilobytes). */ #define VALID_LONG_MESSAGE_TYPE(id) \ - ((id) == 'T' || (id) == 'D' || (id) == 'd' || (id) == 'V' || \ - (id) == 'E' || (id) == 'N' || (id) == 'A') + ((id) == PqMsg_CopyData || \ + (id) == PqMsg_DataRow || \ + (id) == PqMsg_ErrorResponse || \ + (id) == PqMsg_FunctionCallResponse || \ + (id) == PqMsg_NoticeResponse || \ + (id) == PqMsg_NotificationResponse || \ + (id) == PqMsg_RowDescription) static void handleSyncLoss(PGconn *conn, char id, int msgLength); @@ -140,12 +145,12 @@ pqParseInput3(PGconn *conn) * from config file due to SIGHUP), but otherwise we hold off until * BUSY state. */ - if (id == 'A') + if (id == PqMsg_NotificationResponse) { if (getNotify(conn)) return; } - else if (id == 'N') + else if (id == PqMsg_NoticeResponse) { if (pqGetErrorNotice3(conn, false)) return; @@ -165,12 +170,12 @@ pqParseInput3(PGconn *conn) * it is about to close the connection, so we don't want to just * discard it...) */ - if (id == 'E') + if (id == PqMsg_ErrorResponse) { if (pqGetErrorNotice3(conn, false /* treat as notice */ )) return; } - else if (id == 'S') + else if (id == PqMsg_ParameterStatus) { if (getParameterStatus(conn)) return; @@ -192,7 +197,7 @@ pqParseInput3(PGconn *conn) */ switch (id) { - case 'C': /* command complete */ + case PqMsg_CommandComplete: if (pqGets(&conn->workBuffer, conn)) return; if (!pgHavePendingResult(conn)) @@ -210,13 +215,12 @@ pqParseInput3(PGconn *conn) CMDSTATUS_LEN); conn->asyncStatus = PGASYNC_READY; break; - case 'E': /* error return */ + case PqMsg_ErrorResponse: if (pqGetErrorNotice3(conn, true)) return; conn->asyncStatus = PGASYNC_READY; break; - case 'Z': /* sync response, backend is ready for new - * query */ + case PqMsg_ReadyForQuery: if (getReadyForQuery(conn)) return; if (conn->pipelineStatus != PQ_PIPELINE_OFF) @@ -246,7 +250,7 @@ pqParseInput3(PGconn *conn) conn->asyncStatus = PGASYNC_IDLE; } break; - case 'I': /* empty query */ + case PqMsg_EmptyQueryResponse: if (!pgHavePendingResult(conn)) { conn->result = PQmakeEmptyPGresult(conn, @@ -259,7 +263,7 @@ pqParseInput3(PGconn *conn) } conn->asyncStatus = PGASYNC_READY; break; - case '1': /* Parse Complete */ + case PqMsg_ParseComplete: /* If we're doing PQprepare, we're done; else ignore */ if (conn->cmd_queue_head && conn->cmd_queue_head->queryclass == PGQUERY_PREPARE) @@ -277,10 +281,10 @@ pqParseInput3(PGconn *conn) conn->asyncStatus = PGASYNC_READY; } break; - case '2': /* Bind Complete */ + case PqMsg_BindComplete: /* Nothing to do for this message type */ break; - case '3': /* Close Complete */ + case PqMsg_CloseComplete: /* If we're doing PQsendClose, we're done; else ignore */ if (conn->cmd_queue_head && conn->cmd_queue_head->queryclass == PGQUERY_CLOSE) @@ -298,11 +302,11 @@ pqParseInput3(PGconn *conn) conn->asyncStatus = PGASYNC_READY; } break; - case 'S': /* parameter status */ + case PqMsg_ParameterStatus: if (getParameterStatus(conn)) return; break; - case 'K': /* secret key data from the backend */ + case PqMsg_BackendKeyData: /* * This is expected only during backend startup, but it's @@ -314,7 +318,7 @@ pqParseInput3(PGconn *conn) if (pqGetInt(&(conn->be_key), 4, conn)) return; break; - case 'T': /* Row Description */ + case PqMsg_RowDescription: if (conn->error_result || (conn->result != NULL && conn->result->resultStatus == PGRES_FATAL_ERROR)) @@ -346,7 +350,7 @@ pqParseInput3(PGconn *conn) return; } break; - case 'n': /* No Data */ + case PqMsg_NoData: /* * NoData indicates that we will not be seeing a @@ -374,11 +378,11 @@ pqParseInput3(PGconn *conn) conn->asyncStatus = PGASYNC_READY; } break; - case 't': /* Parameter Description */ + case PqMsg_ParameterDescription: if (getParamDescriptions(conn, msgLength)) return; break; - case 'D': /* Data Row */ + case PqMsg_DataRow: if (conn->result != NULL && conn->result->resultStatus == PGRES_TUPLES_OK) { @@ -405,24 +409,24 @@ pqParseInput3(PGconn *conn) conn->inCursor += msgLength; } break; - case 'G': /* Start Copy In */ + case PqMsg_CopyInResponse: if (getCopyStart(conn, PGRES_COPY_IN)) return; conn->asyncStatus = PGASYNC_COPY_IN; break; - case 'H': /* Start Copy Out */ + case PqMsg_CopyOutResponse: if (getCopyStart(conn, PGRES_COPY_OUT)) return; conn->asyncStatus = PGASYNC_COPY_OUT; conn->copy_already_done = 0; break; - case 'W': /* Start Copy Both */ + case PqMsg_CopyBothResponse: if (getCopyStart(conn, PGRES_COPY_BOTH)) return; conn->asyncStatus = PGASYNC_COPY_BOTH; conn->copy_already_done = 0; break; - case 'd': /* Copy Data */ + case PqMsg_CopyData: /* * If we see Copy Data, just silently drop it. This would @@ -431,7 +435,7 @@ pqParseInput3(PGconn *conn) */ conn->inCursor += msgLength; break; - case 'c': /* Copy Done */ + case PqMsg_CopyDone: /* * If we see Copy Done, just silently drop it. This is @@ -1692,21 +1696,21 @@ getCopyDataMessage(PGconn *conn) */ switch (id) { - case 'A': /* NOTIFY */ + case PqMsg_NotificationResponse: if (getNotify(conn)) return 0; break; - case 'N': /* NOTICE */ + case PqMsg_NoticeResponse: if (pqGetErrorNotice3(conn, false)) return 0; break; - case 'S': /* ParameterStatus */ + case PqMsg_ParameterStatus: if (getParameterStatus(conn)) return 0; break; - case 'd': /* Copy Data, pass it back to caller */ + case PqMsg_CopyData: return msgLength; - case 'c': + case PqMsg_CopyDone: /* * If this is a CopyDone message, exit COPY_OUT mode and let @@ -1929,7 +1933,7 @@ pqEndcopy3(PGconn *conn) if (conn->asyncStatus == PGASYNC_COPY_IN || conn->asyncStatus == PGASYNC_COPY_BOTH) { - if (pqPutMsgStart('c', conn) < 0 || + if (pqPutMsgStart(PqMsg_CopyDone, conn) < 0 || pqPutMsgEnd(conn) < 0) return 1; @@ -1940,7 +1944,7 @@ pqEndcopy3(PGconn *conn) if (conn->cmd_queue_head && conn->cmd_queue_head->queryclass != PGQUERY_SIMPLE) { - if (pqPutMsgStart('S', conn) < 0 || + if (pqPutMsgStart(PqMsg_Sync, conn) < 0 || pqPutMsgEnd(conn) < 0) return 1; } @@ -2023,7 +2027,7 @@ pqFunctionCall3(PGconn *conn, Oid fnid, /* PQfn already validated connection state */ - if (pqPutMsgStart('F', conn) < 0 || /* function call msg */ + if (pqPutMsgStart(PqMsg_FunctionCall, conn) < 0 || pqPutInt(fnid, 4, conn) < 0 || /* function id */ pqPutInt(1, 2, conn) < 0 || /* # of format codes */ pqPutInt(1, 2, conn) < 0 || /* format code: BINARY */ diff --git a/src/interfaces/libpq/fe-trace.c b/src/interfaces/libpq/fe-trace.c index 402784f40e..b18e3deab6 100644 --- a/src/interfaces/libpq/fe-trace.c +++ b/src/interfaces/libpq/fe-trace.c @@ -562,110 +562,120 @@ pqTraceOutputMessage(PGconn *conn, const char *message, bool toServer) switch (id) { - case '1': + case PqMsg_ParseComplete: fprintf(conn->Pfdebug, "ParseComplete"); /* No message content */ break; - case '2': + case PqMsg_BindComplete: fprintf(conn->Pfdebug, "BindComplete"); /* No message content */ break; - case '3': + case PqMsg_CloseComplete: fprintf(conn->Pfdebug, "CloseComplete"); /* No message content */ break; - case 'A': /* Notification Response */ + case PqMsg_NotificationResponse: pqTraceOutputA(conn->Pfdebug, message, &logCursor, regress); break; - case 'B': /* Bind */ + case PqMsg_Bind: pqTraceOutputB(conn->Pfdebug, message, &logCursor); break; - case 'c': + case PqMsg_CopyDone: fprintf(conn->Pfdebug, "CopyDone"); /* No message content */ break; - case 'C': /* Close(F) or Command Complete(B) */ + case PqMsg_CommandComplete: + /* Close(F) and CommandComplete(B) use the same identifier. */ + Assert(PqMsg_Close == PqMsg_CommandComplete); pqTraceOutputC(conn->Pfdebug, toServer, message, &logCursor); break; - case 'd': /* Copy Data */ + case PqMsg_CopyData: /* Drop COPY data to reduce the overhead of logging. */ break; - case 'D': /* Describe(F) or Data Row(B) */ + case PqMsg_Describe: + /* Describe(F) and DataRow(B) use the same identifier. */ + Assert(PqMsg_Describe == PqMsg_DataRow); pqTraceOutputD(conn->Pfdebug, toServer, message, &logCursor); break; - case 'E': /* Execute(F) or Error Response(B) */ + case PqMsg_Execute: + /* Execute(F) and ErrorResponse(B) use the same identifier. */ + Assert(PqMsg_Execute == PqMsg_ErrorResponse); pqTraceOutputE(conn->Pfdebug, toServer, message, &logCursor, regress); break; - case 'f': /* Copy Fail */ + case PqMsg_CopyFail: pqTraceOutputf(conn->Pfdebug, message, &logCursor); break; - case 'F': /* Function Call */ + case PqMsg_FunctionCall: pqTraceOutputF(conn->Pfdebug, message, &logCursor, regress); break; - case 'G': /* Start Copy In */ + case PqMsg_CopyInResponse: pqTraceOutputG(conn->Pfdebug, message, &logCursor); break; - case 'H': /* Flush(F) or Start Copy Out(B) */ + case PqMsg_Flush: + /* Flush(F) and CopyOutResponse(B) use the same identifier */ + Assert(PqMsg_CopyOutResponse == PqMsg_Flush); if (!toServer) pqTraceOutputH(conn->Pfdebug, message, &logCursor); else fprintf(conn->Pfdebug, "Flush"); /* no message content */ break; - case 'I': + case PqMsg_EmptyQueryResponse: fprintf(conn->Pfdebug, "EmptyQueryResponse"); /* No message content */ break; - case 'K': /* secret key data from the backend */ + case PqMsg_BackendKeyData: pqTraceOutputK(conn->Pfdebug, message, &logCursor, regress); break; - case 'n': + case PqMsg_NoData: fprintf(conn->Pfdebug, "NoData"); /* No message content */ break; - case 'N': + case PqMsg_NoticeResponse: pqTraceOutputNR(conn->Pfdebug, "NoticeResponse", message, &logCursor, regress); break; - case 'P': /* Parse */ + case PqMsg_Parse: pqTraceOutputP(conn->Pfdebug, message, &logCursor, regress); break; - case 'Q': /* Query */ + case PqMsg_Query: pqTraceOutputQ(conn->Pfdebug, message, &logCursor); break; - case 'R': /* Authentication */ + case PqMsg_AuthenticationRequest: pqTraceOutputR(conn->Pfdebug, message, &logCursor); break; - case 's': + case PqMsg_PortalSuspended: fprintf(conn->Pfdebug, "PortalSuspended"); /* No message content */ break; - case 'S': /* Parameter Status(B) or Sync(F) */ + case PqMsg_Sync: + /* Parameter Status(B) and Sync(F) use the same identifier */ + Assert(PqMsg_ParameterStatus == PqMsg_Sync); if (!toServer) pqTraceOutputS(conn->Pfdebug, message, &logCursor); else fprintf(conn->Pfdebug, "Sync"); /* no message content */ break; - case 't': /* Parameter Description */ + case PqMsg_ParameterDescription: pqTraceOutputt(conn->Pfdebug, message, &logCursor, regress); break; - case 'T': /* Row Description */ + case PqMsg_RowDescription: pqTraceOutputT(conn->Pfdebug, message, &logCursor, regress); break; - case 'v': /* Negotiate Protocol Version */ + case PqMsg_NegotiateProtocolVersion: pqTraceOutputv(conn->Pfdebug, message, &logCursor); break; - case 'V': /* Function Call response */ + case PqMsg_FunctionCallResponse: pqTraceOutputV(conn->Pfdebug, message, &logCursor); break; - case 'W': /* Start Copy Both */ + case PqMsg_CopyBothResponse: pqTraceOutputW(conn->Pfdebug, message, &logCursor, length); break; - case 'X': + case PqMsg_Terminate: fprintf(conn->Pfdebug, "Terminate"); /* No message content */ break; - case 'Z': /* Ready For Query */ + case PqMsg_ReadyForQuery: pqTraceOutputZ(conn->Pfdebug, message, &logCursor); break; default: -- 2.25.1 --GvXjxJ+pjyke8COw-- ^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2023-08-16 19:08 UTC | newest] Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-03-15 05:13 Re: Should vacuum process config file reload more often Masahiko Sawada <[email protected]> 2023-03-18 22:47 ` Melanie Plageman <[email protected]> 2023-03-19 16:48 ` Melanie Plageman <[email protected]> 2023-03-23 06:08 ` Masahiko Sawada <[email protected]> 2023-03-23 16:24 ` Daniel Gustafsson <[email protected]> 2023-03-24 00:27 ` Melanie Plageman <[email protected]> 2023-03-24 05:21 ` Masahiko Sawada <[email protected]> 2023-03-24 17:27 ` Melanie Plageman <[email protected]> 2023-03-25 19:03 ` Melanie Plageman <[email protected]> 2023-03-27 18:12 ` Melanie Plageman <[email protected]> 2023-08-16 19:08 [PATCH v6 1/1] Introduce macros for protocol characters. Nathan Bossart <[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