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

Document issues with PgBouncer and session-level Advisory Locks #52

Closed
bensheldon opened this issue Jul 23, 2020 · 4 comments
Closed

Document issues with PgBouncer and session-level Advisory Locks #52

bensheldon opened this issue Jul 23, 2020 · 4 comments
Labels
documentation Improvements or additions to documentation

Comments

@bensheldon
Copy link
Owner

bensheldon commented Jul 23, 2020

Reference: https://www.samu.space/distributed-locking-with-postgres-advisory-locks/#types-of-advisory-locks

Also, consider offering option of transaction-level advisory lock.

Why use PgBouncer:

when you have hundreds of workers accessing a DB but the DB only has a small connection limit, say 200 or something. You can have the workers access the connection through pgBouncer to bring that up into the thousands.

@bensheldon bensheldon added the documentation Improvements or additions to documentation label Aug 11, 2020
@mockdeep
Copy link

mockdeep commented Mar 5, 2021

We don't use PgBouncer, but we probably should be. I wouldn't want to preclude it as an option, at any rate. It would be helpful to understand the constraints more, and whether they are a necessary aspect of GoodJob or more incidental. From the article you linked:

If you use PgBouncer in session-pooling mode, both advisory lock types are available to you

So maybe GoodJob isn't completely incompatible with PgBouncer?

@bensheldon
Copy link
Owner Author

I have a bit of a blind spot about PgBouncer because I don't use it on any projects. But my understanding is that the default and most-beneficial mode is transaction-based pooling. Heroku's new platform pooling feature is transaction based too.

Why is GoodJob currently incompatible with transaction-based connection pooling? GoodJob currently uses a session-based advisory-lock that persists for the duration of the job execution.

I think there are two clear paths for PgBouncer compatibility, each of which have their downsides:

  • Offer a transaction-based Advisory Lock as an optional feature. This would be a low/moderate-effort change to GoodJob to change the sql to a transaction-based advisory lock and wrap job execution in a transaction. The downside is that the entire job execution would be wrapped in a potentially long-running transaction, which is bad enough, in addition to theoretically reducing the overall benefit of transaction-based connection-pooling because it's essentially treating a transaction like a session. So more like a workaround than something I consider first class.
  • Change the entire locking model of GoodJob to using a locked_at/locked_by column and FOR UPDATE SKIP LOCKED (SKIP LOCKED #65) instead of an advisory lock. This is a high effort change to figure out a reliable strategy for gracefully handling SIGKILL and not orphaning locked jobs. But this also is potentially what might be necessary if a DBA who knows better than me says that the current CTE-based advisory-lock query is problematic (Question about locking internals #212).

@sudhirj
Copy link

sudhirj commented Mar 11, 2021

The third option is to allow Rails to use DATABASE_CONNECTION_POOL_URL while GoodJob always uses DATABASE_URL. I think this will handle pretty much everything. Any time I use a connection pooler, I maintain two URLs, one with pooling and one without. If GoodJob can accept an override for the parameters in database.yml, we can just point it at the OG database to bypass the pooling.

@bensheldon
Copy link
Owner Author

GoodJob now supports setting GoodJob.active_record_parent_class. Together with Rails 6's multiple database support it's now possible to configure a database connection for the non-pooled database connection, create an ActiveRecord base class (e.g. UnpooledRecord) and configure GoodJob to use that base class (GoodJob.active_record_parent_class = "UnpooledRecord")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

3 participants