public inbox for [email protected]
help / color / mirror / Atom feedFrom: Daniel Gustafsson <[email protected]>
To: Masahiko Sawada <[email protected]>
Cc: Melanie Plageman <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Amit Kapila <[email protected]>
Subject: Re: Should vacuum process config file reload more often
Date: Thu, 23 Mar 2023 17:24:36 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAD21AoAJ6medGOeLo4CLkySSPba7yt9UphD-1Sxx+4LY-ixoqw@mail.gmail.com>
References: <CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7+DXhrjDA0gw@mail.gmail.com>
<CAD21AoC=b09pzWNyPYuti0M2DQwy57CGF9UWWb_+v=gdWeNG=A@mail.gmail.com>
<CAAKRu_bR2VrTUskR24GkXV15SxPaCiHYD=soOuWZcT-rGDcA8Q@mail.gmail.com>
<CAD21AoB=EC6wzjjqDXKmNEhu_BhGg0g_Gc50yh_zvXqiDC_OSg@mail.gmail.com>
<CAAKRu_buP5wzsho3qNw5o9_R0pF69FRM5hgCmr-mvXmGXwdA7A@mail.gmail.com>
<CAAKRu_YWNKs2=mE27C1W8VxPJa1UR+YVuckw5kYzWowTStCHiQ@mail.gmail.com>
<CAD21AoAB-jtKvYd-QbnhbanoY+-PDCXvrX-JhBUqGFrB6e7eTw@mail.gmail.com>
<CAAKRu_Zir77=EAQxytN4Bc-P-==1YLC9KiK=kqPricjVzac8ng@mail.gmail.com>
<CAD21AoCBqrzCqXPGjhVHJwJThYW_CmBOpWL2J_6Bb_4qMwcsuw@mail.gmail.com>
<CAAKRu_YxDh-31zzF9vA=6FfggE8-mQo8bwxCxaFqY=pb8kTTag@mail.gmail.com>
<CAD21AoC=BRMwUpVDf1tzDa13--dLhuq=c2XQ_KAYA4Qx3cS=kA@mail.gmail.com>
<CAAKRu_Z4NdHk6EofruFzrasO10bzQYCNuvZ=Rri1wJ6vwoV4-g@mail.gmail.com>
<CAD21AoAJ6medGOeLo4CLkySSPba7yt9UphD-1Sxx+4LY-ixoqw@mail.gmail.com>
> On 23 Mar 2023, at 07:08, Masahiko Sawada <[email protected]> wrote:
> On Sun, Mar 19, 2023 at 7:47 AM Melanie Plageman <[email protected]> wrote:
> It makes sense to me that we need to reload the config file even when
> vacuum-delay is disabled. But I think it's not convenient for users
> that we don't reload the configuration file once the failsafe is
> triggered. I think users might want to change some GUCs such as
> log_autovacuum_min_duration.
I agree with this.
>> On an unrelated note, I was wondering if there were any docs anywhere
>> that should be updated to go along with this.
>
> The current patch improves the internal mechanism of (re)balancing
> vacuum-cost but doesn't change user-visible behavior. I don't have any
> idea so far that we should update somewhere in the doc.
I had a look as well and can't really spot anywhere where the current behavior
is detailed, so there is little to update. On top of that, I also don't think
it's worth adding this to the docs.
>> And, I was wondering if it was worth trying to split up the part that
>> reloads the config file and all of the autovacuum stuff. The reloading
>> of the config file by itself won't actually result in autovacuum workers
>> having updated cost delays because of them overwriting it with
>> wi_cost_delay, but it will allow VACUUM to have those updated values.
>
> It makes sense to me to have changes for overhauling the rebalance
> mechanism in a separate patch.
It would for sure be worth considering,
+bool VacuumFailsafeActive = false;
This needs documentation, how it's used and how it relates to failsafe_active
in LVRelState (which it might replace(?), but until then).
+ pg_atomic_uint32 nworkers_for_balance;
This needs a short oneline documentation update to the struct comment.
- double wi_cost_delay;
- int wi_cost_limit;
- int wi_cost_limit_base;
This change makes the below comment in do_autovacuum in need of an update:
/*
* Remove my info from shared memory. We could, but intentionally.
* don't, clear wi_cost_limit and friends --- this is on the
* assumption that we probably have more to do with similar cost
* settings, so we don't want to give up our share of I/O for a very
* short interval and thereby thrash the global balance.
*/
+ if (av_table_option_cost_delay >= 0)
+ VacuumCostDelay = av_table_option_cost_delay;
+ else
+ VacuumCostDelay = autovacuum_vac_cost_delay >= 0 ?
+ autovacuum_vac_cost_delay : VacuumCostDelay;
While it's a matter of personal preference, I for one would like if we reduced
the number of ternary operators in the vacuum code, especially those mixed into
if statements. The vacuum code is full of this already though so this isn't
less of an objection (as it follows style) than an observation.
+ * note: in cost_limit, zero also means use value from elsewhere, because
+ * zero is not a valid value.
...
+ int vac_cost_limit = autovacuum_vac_cost_limit > 0 ?
+ autovacuum_vac_cost_limit : VacuumCostLimit;
Not mentioning the fact that a magic value in a GUC means it's using the value
from another GUC (which is not great IMHO), it seems we are using zero as well
as -1 as that magic value here? (not introduced in this patch.) The docs does
AFAICT only specify -1 as that value though. Am I missing something or is the
code and documentation slightly out of sync?
I need another few readthroughs to figure out of VacuumFailsafeActive does what
I think it does, and should be doing, but in general I think this is a good
idea and a patch in good condition close to being committable.
--
Daniel Gustafsson
view thread (11+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Should vacuum process config file reload more often
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox