Change the signature of pgstat_report_vacuum() so that it's passed a Relation

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