Date: Wed, 1 Apr 2026 16:28:45 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Sami Imseih <samimseih@gmail.com>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	Robert Treat <rob@xzilla.net>, satyanarlapuram@gmail.com,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Add pg_stat_autovacuum_priority
Message-ID: <ac2ODZSOjWNzueHZ@nathan>
References: 
 <CABV9wwM3nRitsgUxeCF0ywbAaLZV5jWC-9tj6WKxkdmQkHRcWg@mail.gmail.com>
 <CAA5RZ0vMp2B3UBUoqLVedy8G3u8_O8M11+Y5V7uZv3++CGYasg@mail.gmail.com>
 <CABV9wwNBifXpOjxO9rGn1HHK=DG02qApVurWSHa+rDzPriK6pA@mail.gmail.com>
 <CAA5RZ0uiBCiQDUfA5sbc50gybWx1YES3HfLKMd9gv661YpJyRg@mail.gmail.com>
 <CALj2ACVnQRS=T8SY2zegfft=Nku1n6w5609JPdd9jqiM+G_gGg@mail.gmail.com>
 <acwTxpz3Toxt0ty8@nathan>
 <CAA5RZ0uVZLsX_k33QoWt8iRes73jyqs+EjypL9+1wW1v8NsYbg@mail.gmail.com>
 <ac00otrpVnWzZ_i4@nathan>
 <CAA5RZ0vFtDKAOZALeLp3vLyhWcLntaqS-Oh36Xxgx5_8sYeSKQ@mail.gmail.com>
 <CAA5RZ0tOJjUuJidUQYAeoUD4VGjOSjuB7ox4dtLAsyoCZqSN+Q@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAA5RZ0tOJjUuJidUQYAeoUD4VGjOSjuB7ox4dtLAsyoCZqSN+Q@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/ac2ODZSOjWNzueHZ%40nathan>
Precedence: bulk

+    bool        needs_vacuum;   /* threshold exceeded for vacuum */
+    bool        needs_analyze;  /* threshold exceeded for analyze */
+    bool        is_wraparound;  /* at risk of XID/MXID wraparound */

I've been thinking about whether to also return whether autovacuum is
enabled in the view, i.e., AutoVacuumingActive() && av_enabled.  My
instinct was that would help explain why autovacuum isn't picking up tables
that are eligible for autovacuum based on the view.  One wrinkle is that
it'd report false even when autovacuum is working on a table for wraparound
prevention.  I'm also not especially excited about further complicating
this stuff for folks who disable autovacuum.  Furthermore, the value of the
reloptions and autovacuum GUC are discoverable elsewhere.  So, I'm
currently leaning towards leaving that information out for now.

+        if (vactuples > vacthresh)
+        {
+            scores->needs_vacuum = true;
+            if (av_enabled && AutoVacuumingActive())
                 *dovacuum = true;
         }

nitpick:  We might be able to simplify this a bit by 1) storing "av_enabled
&& AutoVacuumingActive()" in a variable and 2) reworking the code to look
more like this:

	scores->needs_vacuum = (vactuples > vacthresh);
	*do_vacuum |= (av_enabled && scores->needs_vacuum);

... but others might find your version easier to read.

Otherwise, 0001 looks good.

In 0003, I think you missed renaming the last argument to
compute_autovac_score() in table_recheck_autovac().

I didn't see anything else in this read-through.  I'm planning to start
preparing this for commit tomorrow.

-- 
nathan