Fix handling of injection_points regarding pending stats
Hi all,
While doing more stuff related to pgstats and WAL, I have bumped on
the fact that the module injection_points uses for its
variable-numbered stats a pending flush callback but it does not
handle its entries so as these would be handled locally first, with a
final flush timed by pgstat_report_stat() that would itself call the
pending flush callback. This is a less efficient practice, because it
requires to touch the pgstats dshash more often than necessary.
This is caused by the use of pgstat_get_entry_ref_locked() rather than
pgstat_prep_pending_entry() which would make sure to time the flush of
any pending entries appropriately depending on pgstat_report_stat().
I'd like to fix the module so as the flush timing is controlled like
all the other stats kinds, which is a better and more efficient
practice to have anyway as this code should stand as a template for
custom pgstats kinds.
While doing so, I have also noticed that injection_stats_flush_cb()
missed a pgstat_unlock_entry() when entries get locked.
All that is addressed in the patch attached. Feel free if you have
any comments.
Thanks,
--
Michael
Attachments:
Hi,
On Fri, Dec 27, 2024 at 09:23:12AM +0900, Michael Paquier wrote:
Hi all,
While doing more stuff related to pgstats and WAL, I have bumped on
the fact that the module injection_points uses for its
variable-numbered stats a pending flush callback but it does not
handle its entries so as these would be handled locally first, with a
final flush timed by pgstat_report_stat() that would itself call the
pending flush callback. This is a less efficient practice, because it
requires to touch the pgstats dshash more often than necessary.This is caused by the use of pgstat_get_entry_ref_locked() rather than
pgstat_prep_pending_entry() which would make sure to time the flush of
any pending entries appropriately depending on pgstat_report_stat().
I'd like to fix the module so as the flush timing is controlled like
all the other stats kinds, which is a better and more efficient
practice to have anyway as this code should stand as a template for
custom pgstats kinds.
Agree that it makes sense to rewrite this part as done in this patch (as it
reflects better how things "should" be done).
While doing so, I have also noticed that injection_stats_flush_cb()
missed a pgstat_unlock_entry() when entries get locked.
Right.
All that is addressed in the patch attached. Feel free if you have
any comments.
The patch is pretty straightforward and LGTM.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Mon, Dec 30, 2024 at 08:34:54AM +0000, Bertrand Drouvot wrote:
On Fri, Dec 27, 2024 at 09:23:12AM +0900, Michael Paquier wrote:
All that is addressed in the patch attached. Feel free if you have
any comments.The patch is pretty straightforward and LGTM.
Thanks for double-checking.
--
Michael