Hi,
Isn't pgstat_lock_flush_cb a bit broken with nowait=true? It'll skip
flushing stats for that particular lock type, but then it'll happily
reset the pending stats anyway, forgetting the stats.
AFAIK it should keep the pending stats, and flush them sometime lager,
when the lock is not contended. That's what the other flush callbacks
do, at least. This probably means it needs to reset the entries one by
one, not the whole struct at once.
TBH I'm rather skeptical about having one lock per entry. Sure, it
allows two backends to write different entries concurrently. But is it
actually worth it? With nowait=true it might even be cheaper to try with
a single lock, and AFAICS that's the case where it matters.
I wouldn't be surprised if this behaved quite poorly with contended
cases, because the backends will be accessing the locks in exactly the
same order and synchronize. So if one lock is contended, won't that
"synchronize" the backends, making the other locks contended too?
Has anyone tested it actually improves the behavior? I only quickly
skimmed the thread, I might have missed it ...
If per-entry locks help, maybe a good micro-optimization would be to
check if there even is anything to sync before acquiring the lock. I
mean, if (waits==0), why obtain the lock?
regards
--
Tomas Vondra
