public inbox for [email protected]
help / color / mirror / Atom feedFrom: Masahiko Sawada <[email protected]>
To: Daniel Gustafsson <[email protected]>
Cc: Melanie Plageman <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Kyotaro Horiguchi <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Amit Kapila <[email protected]>
Subject: Re: Should vacuum process config file reload more often
Date: Fri, 7 Apr 2023 15:52:49 +0900
Message-ID: <CAD21AoBS7o6Ljt_vfqPQPf67AhzKu3fR0iqk8B=vVYczMugKMQ@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<CAAKRu_Yg4-kWH_EN7wtpm3z9B+H27UTmOxzzYv1Vg=YdZ6v_rw@mail.gmail.com>
<CAD21AoCGdxYAJHmE_nXw_MFw1+Qtn3vcThRu5FiadukgQnpWeQ@mail.gmail.com>
<[email protected]>
<CAAKRu_aoa0YT36q2n3xa9MZYdepeH4Jyf2EVJj=7YdC_1c974Q@mail.gmail.com>
<CAAKRu_bSsChKsyEy1ZV2XJQ22FoDdA56efjNr9==m2TO94557w@mail.gmail.com>
<CAD21AoDi4i4tj1Cf9BG0q2iyVhHisQ2xxVBZajEqExX6t6_b3g@mail.gmail.com>
<CAAKRu_bkg+0WAGTxsy0HP75RzFbPwqz3ctYq1kThjFhSL69upw@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAAKRu_ap7nhmqtKQ1TRo3LzHG3FK=YpGO=O0019M3BbN7ysm3A@mail.gmail.com>
<[email protected]>
<CAAKRu_b1HjGCTsFpUnmwLNS8NeXJ+JnrDLhT1osP+Gq9HCU+Rw@mail.gmail.com>
<[email protected]>
<CAAKRu_bJ2DJxYoMkyMo9UpoX74LY5d2Jir86QWFUftrGKWP5Bw@mail.gmail.com>
<CA+TgmoYDYHht9zsHye3tywx7oWGMP4s_6AGefHkDsMuGHLzT7w@mail.gmail.com>
<CAAKRu_auVL_uE794zwiX6t18LJvKtJ5mrmV9tkbpWzS8AYeChQ@mail.gmail.com>
<CAAKRu_aX+MLn32kPnAQ30WChbiLTkKKmY9e8+B1ZY1BokpQZYA@mail.gmail.com>
<CAAKRu_aH-UbvNCQq-EouNUTjtGexQTRVo7ik_vYR4WUocVny7w@mail.gmail.com>
<CA+TgmoY3EaCz3_7F3+Sb3FUHonf4nxdC-_UWnn6fWEPtXU7t5g@mail.gmail.com>
<[email protected]>
<CAAKRu_YmQg_i=0DBfPzy1MMc2GjfuJFo=0rFv4fCzRQStLhjvw@mail.gmail.com>
<CAAKRu_beQECOV4WfU6nG0hhEVobCnH88Y8MQi1LrTwMfRyxuCw@mail.gmail.com>
<[email protected]>
<CAAKRu_Znk6WCCQqEw8hv3+X1Fm8gG0x4KKRAkKmO2JOcA-77-g@mail.gmail.com>
<[email protected]>
On Fri, Apr 7, 2023 at 8:08 AM Daniel Gustafsson <[email protected]> wrote:
>
> > On 7 Apr 2023, at 00:12, Melanie Plageman <[email protected]> wrote:
> >
> > On Thu, Apr 6, 2023 at 5:45 PM Daniel Gustafsson <[email protected]> wrote:
> >>
> >>> On 6 Apr 2023, at 23:06, Melanie Plageman <[email protected]> wrote:
> >>
> >>> Autovacuum workers, at the end of VacuumUpdateCosts(), check if cost
> >>> limit or cost delay have been changed. If they have, they assert that
> >>> they don't already hold the AutovacuumLock, take it in shared mode, and
> >>> do the logging.
> >>
> >> Another idea would be to copy the values to local temp variables while holding
> >> the lock, and release the lock before calling elog() to avoid holding the lock
> >> over potential IO.
> >
> > Good idea. I've done this in attached v19.
> > Also I looked through the docs and everything still looks correct for
> > balancing algo.
>
> I had another read-through and test-through of this version, and have applied
> it with some minor changes to comments and whitespace. Thanks for the quick
> turnaround times on reviews in this thread!
Cool!
Regarding the commit 7d71d3dd08, I have one comment:
+ /* Only log updates to cost-related variables */
+ if (vacuum_cost_delay == original_cost_delay &&
+ vacuum_cost_limit == original_cost_limit)
+ return;
IIUC by default, we log not only before starting the vacuum but also
when changing cost-related variables. Which is good, I think, because
logging the initial values would also be helpful for investigation.
However, I think that we don't log the initial vacuum cost values
depending on the values. For example, if the
autovacuum_vacuum_cost_delay storage option is set to 0, we don't log
the initial values. I think that instead of comparing old and new
values, we can write the log only if
message_level_is_interesting(DEBUG2) is true. That way, we don't need
to acquire the lwlock unnecessarily. And the code looks cleaner to me.
I've attached the patch (use_message_level_is_interesting.patch)
Also, while testing the autovacuum delay with relopt
autovacuum_vacuum_cost_delay = 0, I realized that even if we set
autovacuum_vacuum_cost_delay = 0 to a table, wi_dobalance is set to
true. wi_dobalance comes from the following expression:
/*
* If any of the cost delay parameters has been set individually for
* this table, disable the balancing algorithm.
*/
tab->at_dobalance =
!(avopts && (avopts->vacuum_cost_limit > 0 ||
avopts->vacuum_cost_delay > 0));
The initial values of both avopts->vacuum_cost_limit and
avopts->vacuum_cost_delay are -1. I think we should use ">= 0" instead
of "> 0". Otherwise, we include the autovacuum worker working on a
table whose autovacuum_vacuum_cost_delay is 0 to the balancing
algorithm. Probably this behavior has existed also on back branches
but I haven't checked it yet.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Attachments:
[application/octet-stream] fix.patch (584B, ../CAD21AoBS7o6Ljt_vfqPQPf67AhzKu3fR0iqk8B=vVYczMugKMQ@mail.gmail.com/2-fix.patch)
download | inline diff:
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 53c8f8d79c..2036b39ad5 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2951,8 +2951,8 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
* this table, disable the balancing algorithm.
*/
tab->at_dobalance =
- !(avopts && (avopts->vacuum_cost_limit > 0 ||
- avopts->vacuum_cost_delay > 0));
+ !(avopts && (avopts->vacuum_cost_limit >= 0 ||
+ avopts->vacuum_cost_delay >= 0));
}
heap_freetuple(classTup);
[application/octet-stream] use_message_level_is_interesting.patch (918B, ../CAD21AoBS7o6Ljt_vfqPQPf67AhzKu3fR0iqk8B=vVYczMugKMQ@mail.gmail.com/3-use_message_level_is_interesting.patch)
download | inline diff:
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 53c8f8d79c..44497f6734 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -1785,9 +1785,6 @@ FreeWorkerInfo(int code, Datum arg)
void
VacuumUpdateCosts(void)
{
- double original_cost_delay = vacuum_cost_delay;
- int original_cost_limit = vacuum_cost_limit;
-
if (MyWorkerInfo)
{
if (av_storage_param_cost_delay >= 0)
@@ -1821,16 +1818,11 @@ VacuumUpdateCosts(void)
VacuumCostBalance = 0;
}
- if (MyWorkerInfo)
+ if (MyWorkerInfo && message_level_is_interesting(DEBUG2))
{
Oid dboid,
tableoid;
- /* Only log updates to cost-related variables */
- if (vacuum_cost_delay == original_cost_delay &&
- vacuum_cost_limit == original_cost_limit)
- return;
-
Assert(!LWLockHeldByMe(AutovacuumLock));
LWLockAcquire(AutovacuumLock, LW_SHARED);
view thread (26+ 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], [email protected], [email protected], [email protected]
Subject: Re: Should vacuum process config file reload more often
In-Reply-To: <CAD21AoBS7o6Ljt_vfqPQPf67AhzKu3fR0iqk8B=vVYczMugKMQ@mail.gmail.com>
* 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