Date: Tue, 28 Oct 2025 16:03:23 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: David Rowley <dgrowleyml@gmail.com>
Cc: Sami Imseih <samimseih@gmail.com>, Robert Haas <robertmhaas@gmail.com>,
	Jeremy Schneider <schneider@ardentperf.com>,
	pgsql-hackers@postgresql.org
Subject: Re: another autovacuum scheduling thread
Message-ID: <aQEvm40W3aVizp5Q@nathan>
References: 
 <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>
 <CAApHDvpOq09uVq7aXcuSBPAhZBTfAL-m2c4FOF2PphFe-YcnRg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="skXIAVxeHUwbMCMJ"
Content-Disposition: inline
In-Reply-To: 
 <CAApHDvpOq09uVq7aXcuSBPAhZBTfAL-m2c4FOF2PphFe-YcnRg@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/aQEvm40W3aVizp5Q%40nathan>
Precedence: bulk


--skXIAVxeHUwbMCMJ
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Oct 28, 2025 at 11:47:08AM +1300, David Rowley wrote:
> 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;

Removed.

> 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 <nathandbossart@gmail.com> 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.

This is lifted from vacuum_xid_failsafe_check().  As noted in the docs, the
failsafe settings are silently limited to 105% of *_freeze_max_age.  I
expanded on this in the comment atop these lines.

> 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.

Done.

-- 
nathan

--skXIAVxeHUwbMCMJ
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename=v6-0001-autovacuum-scheduling-improvements.patch