Re: relfilenode statistics

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