Re: relfilenode statistics

Hi,

On Thu, Oct 02, 2025 at 10:23:11AM +0900, Michael Paquier wrote:
> On Wed, Oct 01, 2025 at 02:33:11PM +0000, Bertrand Drouvot wrote:
> > I think the first step is to replace (i.e get rid) PGSTAT_KIND_RELATION by a brand
> > new PGSTAT_KIND_RELFILENODE and move all the existing stats that are currently
> > under the PGSTAT_KIND_RELATION to this new PGSTAT_KIND_RELFILENODE.
>
> Likely so, yes.

PFA the new implementation. It does not introduce a new PGSTAT_KIND_RELFILENODE,
instead it keys the PGSTAT_KIND_RELATION by relfile locator. We may want to
rename PGSTAT_KIND_RELATION to PGSTAT_KIND_RELFILENODE as a next step.

The patch is structured that way:

==== 0001

Add stats tests related to rewrite

While there are existing rewrite tests, the stats behavior during rewrites
doesn't have a good coverage. This patch adds some tests to record some stats
after different rewrite scenarios.

That way, we'll be able to test that the stats are still the ones we
expect after rewrites. Note that it generates a new stats_1.out (which is quite
large), so we may want to move those new tests to "isolation" instead.

==== 0002

Key PGSTAT_KIND_RELATION by relfile locator

This patch changes the key used for the PGSTAT_KIND_RELATION statistic kind.
Instead of the relation oid, it now relies on:

- dboid (linked to RelFileLocator's dbOid)
- objoid which is the result of a new macro (namely RelFileLocatorToPgStatObjid())
that computes an objoid based on the RelFileLocator's spcOid and the RelFileLocator's
relNumber.

That will allow us to add new stats (add writes counters) and ensure that some
counters (n_dead_tup and friends) are replicated.

The patch introduces pgstat_reloid_to_relfilelocator() to 1) avoid calling
RelationIdGetRelation() to get the relfilelocator based on the relation oid
and 2) handle the partitioned table case.

Please note that:

- when running pg_stat_have_stats('relation',...) we now need to be connected
to the database that hosts the relation. As pg_stat_have_stats() is not
documented publicly, then the changes done in 029_stats_restart.pl look
enough.

- this patch does not handle rewrites so some tests are failing. It's only
intent is to ease the review and should not be pushed without being
merged with the following patch that handles the rewrites.

- it can be used to test that stats are incremented correctly and that we're
able to retrieve them as long as rewrites are not involved.

==== 0003

handle relation statistics correctly during rewrites

Now that PGSTAT_KIND_RELATION is keyed by refilenode, we need to handle rewrites.

To do so, this patch:

- Adds PgStat_PendingRewrite, a new struct to track rewrite operations within
a transaction, storing the old locator, new locator, and original locator (for
rewrite chains). This allows stats to be copied from the original location to
the final location at commit time.

- Adds a new function, pgstat_mark_rewrite(), called when a table rewrite begins.
It records the rewrite operation in a local list and detects rewrite chains by
checking if the old_locator matches any existing new_locator, preserving the
chain's original_locator.

- Modifies pgstat_copy_relation_stats(), to accept RelFileLocators instead of
Relations, with a new increment parameter to accumulate stats (needed for rewrite
chains with DML between rewrites).

- Ensures that AtEOXact_PgStat_Relations(), AtPrepare_PgStat_Relations(),
pgstat_twophase_postcommit()/postabort() pgstat_drop_relation() handle the
PgStat_PendingRewrite list correctly.

Note that due to the new flush call in pgstat_twophase_postcommit() we can not
call GetCurrentTransactionStopTimestamp() in pgstat_relation_flush_cb(). So,
adding a check to handle this special case and call GetCurrentTimestamp() instead.
Note that we'd call GetCurrentTimestamp() only if there is a rewrite, so that
the GetCurrentTimestamp() extra cost should be negligible. Another solution
could be to trigger the flush from FinishPreparedTransaction() but that's not
worth the extra complexity.

The new pending_rewrites list is traversed in multiple places. The overhead
should be negligible in comparison to a rewrite and the list should not contain
a lot of rewrites in practice.

Another design that I tried was to copy the stats in pgstat_mark_rewrite() but
that lead to difficulties during abort, subtransactions. It looks to me that
the list approach proposed here makes more sense.

We could also imagine adding a function similar to pg_stat_have_stats() that
would take relfile locator as arguments. That could help validate that after
a rewrite the old stats are gone.

> Do you think it is OK to define non-transactional pending stats as
> being always a subset of the transactional stats? I don't quite see
> if there would be a case to have stats that are only flushed in a
> non-transactional path, while being discarded at the stats report done
> at transaction commit time. This means that it may be possible to
> structure things so as the pending non-transaction stats structure are
> always part of the transactional bits, and that the other way around
> is not possible. Perhaps that influences the design choices, at least
> a bit.

The proposed patch does not change anything it that regard.
It keeps the relation's behavior as it is.

This patch just ensure that a relation rewrite keeps its stats.

Adding new stats (add writes counters) and ensure that some counters
(n_dead_tup and friends) are replicated will be done once this one gets in.

Regards,

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