Date: Mon, 27 Oct 2025 11:06:26 -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: <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>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="3WukBUceFvEOzYGw"
Content-Disposition: inline
In-Reply-To: 
 <CAApHDvoM5MEHHBc0TNdrzkpq39WdEHSZhdWrtnx9zOWNXTSFGw@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/aP-YgrcPi0EhgR9x%40nathan>
Precedence: bulk


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

On Sun, Oct 26, 2025 at 02:25:48PM +1300, David Rowley wrote:
> Thanks. I've just had a look at it. A few comments and questions.

Thanks.

> 1) The subtraction here looks back to front:
> 
> + xid_age = TransactionIdIsNormal(relfrozenxid) ? relfrozenxid - recentXid : 0;
> + mxid_age = MultiXactIdIsValid(relminmxid) ? relminmxid - recentMulti : 0;

D'oh.

> 2) Would it be better to move all the code that sets the xid_score and
> mxid_score to under an "if (force_vacuum)"? Those two variables could
> be declared in there too.

Seems reasonable.

> 3) Could the following be refactored a bit so we only check the "relid
> != StatisticRelationId" condition once?

Yes.  We can update the vacuum part to follow the same pattern, too.

> 4) Should these be TransactionIds?
> 
> + uint32 xid_age;
> + uint32 mxid_age;

Probably.

> 5) Instead of:
> 
> + double score = 0.0;
> 
> Is it better to zero the score inside relation_needs_vacanalyze() so
> it works the same as the other output parameters?

My only concern about this is that some compilers might complain about
potentially-uninitialized uses.  But we can still zero it in the function
regardless.

-- 
nathan

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