Date: Tue, 16 Dec 2025 10:22:06 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Andres Freund <andres@anarazel.de>
Cc: Michael Paquier <michael@paquier.xyz>,
	Kirill Reshke <reshkekirill@gmail.com>,
	Robert Haas <robertmhaas@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: relfilenode statistics
Message-ID: <aUEyzoOJtrCLAEeT@ip-10-97-1-34.eu-west-3.compute.internal>
References: <aNutZbziBycskOYR@ip-10-97-1-34.eu-west-3.compute.internal>
 <aNxiLG0JTCYnpqx6@paquier.xyz>
 <aN07p71KNFR2HdaD@ip-10-97-1-34.eu-west-3.compute.internal>
 <aN3T_5NBv0XKFbs7@paquier.xyz>
 <aQ3X20hbqoThQXgp@ip-10-97-1-34.eu-west-3.compute.internal>
 <aQ_TYhsZG72QkS_A@paquier.xyz>
 <aRGoGcOdutTHQfpn@paquier.xyz>
 <aRS9+yaGhjDslRWj@ip-10-97-1-34.eu-west-3.compute.internal>
 <aUA3XkU9hc1xrtRB@ip-10-97-1-34.eu-west-3.compute.internal>
 <ezbfcpjtfh3vtm667aegyrptcdsvinbuxw6y5p6j3e5fbffrl5@i7v6cjapz3yv>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="tV2Nti+lXhoeT6I5"
Content-Disposition: inline
In-Reply-To: <ezbfcpjtfh3vtm667aegyrptcdsvinbuxw6y5p6j3e5fbffrl5@i7v6cjapz3yv>
Archived-At: 
 <https://www.postgresql.org/message-id/aUEyzoOJtrCLAEeT%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk


--tV2Nti+lXhoeT6I5
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hi,

On Mon, Dec 15, 2025 at 12:48:25PM -0500, Andres Freund wrote:
> On 2025-12-15 16:29:18 +0000, Bertrand Drouvot wrote:
> > From 7908ba56cb8b6255b869af6be13077aa0315d5f1 Mon Sep 17 00:00:00 2001
> 
> I think this needs to make more explicit that this works because the object ID
> now is a uint64, and that both the inputs are 32 bits.

Yeah, it's now added in the commit message (mentioning b14e9ce7d55c).

> > diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
> > index 62035b7f9c3..a9b2b4e1033 100644
> > --- a/src/backend/access/heap/vacuumlazy.c
> > +++ b/src/backend/access/heap/vacuumlazy.c
> > @@ -961,8 +961,7 @@ heap_vacuum_rel(Relation rel, const VacuumParams params,
> >  	 * soon in cases where the failsafe prevented significant amounts of heap
> >  	 * vacuuming.
> >  	 */
> > -	pgstat_report_vacuum(RelationGetRelid(rel),
> > -						 rel->rd_rel->relisshared,
> > +	pgstat_report_vacuum(rel->rd_locator,
> >  						 Max(vacrel->new_live_tuples, 0),
> >  						 vacrel->recently_dead_tuples +
> >  						 vacrel->missed_dead_tuples,
> 
> Why not pass in the Relation itself? Given that we do that already for
> pgstat_report_analyze(), it seems like that'd be an improvement even
> independent of this change?

Makes sense, done in [1].

> > diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
> > index 1bd3924e35e..563a3697690 100644
> > --- a/src/backend/postmaster/autovacuum.c
> > +++ b/src/backend/postmaster/autovacuum.c
> > @@ -2048,8 +2048,7 @@ do_autovacuum(void)
> >  
> >  		/* Fetch reloptions and the pgstat entry for this table */
> >  		relopts = extract_autovac_opts(tuple, pg_class_desc);
> > -		tabentry = pgstat_fetch_stat_tabentry_ext(classForm->relisshared,
> > -												  relid);
> > +		tabentry = pgstat_fetch_stat_tabentry_ext(relid);
> >  
> >  		/* Check if it needs vacuum or analyze */
> >  		relation_needs_vacanalyze(relid, relopts, classForm, tabentry,
> 
> I don't think this is good - now do_autovacuum() will do a separate syscache
> lookup to fetch information the caller already has (due to the
> pgstat_reloid_to_relfilelocator() in pgstat_fetch_stat_tabentry_ext()). That's
> not too bad for things like viewing stats, but do_autovacuum() does this for
> every table in a database...

Good point. In the attached I added pgstat_fetch_stat_tabentry_by_locator().
It's called directly in do_autovacuum() and also in pgstat_fetch_stat_tabentry_ext().

I did not check if there are other places where we can call pgstat_fetch_stat_tabentry_by_locator()
directly. I want first to validate this idea makes sense, does it?

> I don't think this is true as stated. Two reasons:
> 
> 1) This afaict guarantees that the relfilenode will not class with oids, but
>    it does *NOT* guarantee that it does not clash with other relfilenodes

> 2) Note that GetNewRelFileNumber() does *NOT* check for conflicts when
>    creating a new relfilenode for an existing relation:
>  * If the relfilenumber will also be used as the relation's OID, pass the
>  * opened pg_class catalog, and this routine will guarantee that the result
>  * is also an unused OID within pg_class.  If the result is to be used only
>  * as a relfilenumber for an existing relation, pass NULL for pg_class.

Oh right, in case of OID wraparound. In the attached I added a new 

"
#define PSEUDO_PARTITION_TABLE_SPCOID 1665
"

to ensure uniqueness then.

[1]: https://www.postgresql.org/message-id/flat/aUEA6UZZkDCQFgSA%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

--tV2Nti+lXhoeT6I5
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v9-0001-Key-PGSTAT_KIND_RELATION-by-relfile-locator.patch"