Blocking nature of pg_advisory_lock() can lead to Denial Of Service

[pstef]

CodeIgniter's "database driver for sessions" uses advisory locking to serialize read and write access to session data. Specifically in our case, it calls pg_advisory_lock($session_id) on session's read() callback and doesn't unlock it until the session data has been written back to the table. If multiple threads want to read data for the same session, we have a race: the first thread to acquire the lock gets full access to the session data. Each of the losers has to wait for its turn.

This is a problem, because we have a limited number of open connections between pgBouncer and the database. Each connection that is waiting for pg_advisory_lock() to return, utilizes one such connection.

The simplest solution that I can see is to use pg_try_advisory_lock() instead, so that it never waits. When it is successful, it works exactly as before. When the lock is already held, the function immediately returns false and the driver goes into read-only mode and doesn't save data on session's write().

A better, but more invasive, method is described as "auto-merging" in this article: https://www.phparch.com/2018/01/php-sessions-in-depth

[jim-parry]

This sounds like a non-trivial discussion/resolution. It should definitely be raised on the forum instead.

[MGatner]

@pstef We're pretty low on Postgres resources, any chance you would be willing to look at a PR for this? It seems like a possible important change but we are hoping to release next week so your "simplest solution" is definitely the appealing one.

[pstef]

I'd be more interested in implementing what I perceive as the better option, but then again there's the risk of my work being done all for nothing.

[lonnieezell]

@pstef I finally had a chance to read through that article. I would be interested in the auto-merging capabilities added in to our Session libraries. I think it would be a great solution that could be implemented in a BC manner, that would improve performance. I'm all for that.

Of course, it has to work with all of the handlers we specify but this seems like something that would happen on a BaseHandler level and easily integrate.