public inbox for [email protected]help / color / mirror / Atom feed
Should vacuum process config file reload more often 18+ messages / 7 participants [nested] [flat]
* Should vacuum process config file reload more often @ 2023-02-23 22:08 Melanie Plageman <[email protected]> 0 siblings, 2 replies; 18+ messages in thread From: Melanie Plageman @ 2023-02-23 22:08 UTC (permalink / raw) To: pgsql-hackers; +Cc: Andres Freund <[email protected]> Hi, Users may wish to 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). This has been brought up for autovacuum in [1]. Andres suggested that it might be possible to check ConfigReloadPending in vacuum_delay_point(), so I thought I would draft a rough patch and start a discussion. Since vacuum_delay_point() is also called by analyze and we do not want to reload the configuration file if we are in a user transaction, I widened the scope of the in_outer_xact variable in vacuum() and allowed analyze in a user transaction to default to the current configuration file reload cadence in PostgresMain(). I don't think I can set and leave vac_in_outer_xact the way I am doing it in this patch, since I use vac_in_outer_xact in vacuum_delay_point(), which I believe is reachable from codepaths that would not have called vacuum(). It seems that if a backend sets it, the outer transaction commits, and then the backend ends up calling vacuum_delay_point() in a different way later, it wouldn't be quite right. Apart from this, one higher level question I have is if there are other gucs whose modification would make reloading the configuration file during vacuum/analyze unsafe. - Melanie [1] https://www.postgresql.org/message-id/flat/22CA91B4-D341-4075-BD3C-4BAB52AF1E80%40amazon.com#37f05e3... Attachments: [text/x-patch] v1-0001-reload-config-file-vac.patch (2.7K, ../../CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7+DXhrjDA0gw@mail.gmail.com/2-v1-0001-reload-config-file-vac.patch) download | inline diff: From aea6fbfd93ab12e4e27869b755367ab8454e3eef Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Thu, 23 Feb 2023 15:54:55 -0500 Subject: [PATCH v1] reload config file vac --- src/backend/commands/vacuum.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index aa79d9de4d..979d19222d 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/proc.h" @@ -75,6 +76,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 vac_in_outer_xact = false; /* @@ -309,8 +311,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); @@ -327,10 +328,10 @@ vacuum(List *relations, VacuumParams *params, if (params->options & VACOPT_VACUUM) { PreventInTransactionBlock(isTopLevel, stmttype); - in_outer_xact = false; + vac_in_outer_xact = false; } else - in_outer_xact = IsInTransactionBlock(isTopLevel); + vac_in_outer_xact = IsInTransactionBlock(isTopLevel); /* * Due to static variables vac_context, anl_context and vac_strategy, @@ -451,7 +452,7 @@ vacuum(List *relations, VacuumParams *params, Assert(params->options & VACOPT_ANALYZE); if (IsAutoVacuumWorkerProcess()) use_own_xacts = true; - else if (in_outer_xact) + else if (vac_in_outer_xact) use_own_xacts = false; else if (list_length(relations) > 1) use_own_xacts = true; @@ -469,7 +470,7 @@ vacuum(List *relations, VacuumParams *params, */ if (use_own_xacts) { - Assert(!in_outer_xact); + Assert(!vac_in_outer_xact); /* ActiveSnapshot is not set by autovacuum */ if (ActiveSnapshotSet()) @@ -521,7 +522,7 @@ vacuum(List *relations, VacuumParams *params, } analyze_rel(vrel->oid, vrel->relation, params, - vrel->va_cols, in_outer_xact, vac_strategy); + vrel->va_cols, vac_in_outer_xact, vac_strategy); if (use_own_xacts) { @@ -2214,6 +2215,12 @@ vacuum_delay_point(void) WAIT_EVENT_VACUUM_DELAY); ResetLatch(MyLatch); + if (ConfigReloadPending && !vac_in_outer_xact) + { + ConfigReloadPending = false; + ProcessConfigFile(PGC_SIGHUP); + } + VacuumCostBalance = 0; /* update balance values for workers */ -- 2.37.2 ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-02-24 08:42 Pavel Borisov <[email protected]> parent: Melanie Plageman <[email protected]> 1 sibling, 1 reply; 18+ messages in thread From: Pavel Borisov @ 2023-02-24 08:42 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: pgsql-hackers; Andres Freund <[email protected]> Hi, Melanie! On Fri, 24 Feb 2023 at 02:08, Melanie Plageman <[email protected]> wrote: > > Hi, > > Users may wish to 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). This has been brought up for > autovacuum in [1]. > > Andres suggested that it might be possible to check ConfigReloadPending > in vacuum_delay_point(), so I thought I would draft a rough patch and > start a discussion. > > Since vacuum_delay_point() is also called by analyze and we do not want > to reload the configuration file if we are in a user transaction, I > widened the scope of the in_outer_xact variable in vacuum() and allowed > analyze in a user transaction to default to the current configuration > file reload cadence in PostgresMain(). > > I don't think I can set and leave vac_in_outer_xact the way I am doing > it in this patch, since I use vac_in_outer_xact in vacuum_delay_point(), > which I believe is reachable from codepaths that would not have called > vacuum(). It seems that if a backend sets it, the outer transaction > commits, and then the backend ends up calling vacuum_delay_point() in a > different way later, it wouldn't be quite right. > > Apart from this, one higher level question I have is if there are other > gucs whose modification would make reloading the configuration file > during vacuum/analyze unsafe. I have a couple of small questions: Can this patch also read the current GUC value if it's modified by the SET command, without editing config file? What will be if we modify config file with mistakes? (When we try to start the cluster with an erroneous config file it will fail to start, not sure about re-read config) Overall the proposal seems legit and useful. Kind regards, Pavel Borisov ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-02-27 14:11 Masahiko Sawada <[email protected]> parent: Melanie Plageman <[email protected]> 1 sibling, 1 reply; 18+ messages in thread From: Masahiko Sawada @ 2023-02-27 14:11 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: pgsql-hackers; Andres Freund <[email protected]> Hi, On Fri, Feb 24, 2023 at 7:08 AM Melanie Plageman <[email protected]> wrote: > > Hi, > > Users may wish to 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). This has been brought up for > autovacuum in [1]. > > Andres suggested that it might be possible to check ConfigReloadPending > in vacuum_delay_point(), so I thought I would draft a rough patch and > start a discussion. In vacuum_delay_point(), we need to update VacuumCostActive too if necessary. > Apart from this, one higher level question I have is if there are other > gucs whose modification would make reloading the configuration file > during vacuum/analyze unsafe. As far as I know there are not such GUC parameters in the core but there might be in third-party table AM and index AM extensions. Also, I'm concerned that allowing to change any GUC parameters during vacuum/analyze could be a foot-gun in the future. When modifying vacuum/analyze-related codes, we have to consider the case where any GUC parameters could be changed during vacuum/analyze. I guess it would be better to apply the parameter changes for only vacuum delay related parameters. For example, autovacuum launcher advertises the values of the vacuum delay parameters on the shared memory not only for autovacuum processes but also for manual vacuum/analyze processes. Both processes can update them accordingly in vacuum_delay_point(). Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-03-01 19:54 Melanie Plageman <[email protected]> parent: Pavel Borisov <[email protected]> 0 siblings, 0 replies; 18+ messages in thread From: Melanie Plageman @ 2023-03-01 19:54 UTC (permalink / raw) To: Pavel Borisov <[email protected]>; +Cc: pgsql-hackers; Andres Freund <[email protected]> Thanks for the feedback and questions, Pavel! On Fri, Feb 24, 2023 at 3:43 AM Pavel Borisov <[email protected]> wrote: > I have a couple of small questions: > Can this patch also read the current GUC value if it's modified by the > SET command, without editing config file? If a user sets a guc like vacuum_cost_limit with SET, this only modifies the value for that session. That wouldn't affect the in-progress vacuum you initiated from that session because you would have to wait for the vacuum to complete before issuing the SET command. > What will be if we modify config file with mistakes? (When we try to > start the cluster with an erroneous config file it will fail to start, > not sure about re-read config) If you manually add an invalid valid to your postgresql.conf, when it is reloaded, the existing value will remain unchanged and an error will be logged. If you attempt to change the guc value to an invalid value with ALTER SYSTEM, the ALTER SYSTEM command will fail and the existing value will remain unchanged. - Melanie ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-03-02 01:41 Melanie Plageman <[email protected]> parent: Masahiko Sawada <[email protected]> 0 siblings, 1 reply; 18+ messages in thread From: Melanie Plageman @ 2023-03-02 01:41 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: pgsql-hackers; Andres Freund <[email protected]> On Mon, Feb 27, 2023 at 9:12 AM Masahiko Sawada <[email protected]> wrote: > On Fri, Feb 24, 2023 at 7:08 AM Melanie Plageman > <[email protected]> wrote: > > Users may wish to 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). This has been brought up for > > autovacuum in [1]. > > > > Andres suggested that it might be possible to check ConfigReloadPending > > in vacuum_delay_point(), so I thought I would draft a rough patch and > > start a discussion. > > In vacuum_delay_point(), we need to update VacuumCostActive too if necessary. Yes, good point. Thank you! On Thu, Feb 23, 2023 at 5:08 PM Melanie Plageman <[email protected]> wrote: > I don't think I can set and leave vac_in_outer_xact the way I am doing > it in this patch, since I use vac_in_outer_xact in vacuum_delay_point(), > which I believe is reachable from codepaths that would not have called > vacuum(). It seems that if a backend sets it, the outer transaction > commits, and then the backend ends up calling vacuum_delay_point() in a > different way later, it wouldn't be quite right. Perhaps I could just set in_outer_xact to false in the PG_FINALLY() section in vacuum() to avoid this problem. On Wed, Mar 1, 2023 at 7:15 PM Andres Freund <[email protected]> wrote: > On 2023-02-28 11:16:45 +0900, Masahiko Sawada wrote: > > On Tue, Feb 28, 2023 at 10:21 AM Andres Freund <[email protected]> wrote: > > > On 2023-02-27 23:11:53 +0900, Masahiko Sawada wrote: > > > > Also, I'm concerned that allowing to change any GUC parameters during > > > > vacuum/analyze could be a foot-gun in the future. When modifying > > > > vacuum/analyze-related codes, we have to consider the case where any GUC > > > > parameters could be changed during vacuum/analyze. > > > > > > What kind of scenario are you thinking of? > > > > For example, I guess we will need to take care of changes of > > maintenance_work_mem. Currently we initialize the dead tuple space at > > the beginning of lazy vacuum, but perhaps we would need to > > enlarge/shrink it based on the new value? > > I don't think we need to do anything about that initially, just because the > config can be changed in a more granular way, doesn't mean we have to react to > every change for the current operation. Perhaps we can mention in the docs that a change to maintenance_work_mem will not take effect in the middle of vacuuming a table. But, Ithink it probably isn't needed. On another topic, I've just realized that when autovacuuming we only update tab->at_vacuum_cost_delay/limit from autovacuum_vacuum_cost_delay/limit for each table (in table_recheck_autovac()) and then use that to update MyWorkerInfo->wi_cost_delay/limit. MyWorkerInfo->wi_cost_delay/limit is what is used to update VacuumCostDelay/Limit in AutoVacuumUpdateDelay(). So, even if we reload the config file in vacuum_delay_point(), if we don't use the new value of autovacuum_vacuum_cost_delay/limit it will have no effect for autovacuum. I started writing a little helper that could be used to update these workerinfo->wi_cost_delay/limit in vacuum_delay_point(), but I notice when they are first set, we consider the autovacuum table options. So, I suppose I would need to consider these when updating wi_cost_delay/limit later as well? (during vacuum_delay_point() or in AutoVacuumUpdateDelay()) I wasn't quite sure because I found these chained ternaries rather difficult to interpret, but I think table_recheck_autovac() is saying that the autovacuum table options override all other values for vac_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; i.e. this? if (avopts && avopts->vacuum_cost_delay >= 0) vac_cost_delay = avopts->vacuum_cost_delay; else if (autovacuum_vac_cost_delay >= 0) vac_cost_delay = autovacuum_vacuum_cost_delay; else vac_cost_delay = VacuumCostDelay - Melanie ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-03-02 07:36 Masahiko Sawada <[email protected]> parent: Melanie Plageman <[email protected]> 0 siblings, 2 replies; 18+ messages in thread From: Masahiko Sawada @ 2023-03-02 07:36 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: pgsql-hackers; Andres Freund <[email protected]> On Thu, Mar 2, 2023 at 10:41 AM Melanie Plageman <[email protected]> wrote: > > On Mon, Feb 27, 2023 at 9:12 AM Masahiko Sawada <[email protected]> wrote: > > On Fri, Feb 24, 2023 at 7:08 AM Melanie Plageman > > <[email protected]> wrote: > > > Users may wish to 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). This has been brought up for > > > autovacuum in [1]. > > > > > > Andres suggested that it might be possible to check ConfigReloadPending > > > in vacuum_delay_point(), so I thought I would draft a rough patch and > > > start a discussion. > > > > In vacuum_delay_point(), we need to update VacuumCostActive too if necessary. > > Yes, good point. Thank you! > > On Thu, Feb 23, 2023 at 5:08 PM Melanie Plageman > <[email protected]> wrote: > > I don't think I can set and leave vac_in_outer_xact the way I am doing > > it in this patch, since I use vac_in_outer_xact in vacuum_delay_point(), > > which I believe is reachable from codepaths that would not have called > > vacuum(). It seems that if a backend sets it, the outer transaction > > commits, and then the backend ends up calling vacuum_delay_point() in a > > different way later, it wouldn't be quite right. > > Perhaps I could just set in_outer_xact to false in the PG_FINALLY() > section in vacuum() to avoid this problem. > > On Wed, Mar 1, 2023 at 7:15 PM Andres Freund <[email protected]> wrote: > > On 2023-02-28 11:16:45 +0900, Masahiko Sawada wrote: > > > On Tue, Feb 28, 2023 at 10:21 AM Andres Freund <[email protected]> wrote: > > > > On 2023-02-27 23:11:53 +0900, Masahiko Sawada wrote: > > > > > Also, I'm concerned that allowing to change any GUC parameters during > > > > > vacuum/analyze could be a foot-gun in the future. When modifying > > > > > vacuum/analyze-related codes, we have to consider the case where any GUC > > > > > parameters could be changed during vacuum/analyze. > > > > > > > > What kind of scenario are you thinking of? > > > > > > For example, I guess we will need to take care of changes of > > > maintenance_work_mem. Currently we initialize the dead tuple space at > > > the beginning of lazy vacuum, but perhaps we would need to > > > enlarge/shrink it based on the new value? > > > > I don't think we need to do anything about that initially, just because the > > config can be changed in a more granular way, doesn't mean we have to react to > > every change for the current operation. > > Perhaps we can mention in the docs that a change to maintenance_work_mem > will not take effect in the middle of vacuuming a table. But, Ithink it probably > isn't needed. Agreed. > > On another topic, I've just realized that when autovacuuming we only > update tab->at_vacuum_cost_delay/limit from > autovacuum_vacuum_cost_delay/limit for each table (in > table_recheck_autovac()) and then use that to update > MyWorkerInfo->wi_cost_delay/limit. MyWorkerInfo->wi_cost_delay/limit is > what is used to update VacuumCostDelay/Limit in AutoVacuumUpdateDelay(). > So, even if we reload the config file in vacuum_delay_point(), if we > don't use the new value of autovacuum_vacuum_cost_delay/limit it will > have no effect for autovacuum. Right, but IIUC wi_cost_limit (and VacuumCostDelayLimit) might be updated. After the autovacuum launcher reloads the config file, it calls autovac_balance_cost() that updates that value of active workers. I'm not sure why we don't update workers' wi_cost_delay, though. > I started writing a little helper that could be used to update these > workerinfo->wi_cost_delay/limit in vacuum_delay_point(), Since we set vacuum delay parameters for autovacuum workers so that we ration out I/O equally, I think we should keep the current mechanism that the autovacuum launcher sets workers' delay parameters and they update accordingly. > but I notice > when they are first set, we consider the autovacuum table options. So, > I suppose I would need to consider these when updating > wi_cost_delay/limit later as well? (during vacuum_delay_point() or > in AutoVacuumUpdateDelay()) > > I wasn't quite sure because I found these chained ternaries rather > difficult to interpret, but I think table_recheck_autovac() is saying > that the autovacuum table options override all other values for > vac_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; > > i.e. this? > > if (avopts && avopts->vacuum_cost_delay >= 0) > vac_cost_delay = avopts->vacuum_cost_delay; > else if (autovacuum_vac_cost_delay >= 0) > vac_cost_delay = autovacuum_vacuum_cost_delay; > else > vac_cost_delay = VacuumCostDelay Yes, if the table has autovacuum table options, we use these values and the table is excluded from the balancing algorithm I mentioned above. See the code from table_recheck_autovac(), /* * If any of the cost delay parameters has been set individually for * this table, disable the balancing algorithm. */ tab->at_dobalance = !(avopts && (avopts->vacuum_cost_limit > 0 || avopts->vacuum_cost_delay > 0)); So if the table has autovacuum table options, the vacuum delay parameters probably should be updated by ALTER TABLE, not by reloading the config file. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-03-02 23:37 Melanie Plageman <[email protected]> parent: Masahiko Sawada <[email protected]> 1 sibling, 1 reply; 18+ messages in thread From: Melanie Plageman @ 2023-03-02 23:37 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: pgsql-hackers; Andres Freund <[email protected]>; Amit Kapila <[email protected]> On Thu, Mar 2, 2023 at 2:36 AM Masahiko Sawada <[email protected]> wrote: > > On Thu, Mar 2, 2023 at 10:41 AM Melanie Plageman > <[email protected]> wrote: > > On another topic, I've just realized that when autovacuuming we only > > update tab->at_vacuum_cost_delay/limit from > > autovacuum_vacuum_cost_delay/limit for each table (in > > table_recheck_autovac()) and then use that to update > > MyWorkerInfo->wi_cost_delay/limit. MyWorkerInfo->wi_cost_delay/limit is > > what is used to update VacuumCostDelay/Limit in AutoVacuumUpdateDelay(). > > So, even if we reload the config file in vacuum_delay_point(), if we > > don't use the new value of autovacuum_vacuum_cost_delay/limit it will > > have no effect for autovacuum. > > Right, but IIUC wi_cost_limit (and VacuumCostDelayLimit) might be > updated. After the autovacuum launcher reloads the config file, it > calls autovac_balance_cost() that updates that value of active > workers. I'm not sure why we don't update workers' wi_cost_delay, > though. Ah yes, I didn't realize this. Thanks. I went back and did more code reading/analysis, and I see no reason why we shouldn't update worker->wi_cost_delay to the new value of autovacuum_vac_cost_delay in autovac_balance_cost(). Then, as you said, the autovac launcher will call autovac_balance_cost() when it reloads the configuration file. Then, the next time the autovac worker calls AutoVacuumUpdateDelay(), it will update VacuumCostDelay. > > I started writing a little helper that could be used to update these > > workerinfo->wi_cost_delay/limit in vacuum_delay_point(), > > Since we set vacuum delay parameters for autovacuum workers so that we > ration out I/O equally, I think we should keep the current mechanism > that the autovacuum launcher sets workers' delay parameters and they > update accordingly. Yes, agreed, it should go in the same place as where we update wi_cost_limit (autovac_balance_cost()). I think we should potentially rename autovac_balance_cost() because its name and all its comments point to its only purpose being to balance the total of the workers wi_cost_limits to no more than autovacuum_vacuum_cost_limit. And the autovacuum_vacuum_cost_delay doesn't need to be balanced in this way. Though, since this change on its own would make autovacuum pick up new values of autovacuum_vacuum_cost_limit (without having the worker reload the config file), I wonder if it makes sense to try and have vacuum_delay_point() only reload the config file if it is an explicit vacuum or an analyze not being run in an outer transaction (to avoid overhead of reloading config file)? The lifecycle of this different vacuum delay-related gucs and how it differs between autovacuum workers and explicit vacuum is quite tangled already, though. > > but I notice > > when they are first set, we consider the autovacuum table options. So, > > I suppose I would need to consider these when updating > > wi_cost_delay/limit later as well? (during vacuum_delay_point() or > > in AutoVacuumUpdateDelay()) > > > > I wasn't quite sure because I found these chained ternaries rather > > difficult to interpret, but I think table_recheck_autovac() is saying > > that the autovacuum table options override all other values for > > vac_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; > > > > i.e. this? > > > > if (avopts && avopts->vacuum_cost_delay >= 0) > > vac_cost_delay = avopts->vacuum_cost_delay; > > else if (autovacuum_vac_cost_delay >= 0) > > vac_cost_delay = autovacuum_vacuum_cost_delay; > > else > > vac_cost_delay = VacuumCostDelay > > Yes, if the table has autovacuum table options, we use these values > and the table is excluded from the balancing algorithm I mentioned > above. See the code from table_recheck_autovac(), > > /* > * If any of the cost delay parameters has been set individually for > * this table, disable the balancing algorithm. > */ > tab->at_dobalance = > !(avopts && (avopts->vacuum_cost_limit > 0 || > avopts->vacuum_cost_delay > 0)); > > So if the table has autovacuum table options, the vacuum delay > parameters probably should be updated by ALTER TABLE, not by reloading > the config file. Yes, if the table has autovacuum table options, I think the user is out-of-luck until the relation is done being vacuumed because the ALTER TABLE will need to get a lock. - Melanie ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-03-05 20:26 Melanie Plageman <[email protected]> parent: Melanie Plageman <[email protected]> 0 siblings, 1 reply; 18+ messages in thread From: Melanie Plageman @ 2023-03-05 20:26 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: pgsql-hackers; Andres Freund <[email protected]>; Amit Kapila <[email protected]> On Thu, Mar 2, 2023 at 6:37 PM Melanie Plageman <[email protected]> wrote: > > On Thu, Mar 2, 2023 at 2:36 AM Masahiko Sawada <[email protected]> wrote: > > > > On Thu, Mar 2, 2023 at 10:41 AM Melanie Plageman > > <[email protected]> wrote: > > > On another topic, I've just realized that when autovacuuming we only > > > update tab->at_vacuum_cost_delay/limit from > > > autovacuum_vacuum_cost_delay/limit for each table (in > > > table_recheck_autovac()) and then use that to update > > > MyWorkerInfo->wi_cost_delay/limit. MyWorkerInfo->wi_cost_delay/limit is > > > what is used to update VacuumCostDelay/Limit in AutoVacuumUpdateDelay(). > > > So, even if we reload the config file in vacuum_delay_point(), if we > > > don't use the new value of autovacuum_vacuum_cost_delay/limit it will > > > have no effect for autovacuum. > > > > Right, but IIUC wi_cost_limit (and VacuumCostDelayLimit) might be > > updated. After the autovacuum launcher reloads the config file, it > > calls autovac_balance_cost() that updates that value of active > > workers. I'm not sure why we don't update workers' wi_cost_delay, > > though. > > Ah yes, I didn't realize this. Thanks. I went back and did more code > reading/analysis, and I see no reason why we shouldn't update > worker->wi_cost_delay to the new value of autovacuum_vac_cost_delay in > autovac_balance_cost(). Then, as you said, the autovac launcher will > call autovac_balance_cost() when it reloads the configuration file. > Then, the next time the autovac worker calls AutoVacuumUpdateDelay(), it > will update VacuumCostDelay. > > > > I started writing a little helper that could be used to update these > > > workerinfo->wi_cost_delay/limit in vacuum_delay_point(), > > > > Since we set vacuum delay parameters for autovacuum workers so that we > > ration out I/O equally, I think we should keep the current mechanism > > that the autovacuum launcher sets workers' delay parameters and they > > update accordingly. > > Yes, agreed, it should go in the same place as where we update > wi_cost_limit (autovac_balance_cost()). I think we should potentially > rename autovac_balance_cost() because its name and all its comments > point to its only purpose being to balance the total of the workers > wi_cost_limits to no more than autovacuum_vacuum_cost_limit. And the > autovacuum_vacuum_cost_delay doesn't need to be balanced in this way. > > Though, since this change on its own would make autovacuum pick up new > values of autovacuum_vacuum_cost_limit (without having the worker reload > the config file), I wonder if it makes sense to try and have > vacuum_delay_point() only reload the config file if it is an explicit > vacuum or an analyze not being run in an outer transaction (to avoid > overhead of reloading config file)? > > The lifecycle of this different vacuum delay-related gucs and how it > differs between autovacuum workers and explicit vacuum is quite tangled > already, though. So, I've attached a new version of the patch which is quite different from the previous versions. 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. It is worth mentioning that I think that in master, AutoVacuumUpdateDelay() was incorrectly reading wi_cost_limit and wi_cost_delay from shared memory without holding a lock. 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? Also not sure how the patch interacts with failsafe autovac and parallel vacuum. - Melanie Attachments: [text/x-patch] v2-0001-Reload-config-file-more-often-while-vacuuming.patch (11.5K, ../../CAAKRu_YWNKs2=mE27C1W8VxPJa1UR+YVuckw5kYzWowTStCHiQ@mail.gmail.com/2-v2-0001-Reload-config-file-more-often-while-vacuuming.patch) download | inline diff: From 9b5cbbc0c8f892dde3e220f0945b2c1e0d175b84 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Sun, 5 Mar 2023 14:39:16 -0500 Subject: [PATCH v2] Reload config file more often while vacuuming --- src/backend/commands/vacuum.c | 38 ++++++++--- src/backend/postmaster/autovacuum.c | 97 ++++++++++++++++++++++------- src/include/postmaster/autovacuum.h | 2 + 3 files changed, 104 insertions(+), 33 deletions(-) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index aa79d9de4d..f6cea30168 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/proc.h" @@ -75,6 +76,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; /* @@ -309,8 +311,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); @@ -327,10 +328,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, @@ -451,7 +452,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; @@ -469,7 +470,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()) @@ -521,7 +522,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) { @@ -544,6 +545,7 @@ vacuum(List *relations, VacuumParams *params, { in_vacuum = false; VacuumCostActive = false; + analyze_in_outer_xact = false; } PG_END_TRY(); @@ -2214,10 +2216,28 @@ vacuum_delay_point(void) WAIT_EVENT_VACUUM_DELAY); ResetLatch(MyLatch); + /* + * 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 has done a rebalance (as it does when + * launching a new worker). + */ + 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 ff6149a179..78b4233241 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 bool av_use_table_option_cost_delay = false; +static double av_table_option_cost_delay = 0; + /* Flags set by signal handlers */ static volatile sig_atomic_t got_SIGUSR2 = false; @@ -189,7 +192,6 @@ typedef struct autovac_table { Oid at_relid; VacuumParams at_params; - double at_vacuum_cost_delay; int at_vacuum_cost_limit; bool at_dobalance; bool at_sharedrel; @@ -225,7 +227,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; @@ -1756,7 +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, @@ -1780,11 +1780,37 @@ FreeWorkerInfo(int code, Datum arg) 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; + + if (av_use_table_option_cost_delay) { - VacuumCostDelay = MyWorkerInfo->wi_cost_delay; - VacuumCostLimit = MyWorkerInfo->wi_cost_limit; + VacuumCostDelay = av_table_option_cost_delay; } + else + { + VacuumCostDelay = autovacuum_vac_cost_delay >= 0 ? + autovacuum_vac_cost_delay : VacuumCostDelay; + } +} + +/* + * Helper for vacuum_delay_point() to allow workers to read their + * wi_cost_limit. + */ +void +AutoVacuumUpdateLimit(void) +{ + if (!MyWorkerInfo) + return; + + LWLockAcquire(AutovacuumLock, LW_SHARED); + VacuumCostLimit = MyWorkerInfo->wi_cost_limit; + LWLockRelease(AutovacuumLock); } /* @@ -1824,9 +1850,9 @@ autovac_balance_cost(void) if (worker->wi_proc != NULL && worker->wi_dobalance && - worker->wi_cost_limit_base > 0 && worker->wi_cost_delay > 0) + worker->wi_cost_limit_base > 0 && vac_cost_delay > 0) cost_total += - (double) worker->wi_cost_limit_base / worker->wi_cost_delay; + (double) worker->wi_cost_limit_base / vac_cost_delay; } /* there are no cost limits -- nothing to do */ @@ -1844,7 +1870,7 @@ autovac_balance_cost(void) if (worker->wi_proc != NULL && worker->wi_dobalance && - worker->wi_cost_limit_base > 0 && worker->wi_cost_delay > 0) + worker->wi_cost_limit_base > 0 && vac_cost_delay > 0) { int limit = (int) (cost_avail * worker->wi_cost_limit_base / cost_total); @@ -1861,11 +1887,10 @@ autovac_balance_cost(void) } 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)", + elog(DEBUG2, "autovac_balance_cost(pid=%d db=%u, rel=%u, dobalance=%s cost_limit=%d, cost_limit_base=%d)", 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); + worker->wi_cost_limit, worker->wi_cost_limit_base); } } @@ -2326,6 +2351,15 @@ do_autovacuum(void) ConfigReloadPending = false; ProcessConfigFile(PGC_SIGHUP); + /* + * Autovacuum workers should always update VacuumCostDelay and + * VacuumCostLimit in case they were overridden by the reload. + */ + AutoVacuumUpdateDelay(); + LWLockAcquire(AutovacuumLock, LW_SHARED); + VacuumCostLimit = MyWorkerInfo->wi_cost_limit; + LWLockRelease(AutovacuumLock); + /* * You might be tempted to bail out if we see autovacuum is now * disabled. Must resist that temptation -- this might be a @@ -2424,21 +2458,20 @@ do_autovacuum(void) stdVacuumCostDelay = VacuumCostDelay; stdVacuumCostLimit = VacuumCostLimit; + AutoVacuumUpdateDelay(); + /* 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; + VacuumCostLimit = MyWorkerInfo->wi_cost_limit; /* do a balance */ autovac_balance_cost(); - /* set the active cost parameters from the result of that */ - AutoVacuumUpdateDelay(); - /* done */ LWLockRelease(AutovacuumLock); @@ -2569,6 +2602,11 @@ deleted: { ConfigReloadPending = false; ProcessConfigFile(PGC_SIGHUP); + AutoVacuumUpdateDelay(); + + LWLockAcquire(AutovacuumLock, LW_SHARED); + VacuumCostLimit = MyWorkerInfo->wi_cost_limit; + LWLockRelease(AutovacuumLock); } LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); @@ -2771,7 +2809,11 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map, /* fetch the relation's relcache entry */ classTup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(relid)); if (!HeapTupleIsValid(classTup)) + { + av_use_table_option_cost_delay = false; + av_table_option_cost_delay = 0; return NULL; + } classForm = (Form_pg_class) GETSTRUCT(classTup); /* @@ -2802,7 +2844,6 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map, int multixact_freeze_min_age; int multixact_freeze_table_age; int vac_cost_limit; - double vac_cost_delay; int log_min_duration; /* @@ -2812,12 +2853,16 @@ 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; + if (avopts && avopts->vacuum_cost_delay >= 0) + { + av_use_table_option_cost_delay = true; + av_table_option_cost_delay = avopts->vacuum_cost_delay; + } + else + { + av_use_table_option_cost_delay = false; + av_table_option_cost_delay = 0; + } /* 0 or -1 in autovac setting means use plain vacuum_cost_limit */ vac_cost_limit = (avopts && avopts->vacuum_cost_limit > 0) @@ -2880,7 +2925,6 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map, 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_relname = NULL; tab->at_nspname = NULL; tab->at_datname = NULL; @@ -2893,6 +2937,11 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map, !(avopts && (avopts->vacuum_cost_limit > 0 || avopts->vacuum_cost_delay > 0)); } + else + { + av_use_table_option_cost_delay = false; + av_table_option_cost_delay = 0; + } heap_freetuple(classTup); return tab; 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] 18+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-03-07 05:09 Masahiko Sawada <[email protected]> parent: Melanie Plageman <[email protected]> 0 siblings, 1 reply; 18+ messages in thread From: Masahiko Sawada @ 2023-03-07 05:09 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: pgsql-hackers; Andres Freund <[email protected]>; Amit Kapila <[email protected]> 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 > <[email protected]> wrote: > > > > On Thu, Mar 2, 2023 at 2:36 AM Masahiko Sawada <[email protected]> wrote: > > > > > > On Thu, Mar 2, 2023 at 10:41 AM Melanie Plageman > > > <[email protected]> wrote: > > > > On another topic, I've just realized that when autovacuuming we only > > > > update tab->at_vacuum_cost_delay/limit from > > > > autovacuum_vacuum_cost_delay/limit for each table (in > > > > table_recheck_autovac()) and then use that to update > > > > MyWorkerInfo->wi_cost_delay/limit. MyWorkerInfo->wi_cost_delay/limit is > > > > what is used to update VacuumCostDelay/Limit in AutoVacuumUpdateDelay(). > > > > So, even if we reload the config file in vacuum_delay_point(), if we > > > > don't use the new value of autovacuum_vacuum_cost_delay/limit it will > > > > have no effect for autovacuum. > > > > > > Right, but IIUC wi_cost_limit (and VacuumCostDelayLimit) might be > > > updated. After the autovacuum launcher reloads the config file, it > > > calls autovac_balance_cost() that updates that value of active > > > workers. I'm not sure why we don't update workers' wi_cost_delay, > > > though. > > > > Ah yes, I didn't realize this. Thanks. I went back and did more code > > reading/analysis, and I see no reason why we shouldn't update > > worker->wi_cost_delay to the new value of autovacuum_vac_cost_delay in > > autovac_balance_cost(). Then, as you said, the autovac launcher will > > call autovac_balance_cost() when it reloads the configuration file. > > Then, the next time the autovac worker calls AutoVacuumUpdateDelay(), it > > will update VacuumCostDelay. > > > > > > I started writing a little helper that could be used to update these > > > > workerinfo->wi_cost_delay/limit in vacuum_delay_point(), > > > > > > Since we set vacuum delay parameters for autovacuum workers so that we > > > ration out I/O equally, I think we should keep the current mechanism > > > that the autovacuum launcher sets workers' delay parameters and they > > > update accordingly. > > > > Yes, agreed, it should go in the same place as where we update > > wi_cost_limit (autovac_balance_cost()). I think we should potentially > > rename autovac_balance_cost() because its name and all its comments > > point to its only purpose being to balance the total of the workers > > wi_cost_limits to no more than autovacuum_vacuum_cost_limit. And the > > autovacuum_vacuum_cost_delay doesn't need to be balanced in this way. > > > > Though, since this change on its own would make autovacuum pick up new > > values of autovacuum_vacuum_cost_limit (without having the worker reload > > the config file), I wonder if it makes sense to try and have > > vacuum_delay_point() only reload the config file if it is an explicit > > vacuum or an analyze not being run in an outer transaction (to avoid > > overhead of reloading config file)? > > > > The lifecycle of this different vacuum delay-related gucs and how it > > differs between autovacuum workers and explicit vacuum is quite tangled > > already, though. > > So, I've attached a new version of the patch which is quite different > from the previous versions. Thank you for updating the patch! > > 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. --- 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? --- In at autovac_balance_cost(), we have, 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); : /* not set? nothing to do */ if (vac_cost_limit <= 0 || vac_cost_delay <= 0) return; IIUC if autovacuum_vac_cost_delay is changed to 0 during autovacuums running, their vacuum delay parameters are not changed. It's not a bug of the patch but I think we can fix it in this patch. > > It is worth mentioning that I think that in master, > AutoVacuumUpdateDelay() was incorrectly reading wi_cost_limit and > wi_cost_delay from shared memory without holding a lock. Indeed. > 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. > > 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. As far as I can see we don't need special treatments for parallel vacuum cases since it works only in manual vacuum. It calculates the sleep time based on the shared cost balance and how much the worker did I/O but the basic mechanism is the same as non-parallel case. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-03-08 17:42 Jim Nasby <[email protected]> parent: Masahiko Sawada <[email protected]> 1 sibling, 2 replies; 18+ messages in thread From: Jim Nasby @ 2023-03-08 17:42 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; Melanie Plageman <[email protected]>; +Cc: pgsql-hackers; Andres Freund <[email protected]> On 3/2/23 1:36 AM, Masahiko Sawada wrote: >>>> For example, I guess we will need to take care of changes of >>>> maintenance_work_mem. Currently we initialize the dead tuple space at >>>> the beginning of lazy vacuum, but perhaps we would need to >>>> enlarge/shrink it based on the new value? Doesn't the dead tuple space grow as needed? Last I looked we don't allocate up to 1GB right off the bat. >>> I don't think we need to do anything about that initially, just because the >>> config can be changed in a more granular way, doesn't mean we have to react to >>> every change for the current operation. >> Perhaps we can mention in the docs that a change to maintenance_work_mem >> will not take effect in the middle of vacuuming a table. But, Ithink it probably >> isn't needed. > Agreed. I disagree that there's no need for this. Sure, if maintenance_work_memory is 10MB then it's no big deal to just abandon your current vacuum and start a new one, but the index vacuuming phase with maintenance_work_mem set to say 500MB can take quite a while. Forcing a user to either suck it up or throw everything in the phase away isn't terribly good. Of course, if the patch that eliminates the 1GB vacuum limit gets committed the situation will be even worse. While it'd be nice to also honor maintenance_work_mem getting set lower, I don't see any need to go through heroics to accomplish that. Simply recording the change and honoring it for future attempts to grow the memory and on future passes through the heap would be plenty. All that said, don't let these suggestions get in the way of committing this. Just having the ability to tweak cost parameters would be a win. ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-03-09 00:27 Andres Freund <[email protected]> parent: Jim Nasby <[email protected]> 1 sibling, 0 replies; 18+ messages in thread From: Andres Freund @ 2023-03-09 00:27 UTC (permalink / raw) To: Jim Nasby <[email protected]>; +Cc: Masahiko Sawada <[email protected]>; Melanie Plageman <[email protected]>; pgsql-hackers Hi, On 2023-03-08 11:42:31 -0600, Jim Nasby wrote: > On 3/2/23 1:36 AM, Masahiko Sawada wrote: > > > > > > For example, I guess we will need to take care of changes of > > > > > maintenance_work_mem. Currently we initialize the dead tuple space at > > > > > the beginning of lazy vacuum, but perhaps we would need to > > > > > enlarge/shrink it based on the new value? > Doesn't the dead tuple space grow as needed? Last I looked we don't allocate > up to 1GB right off the bat. > > > > I don't think we need to do anything about that initially, just because the > > > > config can be changed in a more granular way, doesn't mean we have to react to > > > > every change for the current operation. > > > Perhaps we can mention in the docs that a change to maintenance_work_mem > > > will not take effect in the middle of vacuuming a table. But, Ithink it probably > > > isn't needed. > > Agreed. > > I disagree that there's no need for this. Sure, if maintenance_work_memory > is 10MB then it's no big deal to just abandon your current vacuum and start > a new one, but the index vacuuming phase with maintenance_work_mem set to > say 500MB can take quite a while. Forcing a user to either suck it up or > throw everything in the phase away isn't terribly good. > > Of course, if the patch that eliminates the 1GB vacuum limit gets committed > the situation will be even worse. > > While it'd be nice to also honor maintenance_work_mem getting set lower, I > don't see any need to go through heroics to accomplish that. Simply > recording the change and honoring it for future attempts to grow the memory > and on future passes through the heap would be plenty. > > All that said, don't let these suggestions get in the way of committing > this. Just having the ability to tweak cost parameters would be a win. Nobody said anything about it not being useful to react to m_w_m changes, just that it's not required to make some progress . So I really don't understand what the point of your comment is. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-03-09 07:47 John Naylor <[email protected]> parent: Jim Nasby <[email protected]> 1 sibling, 1 reply; 18+ messages in thread From: John Naylor @ 2023-03-09 07:47 UTC (permalink / raw) To: Jim Nasby <[email protected]>; +Cc: Masahiko Sawada <[email protected]>; Melanie Plageman <[email protected]>; pgsql-hackers; Andres Freund <[email protected]> On Thu, Mar 9, 2023 at 12:42 AM Jim Nasby <[email protected]> wrote: > > Doesn't the dead tuple space grow as needed? Last I looked we don't allocate up to 1GB right off the bat. Incorrect. > Of course, if the patch that eliminates the 1GB vacuum limit gets committed the situation will be even worse. If you're referring to the proposed tid store, I'd be interested in seeing a reproducible test case with a m_w_m over 1GB where it makes things worse than the current state of affairs. -- John Naylor EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-03-09 13:19 Masahiko Sawada <[email protected]> parent: John Naylor <[email protected]> 0 siblings, 0 replies; 18+ messages in thread From: Masahiko Sawada @ 2023-03-09 13:19 UTC (permalink / raw) To: John Naylor <[email protected]>; +Cc: Jim Nasby <[email protected]>; Melanie Plageman <[email protected]>; pgsql-hackers; Andres Freund <[email protected]> On Thu, Mar 9, 2023 at 4:47 PM John Naylor <[email protected]> wrote: > > > On Thu, Mar 9, 2023 at 12:42 AM Jim Nasby <[email protected]> wrote: > > > > Doesn't the dead tuple space grow as needed? Last I looked we don't allocate up to 1GB right off the bat. > > Incorrect. > > > Of course, if the patch that eliminates the 1GB vacuum limit gets committed the situation will be even worse. > > If you're referring to the proposed tid store, I'd be interested in seeing a reproducible test case with a m_w_m over 1GB where it makes things worse than the current state of affairs. And I think that the tidstore makes it easy to react to maintenance_work_mem changes. We don't need to enlarge it and just update its memory limit at an appropriate time. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-03-10 02:22 Melanie Plageman <[email protected]> parent: Masahiko Sawada <[email protected]> 0 siblings, 1 reply; 18+ messages in thread From: Melanie Plageman @ 2023-03-10 02:22 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: pgsql-hackers; Andres Freund <[email protected]>; Amit Kapila <[email protected]> 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(). However, wi_cost_delay is a double, so if we start updating it on config reload in vacuum_delay_point(), we definitely need some protection against torn reads. The table options can only change when workers start vacuuming a new table, so maybe there is some way to use this to solve this problem? > > It is worth mentioning that I think that in master, > > AutoVacuumUpdateDelay() was incorrectly reading wi_cost_limit and > > wi_cost_delay from shared memory without holding a lock. > > Indeed. > > > 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. Maybe we can do something like this with the table options values? - Melanie ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-03-10 03:26 Masahiko Sawada <[email protected]> parent: Melanie Plageman <[email protected]> 0 siblings, 1 reply; 18+ messages in thread From: Masahiko Sawada @ 2023-03-10 03:26 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: pgsql-hackers; Andres Freund <[email protected]>; Amit Kapila <[email protected]> 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. > > However, wi_cost_delay is a double, so if we start updating it on config > reload in vacuum_delay_point(), we definitely need some protection > against torn reads. > > The table options can only change when workers start vacuuming a new > table, so maybe there is some way to use this to solve this problem? > > > > It is worth mentioning that I think that in master, > > > AutoVacuumUpdateDelay() was incorrectly reading wi_cost_limit and > > > wi_cost_delay from shared memory without holding a lock. > > > > Indeed. > > > > > 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. > > Maybe we can do something like this with the table options values? Since an autovacuum that uses any of table option cost delay parameters is excluded from the balancing algorithm, the launcher doesn't need to notify such workers of changes of the cost-limit, no? Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-03-10 23:11 Melanie Plageman <[email protected]> parent: Masahiko Sawada <[email protected]> 0 siblings, 1 reply; 18+ messages in thread From: Melanie Plageman @ 2023-03-10 23:11 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: pgsql-hackers; Andres Freund <[email protected]>; Amit Kapila <[email protected]> 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. As for the cost limit safe access issue, maybe we can avoid a LWLock acquisition for reading wi_cost_limit by using an atomic similar to what you suggested here for "did_rebalance". > > I've added in a shared lock for reading from wi_cost_limit in this > > patch. However, AutoVacuumUpdateLimit() is called unconditionally in > > vacuum_delay_point(), which is called quite often (per block-ish), so I > > was trying to think if there is a way we could avoid having to check > > this shared memory variable on every call to vacuum_delay_point(). > > Rebalances shouldn't happen very often (done by the launcher when a new > > worker is launched and by workers between vacuuming tables). Maybe we > > can read from it less frequently? > > Yeah, acquiring the lwlock for every call to vacuum_delay_point() > seems to be harmful. One idea would be to have one sig_atomic_t > variable in WorkerInfoData and autovac_balance_cost() set it to true > after rebalancing the worker's cost-limit. The worker can check it > without locking and update its delay parameters if the flag is true. Instead of having the atomic indicate whether or not someone (launcher or another worker) did a rebalance, it would simply store the current cost limit. Then the worker can normally access it with a simple read. My rationale is that if we used an atomic to indicate whether or not we did a rebalance ("did_rebalance"), we would have the same cache coherency guarantees as if we just used the atomic for the cost limit. If we read from the "did_rebalance" variable and missed someone having written to it on another core, we still wouldn't get around to checking the wi_cost_limit variable in shared memory, so it doesn't matter that we bothered to keep it in shared memory and use a lock to access it. I noticed we don't allow wi_cost_limit to ever be less than 0, so we could store wi_cost_limit in an atomic uint32. I'm not sure if it is okay to do pg_atomic_read_u32() and pg_atomic_unlocked_write_u32() or if we need pg_atomic_write_u32() in most cases. I've implemented the atomic cost limit in the attached patch. Though, I'm pretty unsure about how I initialized the atomics in AutoVacuumShmemInit()... If the consensus is that it is simply too confusing to take wi_cost_delay out of WorkerInfo, we might be able to afford using a shared lock to access it because we won't call AutoVacuumUpdateDelay() on every invocation of vacuum_delay_point() -- only when we've reloaded the config file. One 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. > --- > 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(). 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? > --- > In at autovac_balance_cost(), we have, > > 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); > : > /* not set? nothing to do */ > if (vac_cost_limit <= 0 || vac_cost_delay <= 0) > return; > > IIUC if autovacuum_vac_cost_delay is changed to 0 during autovacuums > running, their vacuum delay parameters are not changed. It's not a bug > of the patch but I think we can fix it in this patch. Yes, currently (in master) wi_cost_delay does not get updated anywhere. In my patch, the global variable we are using for delay is updated but it is not done in autovac_balance_cost(). > > 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. Thanks again for the detailed feedback! - Melanie Attachments: [text/x-patch] v3-0001-Reload-config-file-more-often-while-vacuuming.patch (13.2K, ../../CAAKRu_YxDh-31zzF9vA=6FfggE8-mQo8bwxCxaFqY=pb8kTTag@mail.gmail.com/2-v3-0001-Reload-config-file-more-often-while-vacuuming.patch) download | inline diff: From 97b6ebc3eaa3d8ec53c25822c6cad2d2bbe22837 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Sun, 5 Mar 2023 14:39:16 -0500 Subject: [PATCH v3] Reload config file more often while vacuuming --- src/backend/commands/vacuum.c | 38 +++++++--- src/backend/postmaster/autovacuum.c | 113 ++++++++++++++++++++-------- src/include/postmaster/autovacuum.h | 2 + 3 files changed, 114 insertions(+), 39 deletions(-) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 2e12baf8eb..9d5ce846a5 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/proc.h" @@ -75,6 +76,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; /* @@ -313,8 +315,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); @@ -331,10 +332,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, @@ -456,7 +457,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; @@ -474,7 +475,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()) @@ -526,7 +527,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) { @@ -549,6 +550,7 @@ vacuum(List *relations, VacuumParams *params, { in_vacuum = false; VacuumCostActive = false; + analyze_in_outer_xact = false; } PG_END_TRY(); @@ -2238,10 +2240,28 @@ vacuum_delay_point(void) WAIT_EVENT_VACUUM_DELAY); ResetLatch(MyLatch); + /* + * 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 has done a rebalance (as it does when + * launching a new worker). + */ + 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..a9b7217638 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 bool av_use_table_option_cost_delay = false; +static double av_table_option_cost_delay = 0; + /* Flags set by signal handlers */ static volatile sig_atomic_t got_SIGUSR2 = false; @@ -189,7 +192,6 @@ typedef struct autovac_table { Oid at_relid; VacuumParams at_params; - double at_vacuum_cost_delay; int at_vacuum_cost_limit; bool at_dobalance; bool at_sharedrel; @@ -225,8 +227,7 @@ typedef struct WorkerInfoData TimestampTz wi_launchtime; bool wi_dobalance; bool wi_sharedrel; - double wi_cost_delay; - int wi_cost_limit; + pg_atomic_uint32 wi_cost_limit; int wi_cost_limit_base; } WorkerInfoData; @@ -743,6 +744,7 @@ AutoVacLauncherMain(int argc, char *argv[]) worker = AutoVacuumShmem->av_startingWorker; worker->wi_dboid = InvalidOid; worker->wi_tableoid = InvalidOid; + pg_atomic_write_u32(&MyWorkerInfo->wi_cost_limit, 0); worker->wi_sharedrel = false; worker->wi_proc = NULL; worker->wi_launchtime = 0; @@ -815,6 +817,7 @@ HandleAutoVacLauncherInterrupts(void) ConfigReloadPending = false; ProcessConfigFile(PGC_SIGHUP); + AutoVacuumUpdateDelay(); /* shutdown requested in config file? */ if (!AutoVacuumingActive()) AutoVacLauncherShutdown(); @@ -1756,8 +1759,7 @@ 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; + pg_atomic_write_u32(&MyWorkerInfo->wi_cost_limit, 0); MyWorkerInfo->wi_cost_limit_base = 0; dlist_push_head(&AutoVacuumShmem->av_freeWorkers, &MyWorkerInfo->wi_links); @@ -1780,13 +1782,37 @@ FreeWorkerInfo(int code, Datum arg) 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; + + if (av_use_table_option_cost_delay) { - VacuumCostDelay = MyWorkerInfo->wi_cost_delay; - VacuumCostLimit = MyWorkerInfo->wi_cost_limit; + VacuumCostDelay = av_table_option_cost_delay; + } + else + { + VacuumCostDelay = autovacuum_vac_cost_delay >= 0 ? + autovacuum_vac_cost_delay : VacuumCostDelay; } } +/* + * Helper for vacuum_delay_point() to allow workers to read their + * wi_cost_limit. + */ +void +AutoVacuumUpdateLimit(void) +{ + if (!MyWorkerInfo) + return; + + VacuumCostLimit = pg_atomic_read_u32(&MyWorkerInfo->wi_cost_limit); +} + /* * autovac_balance_cost * Recalculate the cost limit setting for each active worker. @@ -1824,9 +1850,9 @@ autovac_balance_cost(void) if (worker->wi_proc != NULL && worker->wi_dobalance && - worker->wi_cost_limit_base > 0 && worker->wi_cost_delay > 0) + worker->wi_cost_limit_base > 0 && vac_cost_delay > 0) cost_total += - (double) worker->wi_cost_limit_base / worker->wi_cost_delay; + (double) worker->wi_cost_limit_base / vac_cost_delay; } /* there are no cost limits -- nothing to do */ @@ -1844,7 +1870,7 @@ autovac_balance_cost(void) if (worker->wi_proc != NULL && worker->wi_dobalance && - worker->wi_cost_limit_base > 0 && worker->wi_cost_delay > 0) + worker->wi_cost_limit_base > 0 && vac_cost_delay > 0) { int limit = (int) (cost_avail * worker->wi_cost_limit_base / cost_total); @@ -1855,17 +1881,15 @@ autovac_balance_cost(void) * 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); + pg_atomic_unlocked_write_u32(&worker->wi_cost_limit, + Max(Min(limit, worker->wi_cost_limit_base), 1)); } 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)", + elog(DEBUG2, "autovac_balance_cost(pid=%d db=%u, rel=%u, dobalance=%s cost_limit=%d, cost_limit_base=%d)", 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); + pg_atomic_read_u32(&worker->wi_cost_limit), worker->wi_cost_limit_base); } } @@ -2326,6 +2350,13 @@ do_autovacuum(void) ConfigReloadPending = false; ProcessConfigFile(PGC_SIGHUP); + /* + * Autovacuum workers should always update VacuumCostDelay and + * VacuumCostLimit in case they were overridden by the reload. + */ + AutoVacuumUpdateDelay(); + AutoVacuumUpdateLimit(); + /* * You might be tempted to bail out if we see autovacuum is now * disabled. Must resist that temptation -- this might be a @@ -2424,21 +2455,22 @@ do_autovacuum(void) stdVacuumCostDelay = VacuumCostDelay; stdVacuumCostLimit = VacuumCostLimit; + AutoVacuumUpdateDelay(); + /* 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; + /* this cannot exceed 10000 anyway */ + pg_atomic_unlocked_write_u32(&MyWorkerInfo->wi_cost_limit, + tab->at_vacuum_cost_limit); 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(); - /* done */ LWLockRelease(AutovacuumLock); @@ -2569,6 +2601,8 @@ deleted: { ConfigReloadPending = false; ProcessConfigFile(PGC_SIGHUP); + AutoVacuumUpdateDelay(); + AutoVacuumUpdateLimit(); } LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); @@ -2771,7 +2805,11 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map, /* fetch the relation's relcache entry */ classTup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(relid)); if (!HeapTupleIsValid(classTup)) + { + av_use_table_option_cost_delay = false; + av_table_option_cost_delay = 0; return NULL; + } classForm = (Form_pg_class) GETSTRUCT(classTup); /* @@ -2802,7 +2840,6 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map, int multixact_freeze_min_age; int multixact_freeze_table_age; int vac_cost_limit; - double vac_cost_delay; int log_min_duration; /* @@ -2812,12 +2849,16 @@ 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; + if (avopts && avopts->vacuum_cost_delay >= 0) + { + av_use_table_option_cost_delay = true; + av_table_option_cost_delay = avopts->vacuum_cost_delay; + } + else + { + av_use_table_option_cost_delay = false; + av_table_option_cost_delay = 0; + } /* 0 or -1 in autovac setting means use plain vacuum_cost_limit */ vac_cost_limit = (avopts && avopts->vacuum_cost_limit > 0) @@ -2882,7 +2923,6 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map, 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_relname = NULL; tab->at_nspname = NULL; tab->at_datname = NULL; @@ -2895,6 +2935,11 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map, !(avopts && (avopts->vacuum_cost_limit > 0 || avopts->vacuum_cost_delay > 0)); } + else + { + av_use_table_option_cost_delay = false; + av_table_option_cost_delay = 0; + } heap_freetuple(classTup); return tab; @@ -3361,6 +3406,7 @@ AutoVacuumShmemInit(void) { WorkerInfo worker; int i; + dlist_iter iter; Assert(!found); @@ -3374,10 +3420,17 @@ 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); + + dlist_foreach(iter, &AutoVacuumShmem->av_freeWorkers) + pg_atomic_init_u32( + &(dlist_container(WorkerInfoData, wi_links, iter.cur))->wi_cost_limit, + 0); + } 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] 18+ messages in thread
* Re: Should vacuum process config file reload more often @ 2023-03-11 00:34 Melanie Plageman <[email protected]> parent: Melanie Plageman <[email protected]> 0 siblings, 0 replies; 18+ messages in thread From: Melanie Plageman @ 2023-03-11 00:34 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: pgsql-hackers; Andres Freund <[email protected]>; Amit Kapila <[email protected]> On Fri, Mar 10, 2023 at 6:11 PM 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. > > As for the cost limit safe access issue, maybe we can avoid a LWLock > acquisition for reading wi_cost_limit by using an atomic similar to what > you suggested here for "did_rebalance". > > > > I've added in a shared lock for reading from wi_cost_limit in this > > > patch. However, AutoVacuumUpdateLimit() is called unconditionally in > > > vacuum_delay_point(), which is called quite often (per block-ish), so I > > > was trying to think if there is a way we could avoid having to check > > > this shared memory variable on every call to vacuum_delay_point(). > > > Rebalances shouldn't happen very often (done by the launcher when a new > > > worker is launched and by workers between vacuuming tables). Maybe we > > > can read from it less frequently? > > > > Yeah, acquiring the lwlock for every call to vacuum_delay_point() > > seems to be harmful. One idea would be to have one sig_atomic_t > > variable in WorkerInfoData and autovac_balance_cost() set it to true > > after rebalancing the worker's cost-limit. The worker can check it > > without locking and update its delay parameters if the flag is true. > > Instead of having the atomic indicate whether or not someone (launcher > or another worker) did a rebalance, it would simply store the current > cost limit. Then the worker can normally access it with a simple read. > > My rationale is that if we used an atomic to indicate whether or not we > did a rebalance ("did_rebalance"), we would have the same cache > coherency guarantees as if we just used the atomic for the cost limit. > If we read from the "did_rebalance" variable and missed someone having > written to it on another core, we still wouldn't get around to checking > the wi_cost_limit variable in shared memory, so it doesn't matter that > we bothered to keep it in shared memory and use a lock to access it. > > I noticed we don't allow wi_cost_limit to ever be less than 0, so we > could store wi_cost_limit in an atomic uint32. > > I'm not sure if it is okay to do pg_atomic_read_u32() and > pg_atomic_unlocked_write_u32() or if we need pg_atomic_write_u32() in > most cases. > > I've implemented the atomic cost limit in the attached patch. Though, > I'm pretty unsure about how I initialized the atomics in > AutoVacuumShmemInit()... > > If the consensus is that it is simply too confusing to take > wi_cost_delay out of WorkerInfo, we might be able to afford using a > shared lock to access it because we won't call AutoVacuumUpdateDelay() > on every invocation of vacuum_delay_point() -- only when we've reloaded > the config file. One such implementation is attached. - Melanie Attachments: [text/x-patch] v4-0001-vacuum-reloads-config-file-more-often.patch (9.2K, ../../CAAKRu_Y0xdDu-6rKEJk8APJTupdOobjoNtGVKLsfyFYcQEMKZw@mail.gmail.com/2-v4-0001-vacuum-reloads-config-file-more-often.patch) download | inline diff: From f0029f1e410852b41b5562939051ede86235508b Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Fri, 10 Mar 2023 18:33:47 -0500 Subject: [PATCH v4] vacuum reloads config file more often --- src/backend/commands/vacuum.c | 38 ++++++++++++---- src/backend/postmaster/autovacuum.c | 69 +++++++++++++++++++++++------ src/include/postmaster/autovacuum.h | 2 + 3 files changed, 87 insertions(+), 22 deletions(-) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 2e12baf8eb..9d5ce846a5 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/proc.h" @@ -75,6 +76,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; /* @@ -313,8 +315,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); @@ -331,10 +332,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, @@ -456,7 +457,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; @@ -474,7 +475,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()) @@ -526,7 +527,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) { @@ -549,6 +550,7 @@ vacuum(List *relations, VacuumParams *params, { in_vacuum = false; VacuumCostActive = false; + analyze_in_outer_xact = false; } PG_END_TRY(); @@ -2238,10 +2240,28 @@ vacuum_delay_point(void) WAIT_EVENT_VACUUM_DELAY); ResetLatch(MyLatch); + /* + * 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 has done a rebalance (as it does when + * launching a new worker). + */ + 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..7a202b2bd9 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -226,7 +226,7 @@ typedef struct WorkerInfoData bool wi_dobalance; bool wi_sharedrel; double wi_cost_delay; - int wi_cost_limit; + pg_atomic_uint32 wi_cost_limit; int wi_cost_limit_base; } WorkerInfoData; @@ -743,6 +743,7 @@ AutoVacLauncherMain(int argc, char *argv[]) worker = AutoVacuumShmem->av_startingWorker; worker->wi_dboid = InvalidOid; worker->wi_tableoid = InvalidOid; + pg_atomic_write_u32(&MyWorkerInfo->wi_cost_limit, 0); worker->wi_sharedrel = false; worker->wi_proc = NULL; worker->wi_launchtime = 0; @@ -1757,7 +1758,7 @@ FreeWorkerInfo(int code, Datum arg) MyWorkerInfo->wi_launchtime = 0; MyWorkerInfo->wi_dobalance = false; MyWorkerInfo->wi_cost_delay = 0; - MyWorkerInfo->wi_cost_limit = 0; + pg_atomic_write_u32(&MyWorkerInfo->wi_cost_limit, 0); MyWorkerInfo->wi_cost_limit_base = 0; dlist_push_head(&AutoVacuumShmem->av_freeWorkers, &MyWorkerInfo->wi_links); @@ -1780,11 +1781,36 @@ FreeWorkerInfo(int code, Datum arg) void AutoVacuumUpdateDelay(void) { - if (MyWorkerInfo) + if (!MyWorkerInfo) + return; + + LWLockAcquire(AutovacuumLock, LW_SHARED); + + if (MyWorkerInfo->wi_dobalance) { - VacuumCostDelay = MyWorkerInfo->wi_cost_delay; - VacuumCostLimit = MyWorkerInfo->wi_cost_limit; + VacuumCostDelay = autovacuum_vac_cost_delay >= 0 ? + autovacuum_vac_cost_delay : VacuumCostDelay; + + MyWorkerInfo->wi_cost_delay = VacuumCostDelay; } + else + VacuumCostDelay = MyWorkerInfo->wi_cost_delay; + + LWLockRelease(AutovacuumLock); + +} + +/* + * Helper for vacuum_delay_point() to allow workers to read their + * wi_cost_limit. + */ +void +AutoVacuumUpdateLimit(void) +{ + if (!MyWorkerInfo) + return; + + VacuumCostLimit = pg_atomic_read_u32(&MyWorkerInfo->wi_cost_limit); } /* @@ -1855,16 +1881,15 @@ autovac_balance_cost(void) * 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); + pg_atomic_unlocked_write_u32(&worker->wi_cost_limit, + Max(Min(limit, worker->wi_cost_limit_base), 1)); } 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, + pg_atomic_read_u32(&worker->wi_cost_limit), worker->wi_cost_limit_base, worker->wi_cost_delay); } } @@ -2326,6 +2351,13 @@ do_autovacuum(void) ConfigReloadPending = false; ProcessConfigFile(PGC_SIGHUP); + /* + * Autovacuum workers should always update VacuumCostDelay and + * VacuumCostLimit in case they were overridden by the reload. + */ + AutoVacuumUpdateDelay(); + AutoVacuumUpdateLimit(); + /* * You might be tempted to bail out if we see autovacuum is now * disabled. Must resist that temptation -- this might be a @@ -2424,21 +2456,22 @@ do_autovacuum(void) stdVacuumCostDelay = VacuumCostDelay; stdVacuumCostLimit = VacuumCostLimit; + /* 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; + /* this cannot exceed 10000 anyway */ + pg_atomic_unlocked_write_u32(&MyWorkerInfo->wi_cost_limit, + tab->at_vacuum_cost_limit); 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(); - /* done */ LWLockRelease(AutovacuumLock); @@ -2569,6 +2602,8 @@ deleted: { ConfigReloadPending = false; ProcessConfigFile(PGC_SIGHUP); + AutoVacuumUpdateDelay(); + AutoVacuumUpdateLimit(); } LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); @@ -3361,6 +3396,7 @@ AutoVacuumShmemInit(void) { WorkerInfo worker; int i; + dlist_iter iter; Assert(!found); @@ -3374,10 +3410,17 @@ 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); + + dlist_foreach(iter, &AutoVacuumShmem->av_freeWorkers) + pg_atomic_init_u32( + &(dlist_container(WorkerInfoData, wi_links, iter.cur))->wi_cost_limit, + 0); + } 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] 18+ messages in thread
* [PATCH] Tweak xheader_width input parsing @ 2023-05-19 10:58 Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 18+ messages in thread From: Alvaro Herrera @ 2023-05-19 10:58 UTC (permalink / raw) Don't throw away the previous value when an invalid value is proposed. Also, change the error messages to not need separate translations. While at it, change pager_min_lines to also avoid changing the setting when an invalid value is given. --- src/bin/psql/command.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index 607a57715a3..07c5f026b9b 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -4521,13 +4521,16 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet) popt->topt.expanded_header_width_type = PRINT_XHEADER_PAGE; else { - popt->topt.expanded_header_width_type = PRINT_XHEADER_EXACT_WIDTH; - popt->topt.expanded_header_exact_width = atoi(value); - if (popt->topt.expanded_header_exact_width == 0) + int intval = atoi(value); + + if (intval == 0) { pg_log_error("\\pset: allowed xheader_width values are full (default), column, page, or a number specifying the exact width."); return false; } + + popt->topt.expanded_header_width_type = PRINT_XHEADER_EXACT_WIDTH; + popt->topt.expanded_header_exact_width = intval; } } @@ -4660,8 +4663,9 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet) /* set minimum lines for pager use */ else if (strcmp(param, "pager_min_lines") == 0) { - if (value) - popt->topt.pager_min_lines = atoi(value); + if (value && + !ParseVariableNum(value, "pager_min_lines", &popt->topt.pager_min_lines)) + return false; } /* disable "(x rows)" footer */ @@ -4727,11 +4731,11 @@ printPsetInfo(const char *param, printQueryOpt *popt) else if (strcmp(param, "xheader_width") == 0) { if (popt->topt.expanded_header_width_type == PRINT_XHEADER_FULL) - printf(_("Expanded header width is 'full'.\n")); + printf(_("Expanded header width is '%s'.\n"), "full"); else if (popt->topt.expanded_header_width_type == PRINT_XHEADER_COLUMN) - printf(_("Expanded header width is 'column'.\n")); + printf(_("Expanded header width is '%s'.\n"), "column"); else if (popt->topt.expanded_header_width_type == PRINT_XHEADER_PAGE) - printf(_("Expanded header width is 'page'.\n")); + printf(_("Expanded header width is '%s'.\n"), "page"); else if (popt->topt.expanded_header_width_type == PRINT_XHEADER_EXACT_WIDTH) printf(_("Expanded header width is %d.\n"), popt->topt.expanded_header_exact_width); } base-commit: 0b8ace8d773257fffeaceda196ed94877c2b74df -- 2.39.2 --7dltxkhfrj6skipq-- ^ permalink raw reply [nested|flat] 18+ messages in thread
end of thread, other threads:[~2023-05-19 10:58 UTC | newest] Thread overview: 18+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-02-23 22:08 Should vacuum process config file reload more often Melanie Plageman <[email protected]> 2023-02-24 08:42 ` Pavel Borisov <[email protected]> 2023-03-01 19:54 ` Melanie Plageman <[email protected]> 2023-02-27 14:11 ` Masahiko Sawada <[email protected]> 2023-03-02 01:41 ` Melanie Plageman <[email protected]> 2023-03-02 07:36 ` Masahiko Sawada <[email protected]> 2023-03-02 23:37 ` Melanie Plageman <[email protected]> 2023-03-05 20:26 ` Melanie Plageman <[email protected]> 2023-03-07 05:09 ` Masahiko Sawada <[email protected]> 2023-03-10 02:22 ` Melanie Plageman <[email protected]> 2023-03-10 03:26 ` Masahiko Sawada <[email protected]> 2023-03-10 23:11 ` Melanie Plageman <[email protected]> 2023-03-11 00:34 ` Melanie Plageman <[email protected]> 2023-03-08 17:42 ` Jim Nasby <[email protected]> 2023-03-09 00:27 ` Andres Freund <[email protected]> 2023-03-09 07:47 ` John Naylor <[email protected]> 2023-03-09 13:19 ` Masahiko Sawada <[email protected]> 2023-05-19 10:58 [PATCH] Tweak xheader_width input parsing Alvaro Herrera <[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