Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Change pgstat_report_vacuum() to use Relation

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

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-12-16T06:49:13Z

    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]: https://www.postgresql.org/message-id/ezbfcpjtfh3vtm667aegyrptcdsvinbuxw6y5p6j3e5fbffrl5@i7v6cjapz3yv

Regards,

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

  2. Re: Change the signature of pgstat_report_vacuum() so that it's passed a Relation

    Chao Li <li.evan.chao@gmail.com> — 2025-12-16T07:00:11Z

    
> 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]: https://www.postgresql.org/message-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/

  3. Re: Change the signature of pgstat_report_vacuum() so that it's passed a Relation

    Michael Paquier <michael@paquier.xyz> — 2025-12-16T07:39:05Z

    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

  4. Re: Change the signature of pgstat_report_vacuum() so that it's passed a Relation

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-12-16T09:45:34Z

    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

  5. Re: Change the signature of pgstat_report_vacuum() so that it's passed a Relation

    Chao Li <li.evan.chao@gmail.com> — 2025-12-16T09:56:20Z

    
> 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/

  6. Re: Change the signature of pgstat_report_vacuum() so that it's passed a Relation

    Chao Li <li.evan.chao@gmail.com> — 2025-12-16T13:32:23Z

    
> 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/

  7. Re: Change the signature of pgstat_report_vacuum() so that it's passed a Relation

    Andres Freund <andres@anarazel.de> — 2025-12-16T14:55:09Z

    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

  8. Re: Change the signature of pgstat_report_vacuum() so that it's passed a Relation

    Michael Paquier <michael@paquier.xyz> — 2025-12-17T02:48:44Z

    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