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
