On 2025-May-01, Tom Lane wrote:
> One other thing that comes to mind is that pg_stat_statements
> has stretched the intention of "short straight-line code segment"
> to the point of unrecognizability. Maybe it needs to stop using
> spinlocks to protect pgssEntry updates. But I'm not sure if that
> would move the needle on whether ISB is a good idea or not.
Yeah, it looks like pgss_store() is being too generous on the amount of
code run with that spinlock held. However, changing that spinlock to an
lwlock doesn't look easy, because of the way each pgss entry is created
as a dynahash entry, and then deallocated from there. With spinlocks we
can just reinit the spinlock each time, but that doesn't work with
lwlocks. We have no easy way to associate then disassociate each entry
from a specific lwlock.
Maybe we'd need to do
RequestNamedLWLockTranche("pg_stat_statements", 1 + pgss_max)
at startup, then put them all in an lwlock-freelist: a list of unused
lwlocks in this tranche, to take one on entry_alloc and put it back on
entry_dealloc.
I think Salvatore's comment that enabling track_planning makes the
performance gain more pronounced is evidence that changing this spinlock
to lwlock would be beneficial.
Another benefit of using an lwlock: pg_stat_statements_internal() could
use shared mode to read each entry. Probably not as critical (because
it requires the view to be read), but it'd reduce the performance hit
when the user is reading the view.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"I can't go to a restaurant and order food because I keep looking at the
fonts on the menu. Five minutes later I realize that it's also talking
about food" (Donald Knuth)
