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
From d0aae1f873ec4f0cde91d851d131767e7ca66171 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Fri, 27 Dec 2024 09:16:02 +0900 Subject: [PATCH] injection_points: Tweak variable-numbered stats to work with pending data As coded, the module was not using pending entries to store its data locally before a flush to the central dshash is done with a timed pgstat_report_stat() call. Hence, the flush callback was defined, but finished by being not used. As a template, this is more efficient than the original logic of updating directly the shared memory entries as this reduces the interactions that need to be done with the pgstats hash table in shared memory. injection_stats_flush_cb() was also missing a pgstat_unlock_entry(), so add one, while on it. --- .../modules/injection_points/injection_stats.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/test/modules/injection_points/injection_stats.c b/src/test/modules/injection_points/injection_stats.c index e16b9db284..21d5c10f39 100644 --- a/src/test/modules/injection_points/injection_stats.c +++ b/src/test/modules/injection_points/injection_stats.c @@ -80,6 +80,9 @@ injection_stats_flush_cb(PgStat_EntryRef *entry_ref, bool nowait) return false; shfuncent->stats.numcalls += localent->numcalls; + + pgstat_unlock_entry(entry_ref); + return true; } @@ -127,13 +130,13 @@ pgstat_create_inj(const char *name) if (!inj_stats_loaded || !inj_stats_enabled) return; - entry_ref = pgstat_get_entry_ref_locked(PGSTAT_KIND_INJECTION, InvalidOid, - PGSTAT_INJ_IDX(name), false); + entry_ref = pgstat_prep_pending_entry(PGSTAT_KIND_INJECTION, InvalidOid, + PGSTAT_INJ_IDX(name), NULL); + shstatent = (PgStatShared_InjectionPoint *) entry_ref->shared_stats; /* initialize shared memory data */ memset(&shstatent->stats, 0, sizeof(shstatent->stats)); - pgstat_unlock_entry(entry_ref); } /* @@ -168,16 +171,14 @@ pgstat_report_inj(const char *name) if (!inj_stats_loaded || !inj_stats_enabled) return; - entry_ref = pgstat_get_entry_ref_locked(PGSTAT_KIND_INJECTION, InvalidOid, - PGSTAT_INJ_IDX(name), false); + entry_ref = pgstat_prep_pending_entry(PGSTAT_KIND_INJECTION, InvalidOid, + PGSTAT_INJ_IDX(name), NULL); shstatent = (PgStatShared_InjectionPoint *) entry_ref->shared_stats; statent = &shstatent->stats; /* Update the injection point statistics */ statent->numcalls++; - - pgstat_unlock_entry(entry_ref); } /* -- 2.45.2
signature.asc
Description: PGP signature
