Change the signature of pgstat_report_vacuum() so that it's passed a Relation
Hi hackers,
While working on relfilenode statistics, Andres suggested that we pass the Relation
to pgstat_report_vacuum() (instead of the parameters inherited from the Relation,
(See [1]/messages/by-id/ezbfcpjtfh3vtm667aegyrptcdsvinbuxw6y5p6j3e5fbffrl5@i7v6cjapz3yv)).
That looks like a good idea to me as it reduces the number of parameters and it's
consistent with pgstat_report_analyze().
PFA a patch to $SUBJECT.
Thoughts?
[1]: /messages/by-id/ezbfcpjtfh3vtm667aegyrptcdsvinbuxw6y5p6j3e5fbffrl5@i7v6cjapz3yv
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-Change-the-signature-of-pgstat_report_vacuum-so-t.patchtext/x-diff; charset=us-asciiDownload
From 3aba5eff38c4af99c18080ce06249d3dcdfbdf0b Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Tue, 16 Dec 2025 06:08:59 +0000
Subject: [PATCH v1] Change the signature of pgstat_report_vacuum() so that
it's passed a Relation
Instead of passing parameters inherited from the Relation, just pass the Relation
itself and extract the removed parameters from the Relation. That way we reduce
the number of parameters and we are consistent with pgstat_report_analyze().
Author: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Suggested-by: Andres Freund <andres@anarazel.de>
---
src/backend/access/heap/vacuumlazy.c | 3 +--
src/backend/utils/activity/pgstat_relation.c | 11 +++++------
src/include/pgstat.h | 4 ++--
3 files changed, 8 insertions(+), 10 deletions(-)
11.3% src/backend/access/heap/
63.5% src/backend/utils/activity/
25.1% src/include/
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 62035b7f9c3..30778a15639 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,
Max(vacrel->new_live_tuples, 0),
vacrel->recently_dead_tuples +
vacrel->missed_dead_tuples,
diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
index b90754f8578..55a10c299db 100644
--- a/src/backend/utils/activity/pgstat_relation.c
+++ b/src/backend/utils/activity/pgstat_relation.c
@@ -207,14 +207,13 @@ pgstat_drop_relation(Relation rel)
* Report that the table was just vacuumed and flush IO statistics.
*/
void
-pgstat_report_vacuum(Oid tableoid, bool shared,
- PgStat_Counter livetuples, PgStat_Counter deadtuples,
- TimestampTz starttime)
+pgstat_report_vacuum(Relation rel, PgStat_Counter livetuples,
+ PgStat_Counter deadtuples, TimestampTz starttime)
{
PgStat_EntryRef *entry_ref;
PgStatShared_Relation *shtabentry;
PgStat_StatTabEntry *tabentry;
- Oid dboid = (shared ? InvalidOid : MyDatabaseId);
+ Oid dboid = (rel->rd_rel->relisshared ? InvalidOid : MyDatabaseId);
TimestampTz ts;
PgStat_Counter elapsedtime;
@@ -226,8 +225,8 @@ pgstat_report_vacuum(Oid tableoid, bool shared,
elapsedtime = TimestampDifferenceMilliseconds(starttime, ts);
/* block acquiring lock for the same reason as pgstat_report_autovac() */
- entry_ref = pgstat_get_entry_ref_locked(PGSTAT_KIND_RELATION,
- dboid, tableoid, false);
+ entry_ref = pgstat_get_entry_ref_locked(PGSTAT_KIND_RELATION, dboid,
+ RelationGetRelid(rel), false);
shtabentry = (PgStatShared_Relation *) entry_ref->shared_stats;
tabentry = &shtabentry->stats;
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index f23dd5870da..6714363144a 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -669,8 +669,8 @@ extern void pgstat_init_relation(Relation rel);
extern void pgstat_assoc_relation(Relation rel);
extern void pgstat_unlink_relation(Relation rel);
-extern void pgstat_report_vacuum(Oid tableoid, bool shared,
- PgStat_Counter livetuples, PgStat_Counter deadtuples,
+extern void pgstat_report_vacuum(Relation rel, PgStat_Counter livetuples,
+ PgStat_Counter deadtuples,
TimestampTz starttime);
extern void pgstat_report_analyze(Relation rel,
PgStat_Counter livetuples, PgStat_Counter deadtuples,
--
2.34.1
On Dec 16, 2025, at 14:49, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
Hi hackers,
While working on relfilenode statistics, Andres suggested that we pass the Relation
to pgstat_report_vacuum() (instead of the parameters inherited from the Relation,
(See [1])).That looks like a good idea to me as it reduces the number of parameters and it's
consistent with pgstat_report_analyze().PFA a patch to $SUBJECT.
Thoughts?
[1]: /messages/by-id/ezbfcpjtfh3vtm667aegyrptcdsvinbuxw6y5p6j3e5fbffrl5@i7v6cjapz3yv
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
<v1-0001-Change-the-signature-of-pgstat_report_vacuum-so-t.patch>
Combing the two parameters into one looks clearer. And the function pgstat_report_vacuum() has only 1 caller, the change is a small refactoring. LGTM.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On Tue, Dec 16, 2025 at 06:49:13AM +0000, Bertrand Drouvot wrote:
While working on relfilenode statistics, Andres suggested that we pass the Relation
to pgstat_report_vacuum() (instead of the parameters inherited from the Relation,
(See [1])).That looks like a good idea to me as it reduces the number of parameters and it's
consistent with pgstat_report_analyze().
Fine by me. I can get behind the symmetry argument with
pgstat_report_analyze() for pgstat_report_vacuum(). Another
appoealing argument with this change is that it forces the callers of
pgstat_report_vacuum() to open a relation, enforcing the policy that
we need a lock of them before reporting stats. I don't think that we
will ever have dozens of callers of pgstat_report_vacuum() in the
tree, but it makes the API contract cleaner IMO with a long-term
picture in mind.
--
Michael
Hi,
On Tue, Dec 16, 2025 at 04:39:05PM +0900, Michael Paquier wrote:
On Tue, Dec 16, 2025 at 06:49:13AM +0000, Bertrand Drouvot wrote:
While working on relfilenode statistics, Andres suggested that we pass the Relation
to pgstat_report_vacuum() (instead of the parameters inherited from the Relation,
(See [1])).That looks like a good idea to me as it reduces the number of parameters and it's
consistent with pgstat_report_analyze().Fine by me.
Thank you both for looking at it!
I'm just thinking that we could mark the new "Relation rel" parameter as a
const one. Indeed we are in a "report" function that only makes use of the
Relation as read only.
But, we can't do the same for pgstat_report_analyze() because pgstat_should_count_relation()
can modify the relation through pgstat_assoc_relation(). So I'm inclined to
let it as in v1. Thoughts?
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Dec 16, 2025, at 17:45, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
Hi,
On Tue, Dec 16, 2025 at 04:39:05PM +0900, Michael Paquier wrote:
On Tue, Dec 16, 2025 at 06:49:13AM +0000, Bertrand Drouvot wrote:
While working on relfilenode statistics, Andres suggested that we pass the Relation
to pgstat_report_vacuum() (instead of the parameters inherited from the Relation,
(See [1])).That looks like a good idea to me as it reduces the number of parameters and it's
consistent with pgstat_report_analyze().Fine by me.
Thank you both for looking at it!
I'm just thinking that we could mark the new "Relation rel" parameter as a
const one. Indeed we are in a "report" function that only makes use of the
Relation as read only.But, we can't do the same for pgstat_report_analyze() because pgstat_should_count_relation()
can modify the relation through pgstat_assoc_relation(). So I'm inclined to
let it as in v1. Thoughts?
I guess you don’t have to. I search over the code base, and cannot find a “const Ration” parameter. And actually, Relation is typedef of “structure RelationData *”, so if you want to make it const, then you have to do “const structure RelationData *rel”, because “const Relation rel” won’t behave as your intention.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On Dec 16, 2025, at 17:45, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
Hi,
On Tue, Dec 16, 2025 at 04:39:05PM +0900, Michael Paquier wrote:
On Tue, Dec 16, 2025 at 06:49:13AM +0000, Bertrand Drouvot wrote:
While working on relfilenode statistics, Andres suggested that we pass the Relation
to pgstat_report_vacuum() (instead of the parameters inherited from the Relation,
(See [1])).That looks like a good idea to me as it reduces the number of parameters and it's
consistent with pgstat_report_analyze().Fine by me.
Thank you both for looking at it!
I'm just thinking that we could mark the new "Relation rel" parameter as a
const one. Indeed we are in a "report" function that only makes use of the
Relation as read only.But, we can't do the same for pgstat_report_analyze() because pgstat_should_count_relation()
can modify the relation through pgstat_assoc_relation(). So I'm inclined to
let it as in v1. Thoughts?
I guess you don’t have to. I search over the code base, and cannot find a “const Ration” parameter. And actually, Relation is typedef of “structure RelationData *”, so if you want to make it const, then you have to do “const structure RelationData *rel”, because “const Relation rel” won’t behave as your intention.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Hi,
On 2025-12-16 09:45:34 +0000, Bertrand Drouvot wrote:
On Tue, Dec 16, 2025 at 04:39:05PM +0900, Michael Paquier wrote:
On Tue, Dec 16, 2025 at 06:49:13AM +0000, Bertrand Drouvot wrote:
While working on relfilenode statistics, Andres suggested that we pass the Relation
to pgstat_report_vacuum() (instead of the parameters inherited from the Relation,
(See [1])).That looks like a good idea to me as it reduces the number of parameters and it's
consistent with pgstat_report_analyze().Fine by me.
Thank you both for looking at it!
I'm just thinking that we could mark the new "Relation rel" parameter as a
const one. Indeed we are in a "report" function that only makes use of the
Relation as read only.
-1.
But, we can't do the same for pgstat_report_analyze() because pgstat_should_count_relation()
can modify the relation through pgstat_assoc_relation(). So I'm inclined to
let it as in v1. Thoughts?
I think const markings for things like this just means more code churn or ugly
casts when it inevitably ends up not working at some point.
Greetings,
Andres Freund
On Tue, Dec 16, 2025 at 09:55:09AM -0500, Andres Freund wrote:
On 2025-12-16 09:45:34 +0000, Bertrand Drouvot wrote:
But, we can't do the same for pgstat_report_analyze() because pgstat_should_count_relation()
can modify the relation through pgstat_assoc_relation(). So I'm inclined to
let it as in v1. Thoughts?I think const markings for things like this just means more code churn or ugly
casts when it inevitably ends up not working at some point.
I'm unconvinced as well by the addition of some consts here, so I've
applied v1.
--
Michael