Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
pstef opened this issue Sep 23, 2019 · 4 comments
Open

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

pstef opened this issue Sep 23, 2019 · 4 comments
Labels
database Issues or pull requests that affect the database layer

Comments

@pstef
Copy link
Contributor

pstef commented Sep 23, 2019

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
Copy link
Contributor

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

@MGatner
Copy link
Member

MGatner commented Feb 18, 2020

@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
Copy link
Contributor Author

pstef commented Feb 27, 2020

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
Copy link
Member

@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.

@kenjis kenjis added the database Issues or pull requests that affect the database layer label Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database Issues or pull requests that affect the database layer
Projects
None yet
Development

No branches or pull requests

5 participants