On Thu, Nov 7, 2024 at 11:17 AM Michael Paquier <mich...@paquier.xyz> wrote: > The comment could be simpler, say a "The spinlock is not required when > reading these two as they are always updated when holding pgss->lock > exclusively.". Or something like that.
Thank you for your feedback and the shorter wording of the comment. I used it in the new version of the patch. Best regards, Karina Litskevich Postgres Professional: http://postgrespro.com/
From 4b7f7ea0c9923fa9d2622d86aa1214f331c600ec Mon Sep 17 00:00:00 2001 From: Karina Litskevich <litskevichkar...@gmail.com> Date: Thu, 7 Nov 2024 15:54:32 +0300 Subject: [PATCH v2] pg_stat_statements: Avoid excessive lock holding Entry's mutex spinlock is used to be able to modify counters without holding pgss->lock exclusively (though holding it at least shared so the entry doesn't disappear). We never actually modify stats_since and minmax_stats_since without holding pgss->lock exclusively, so there is no need to hold entry's mutex spinlock when reading stats_since and minmax_stats_since. This also restores the consistency between the code and the comments about entry's mutex spinlock usage. --- contrib/pg_stat_statements/pg_stat_statements.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 1798e1d016..49c657b3e0 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -1869,9 +1869,14 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo, /* copy counters to a local variable to keep locking time short */ SpinLockAcquire(&entry->mutex); tmp = entry->counters; + SpinLockRelease(&entry->mutex); + + /* + * The spinlock is not required when reading these two as they are + * always updated when holding pgss->lock exclusively. + */ stats_since = entry->stats_since; minmax_stats_since = entry->minmax_stats_since; - SpinLockRelease(&entry->mutex); /* Skip entry if unexecuted (ie, it's a pending "sticky" entry) */ if (IS_STICKY(tmp)) -- 2.34.1
