MIME-Version: 1.0
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>
In-Reply-To: <aP-YgrcPi0EhgR9x@nathan>
From: David Rowley <dgrowleyml@gmail.com>
Date: Tue, 28 Oct 2025 11:47:08 +1300
Message-ID: 
 <CAApHDvpOq09uVq7aXcuSBPAhZBTfAL-m2c4FOF2PphFe-YcnRg@mail.gmail.com>
Subject: Re: another autovacuum scheduling thread
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Sami Imseih <samimseih@gmail.com>, Robert Haas <robertmhaas@gmail.com>,
	Jeremy Schneider <schneider@ardentperf.com>, pgsql-hackers@postgresql.org
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAApHDvpOq09uVq7aXcuSBPAhZBTfAL-m2c4FOF2PphFe-YcnRg%40mail.gmail.com>
Precedence: bulk

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

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