public inbox for [email protected]
help / color / mirror / Atom feedFrom: David Rowley <[email protected]>
To: Nathan Bossart <[email protected]>
Cc: Sami Imseih <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Jeremy Schneider <[email protected]>
Cc: [email protected]
Subject: Re: another autovacuum scheduling thread
Date: Tue, 28 Oct 2025 11:47:08 +1300
Message-ID: <CAApHDvpOq09uVq7aXcuSBPAhZBTfAL-m2c4FOF2PphFe-YcnRg@mail.gmail.com> (raw)
In-Reply-To: <aP-YgrcPi0EhgR9x@nathan>
References: <CAApHDvqobtKMwJbhKB_c=3-TM=TgS3bcuvzcWMm3ee1c0mz9hw@mail.gmail.com>
<aPkz9grlBAJRXrbe@nathan>
<CAA5RZ0vSPqd5vP4-17E6QELRgQzaoKChgp5TDPK9GhZEK=0Gjg@mail.gmail.com>
<aPp4VyLo2Zqk7oCV@nathan>
<CAA5RZ0sfQ-VSCSafsrvyJ7wsW1utLwtPVJ5N6hB0726BGRDrgQ@mail.gmail.com>
<CAApHDvpxE8ci83d02dRE3-fMetb4Dc89-80FrjkGDz2q+ByJog@mail.gmail.com>
<CAA5RZ0upTpKqgrdNfMSX7UJdjx=+=CsQ6Xct+vcCZPvUVhdZvw@mail.gmail.com>
<CAApHDvp1=FOs6GneTzLSCHnCmC7z1_80=U3M=CKd82-pwS3YHg@mail.gmail.com>
<aPuWev3D9M4iGCUt@nathan>
<CAApHDvoM5MEHHBc0TNdrzkpq39WdEHSZhdWrtnx9zOWNXTSFGw@mail.gmail.com>
<aP-YgrcPi0EhgR9x@nathan>
The patch is starting to look good. Here's a review of v5:
1. I think the following code at the bottom of
relation_needs_vacanalyze() can be deleted. You've added the check to
ensure *doanalyze never gets set to true for pg_statistic.
/* ANALYZE refuses to work with pg_statistic */
if (relid == StatisticRelationId)
*doanalyze = false;
2. As #1, but in recheck_relation_needs_vacanalyze(), the following I
think can now be removed:
/* ignore ANALYZE for toast tables */
if (classForm->relkind == RELKIND_TOASTVALUE)
*doanalyze = false;
3. Would you be able to include what the idea behind the * 1.05 in the
preceding comment?
On Tue, 28 Oct 2025 at 05:06, Nathan Bossart <[email protected]> wrote:
> + effective_xid_failsafe_age = Max(vacuum_failsafe_age,
> + autovacuum_freeze_max_age * 1.05);
> + effective_mxid_failsafe_age = Max(vacuum_multixact_failsafe_age,
> + autovacuum_multixact_freeze_max_age * 1.05);
I assume it's to workaround some strange configuration settings, but
don't know for sure, or why 1.05 is a good value.
4. I think it might be neater to format the following as 3 separate "if" tests:
> + if (force_vacuum ||
> + vactuples > vacthresh ||
> + (vac_ins_base_thresh >= 0 && instuples > vacinsthresh))
> + {
> + *dovacuum = true;
> + *score = Max(*score, (double) vactuples / Max(vacthresh, 1));
> + if (vac_ins_base_thresh >= 0)
> + *score = Max(*score, (double) instuples / Max(vacinsthresh, 1));
> + }
> + else
> + *dovacuum = false;
i.e:
if (force_vacuum)
*dovacuum = true;
if (vactuples > vacthresh)
{
*dovacuum = true;
*score = Max(*score, (double) vactuples / Max(vacthresh, 1));
}
if (vac_ins_base_thresh >= 0 && instuples > vacinsthresh)
{
*dovacuum = true;
*score = Max(*score, (double) instuples / Max(vacinsthresh, 1));
}
and also get rid of all the "else *dovacuum = false;" (and *dovacuum =
false) in favour of setting those to false at the top of the function.
It's just getting harder to track that those parameters are getting
set in all cases when they're meant to be.
doing that also gets rid of the duplicative "if (vac_ins_base_thresh
>= 0)" check and also saves doing the score calc when the inputs to it
don't make sense. The current code is relying on Max always picking
the current *score when the threshold isn't met.
David
view thread (143+ 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: another autovacuum scheduling thread
In-Reply-To: <CAApHDvpOq09uVq7aXcuSBPAhZBTfAL-m2c4FOF2PphFe-YcnRg@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