Re: pg_stat_statements locking

Hi,

On Mon, Sep 12, 2022 at 11:52:28AM +0500, Andrey Borodin wrote:
>
> === How to fix ===
> pgss uses LWLock to protect hashtable.
> When the hashtable is reset or new user query is inserted in pgss_store() -
> exclusive lock is used.
> When stats are updated for known query or pg_stat_statements are read - shared lock is used.
>
> I propose to swap shared lock for stats update to conditional shared lock.
> It cannot be taken during reset() - and that's totally fine. It cannot be
> taken during entering new query - and I think it's OK to spill some stats in
> this case. PFA patch attached.
>
> This patch prevents from a disaster incurred by described here locking.

I'm not a fan of that patch as it now silently ignores entries if the lwlock
can't be acquired *immediately*, without any way to avoid that if your
configuration and/or workload doesn't lead to this problem, or any way to know
that entries were ignored.

> === Other possible mitigation ===
> The incident would not occur if tuplestore did not convert into on-disk
> representation. But I don't see how to reliably protect from this.

I'm not sure that's true. If you need an exclusive lwlock it means that new
entries are added. If that keeps happening it means that you will eventually
need to defragment the query text file, and this operation will certainly hold
an exclusive lwlock for a very long time.

I think that the better long term approach is to move pg_stat_statements and
the query texts to dynamic shared memory. This should also help in this
scenario as dshash is partitioned, so you don't have a single lwlock for the
whole hash table. And as discussed recently, see [1], we should make the stat
collector extensible to reuse it in extensions like pg_stat_statements to
benefit from all the other optimizations.

[1] https://www.postgresql.org/message-id/20220818195124.c7ipzf6c5v7vxymc@awork3.anarazel.de