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

Allow Migration Advisory Locks to be disabled. #31190

Closed
tgxworld opened this issue Nov 21, 2017 · 25 comments
Closed

Allow Migration Advisory Locks to be disabled. #31190

tgxworld opened this issue Nov 21, 2017 · 25 comments

Comments

@tgxworld
Copy link
Contributor

@tgxworld tgxworld commented Nov 21, 2017

Related: #22122

Migration Advisory Locks for Postgres does not work well when used with PgBouncer in Transaction pooling mode. We've been running into advisory locks being left in the DB and would like to turn this feature off completely as we have no need for the protection since we don't run migrations concurrently. I was wondering if the Rails team is open to a PR that adds a config allowing this to be disabled.

System configuration

Rails version: 5.1

Ruby version: 2.4.2

@matthewd
Copy link
Member

@matthewd matthewd commented Nov 21, 2017

Yep, seems reasonable. It should probably be configured in database.yml.

@tgxworld
Copy link
Contributor Author

@tgxworld tgxworld commented Nov 21, 2017

I was thinking more along the line of a config active_record.config.raise_error_on_concurrent_migration = (false|true)? The config would default to false. Users who are doing concurrent migrations on MySQL or PG databases would have to turn it on. My feeling on this is that most people don't run migration concurrently and I don't think that is even recommended? At least for Postgres, migrations are done in a ddl_transaction so I'm not quite sure why the protection is even needed.

@matthewd
Copy link
Member

@matthewd matthewd commented Nov 21, 2017

The config would default to false

No longer reasonable.

Most people don't run migrations through PgBouncer.

At least for Postgres, migrations are done in a ddl_transaction so I'm [not] quite sure why the protection is even needed

.. you linked to the PR which links to the issue which describes the problem it's solving 😉

It's not about enabling people to run migrations concurrently, it's a safety measure to enforce that people aren't running migrations concurrently -- because it's very easy for a deploy script, say, to blindly db:migrate without being aware that it's part of a cluster / other servers might be doing the same thing.

And as a safety measure, it's not very useful if people have to opt in to it: people who are thinking about this as a possible issue, probably don't need it.


database.yml is where you apply other pooling-relevant configuration, like disabling prepared statements.. so to me this seems like a good fit for that. Mostly, it seems the most compatible with guidance that says "if you're using PgBouncer in mess-things-up mode, use these connection settings".

@SamSaffron
Copy link
Contributor

@SamSaffron SamSaffron commented Nov 22, 2017

And as a safety measure, it's not very useful if people have to opt in to it: people who are thinking about this as a possible issue, probably don't need it.

That is where I am lost here, what safety does this advisory lock provide?

All this does is ensures you fail a TINY bit earlier on extremely carefully crafted migrations. It makes the error message more consistent I guess, but this comes at an enormous cost cause now you have to teach everyone using pgbouncer about the new option.

@SamSaffron
Copy link
Contributor

@SamSaffron SamSaffron commented Nov 22, 2017

Discussed this with tgx and I think the simplest thing to do here with the path of least resistance is simply acquiring the advisory lock within the migration transaction (instead of outside of it)... functionality wise this would have 100% parity, only difference is that it would be safe with pgbouncer as well.

@matthewd
Copy link
Member

@matthewd matthewd commented Nov 22, 2017

Most importantly, it protects non-transactional migrations. But it also prevents dueling migrators (where A runs migration 1, then B starts, notes that migration 2 is due and runs that, then A goes to run 2 [because it comes after 1] and hits an error -- potentially leaving any success-reporting quite confused). And finally, it protects migrations that wouldn't error (but might mangle data) if run more than once -- not everything is as one-shot as a RENAME COLUMN.

This is a lock around the entire db:migrate, not around a single migration. That's where its value comes from, and is also why it isn't, and can't be, inside a transaction.

We use prepared statements by default, which are equally incompatible with PgBouncer's transaction pooling mechanism. I'm not convinced it's terrible to make people set two adjacent options where they were previously setting one; it's PgBouncer that's implementing its own half-take on how a PostgreSQL server should behave, not us.

If, say, PgBouncer had a special "lock this connection until the client disconnects" (i.e., "put this one connection into session pooling mode") query, I'd be happy to run that by default as a clue that migrations are special. (Or is there even some inquiry query we could use to identify PgBouncer [and its pooling mode] after we connect?) But I'm not excited about universally treating our database connections as stateless just because a [useful] proxy might be getting in the way and interfering with documented behaviour. 😐

@tgxworld
Copy link
Contributor Author

@tgxworld tgxworld commented Nov 22, 2017

Most importantly, it protects non-transactional migrations.

Is running non-transactional migrations possible in Rails? This is something that I am not aware of and would like to learn more about.

And finally, it protects migrations that wouldn't error (but might mangle data) if run more than once -- not everything is as one-shot as a RENAME COLUMN.

Is this possible? My understanding, at least for Postgres, is that the second migration will fail to record the version state so the transaction gets rollbacked anyway.

But it also prevents dueling migrators (where A runs migration 1, then B starts, notes that migration 2 is due and runs that, then A goes to run 2 [because it comes after 1] and hits an error -- potentially leaving any success-reporting quite confused).

While bad, it doesn't leave the database in a bad state. If someone is running migrations concurrently, the rake task would already blow up in a way that cannot be missed. The new ConcurrentMIgrationError isn't buying us any protection that doesn't already exist.

Thank you for your time 👍

@SamSaffron
Copy link
Contributor

@SamSaffron SamSaffron commented Nov 22, 2017

I guess but at the end of every transaction it inserts into schema_migrations table that already has a pk on name so if anything happens to run concurrently it would abort at that point.

The protection here is mostly about

  • Improved error message if you run db:migrate concurrently
  • Potentially reduced work if you run db:migrate concurrently
  • Guaranteed success on one of the concurrent db:migrate jobs

I follow and if this but the concept of running migrations in an uncontrolled way is very uneasy with me and I am not a huge fan of rails taking on this extra work. Running concurrent migrations in production is an "esoteric" deploy system in my books.

That said, I can accept adding an extra setting here for quirky pbbouncer, I did a bit of searching and can not see if there is a way to "trigger session mode" without triggering a transaction in transaction pooling mode. I guess it would be pretty simple to add to pg bouncer and longer term maybe rails can do this automatically.

I guess for now tgx will add the extra setting and make it default off, what should we call it? `migration_advisory_locks' ? default on, can be turned off in db yml.

@matthewd
Copy link
Member

@matthewd matthewd commented Nov 22, 2017

Is running non-transactional migrations possible in Rails?

Yep! disable_ddl_transaction!. e.g. https://robots.thoughtbot.com/how-to-create-postgres-indexes-concurrently-in

Yeah, I'll concede that (if you haven't disabled transactions), you'll ultimately hit a rollback on the unique key in schema_migrations, so there aren't as many failure conditions as I was imagining.

I think we can probably just call it advisory_locks and have it override the connection's opinion of whether the adapter supports them at all -- leave any future "well it's okay as long as you're inside a transaction" distinction for the day that becomes important/useful.

I also don't know of any existing way to force session pooling on a connection, though it seems like it'd be easy to support and widely useful... which makes me suspect there's some reason it's not as easy as it sounds, or something. 😕

I don't have a PgBouncer handy to play with.. is there any way to identify it from within a pooled session? IIRC all of the special SHOW commands etc only work when connected to its special database, but maybe there's something? Other than acquiring an advisory lock and then querying whether you still have it 😅

@SamSaffron
Copy link
Contributor

@SamSaffron SamSaffron commented Nov 22, 2017

Other than acquiring an advisory lock and then querying whether you still have it

A spin lock to eventually release it would do the trick, it would also make me quite ill ;p

We will go with advisory_locks agree it could be beneficial to build advisory lock support into pgbouncer in transaction pooling mode. But I guess it would be rather tricky.

@SamSaffron
Copy link
Contributor

@SamSaffron SamSaffron commented Nov 26, 2017

why not simply open up a second connection to the db, start a transaction and acquire the advisory lock there? Then when you are done migrating you finish the transaction and nobody needs to add any extra hacks. advisory locks are supported in pg on pgbouncer provided you are in a transaction see: https://www.postgresql.org/docs/9.4/static/explicit-locking.html

For wider compatibility you can acquire an exclusive table lock on schema_migrations in this second transaction (and do the inserting into it from there), so behavior will be far more consistent across adapters.

@SamSaffron
Copy link
Contributor

@SamSaffron SamSaffron commented Nov 29, 2017

@matthewd thoughts on the above ^^

Would you accept a PR that juggles the 2 connections so pgbouncer has safe advisory locks out of the box?

@matthewd
Copy link
Member

@matthewd matthewd commented Nov 29, 2017

I worry that it could be trading one restriction for another -- needing two sessions could interfere with anyone using DB user access control to keep migrations isolated, for example.

If this were our only pgbouncer incompatibility I might feel more sympathetic, but when basic querying's broken out of the box, it seems harder to worry about a second config. 😕

Have you had any luck finding any way to identify a pgbouncer connection from inside it? If we could, then we could change both settings automatically, and actually get it working out of the box.

@SamSaffron
Copy link
Contributor

@SamSaffron SamSaffron commented Nov 29, 2017

From what I can see nothing exists, I do wonder how expensive it would be to implement though cause you don't want every single query parsed just to see if it is version...

A simple workaround would be for pg bouncer just to always set something on the session, it is easily achievable in the config format, but what we need is something out-of-the-box

@meysammeisam
Copy link

@meysammeisam meysammeisam commented Dec 20, 2017

any news?
Is there any way or hack to use pgbouncer(transaction pooling mode) and rails together for now?

@SamSaffron
Copy link
Contributor

@SamSaffron SamSaffron commented Dec 20, 2017

@meysammeisam https://github.com/discourse/discourse/blob/master/lib/freedom_patches/postgresql_adapter.rb works for us

@davekapp
Copy link

@davekapp davekapp commented Jan 9, 2018

This unfortunately has become a problem for us as well, as we use PgBouncer as well.

So I'd give a big +1 to the ability to request to turn this off. It makes sense as an on-by-default feature for people who are not using PgBouncer, but the ability to turn it off without having to monkey-patch PosgtreSQLAdapter would be terrific.

(In the meantime a slightly modified version of the patch linked to by @SamSaffron above worked for us, thank you very much Sam!)

@rails-bot rails-bot bot added the stale label Apr 9, 2018
@rails-bot
Copy link

@rails-bot rails-bot bot commented Apr 9, 2018

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 5-1-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Apr 10, 2018

This is only missing someone that cares about this problem to open a PR to implement it. Who wants to chime in? 😄

@rails-bot rails-bot bot removed the stale label Apr 10, 2018
amatriain added a commit to amatriain/feedbunch that referenced this issue Apr 18, 2018
Since this commit:

rails/rails#22122

rails tries to acquire an advisory lock when running migrations; this is
intended to avoid the (unlikely) case of multiple migrations running
concurrently.

However this fails when the app is accessing a Postgres db through
pgbounce in transaction pooling mode (which is the only mode that gives
reasonable memory gains while allowing transactions to work correctly).
Unfortunately FeedBunch is impacted by this. This means that capistrano
deployments fail when trying to run migrations, even if no migration is
actually part of the deployment.

The current ugly fix is a monkey patch to disable advisory lock support
in the Postgres adapter. This is messy and could break other things,
I'm not sure if advisory locks are used elsewhere in Rails.

A discussion is currently underway about adding a config option to
disable the acquisiton of an advisory lock before running a migration,
see:

rails/rails#31190

If such an option is implemented this patch should be removed at once.
@rails-bot rails-bot bot added the stale label Jul 10, 2018
@rails-bot
Copy link

@rails-bot rails-bot bot commented Jul 10, 2018

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 5-2-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot closed this as completed Jul 17, 2018
@stve
Copy link
Contributor

@stve stve commented Aug 9, 2018

We're experiencing an issue on Heroku that I believe is related to this. I'd be happy to attempt a patch. Could someone offer some pointers to things I should consider while making this change?

@oehlschl
Copy link

@oehlschl oehlschl commented Nov 16, 2019

This thread is now pretty dated, but we hit this same case and I wanted to offer up an alternate solution (already suggested above, I think) that may work for some.

While I think a toggle to disable advisory locks is reasonable, I know that it's also not a complete solution to the "running migrations through PgBouncer in transaction pooling mode" problem. The migrations you'd want to run outside of a transaction (like concurrent indexing) are also ones for which you'd want to extend your database statement timeout (assuming you have a low one in the rest of your app), but doing that ad hoc requires that you maintain a consistent DB session for multiple commands. Transaction pooling mode can't guarantee that, so if you plan to manually extend your statement timeout before long-running migrations, you'd need to use PgBouncer's "session pool" mode.

We ended up running a PgBouncer cluster with two separate pools -- the main one used by the app in "transaction" mode, and a smaller pool in "session" mode specifically for migrations. From an app standpoint, we just had to change the DATABASE_URL to connect via the session pool when running migrations.

This solution may not be possible for people running PgBouncer on Heroku, since, from memory, you only have the ability to configure a single pool, but it is possible with a separate cluster.

Understood that PgBouncer specifics are not the main point of this thread, but, as that was the communicated use case, I hope this is helpful to some.

@jeffdill2
Copy link

@jeffdill2 jeffdill2 commented Dec 3, 2020

Does anyone have a workaround that they're using to solve this, as it's still an issue? I'm currently having to manually log into an EC2 instance and change the DB_PORT to 5432 (pgBouncer runs on 6432) and then manually run migrations to get them to work which is...less than ideal.

@SamSaffron
Copy link
Contributor

@SamSaffron SamSaffron commented Dec 3, 2020

You just need to notify the connection object you don't want advisory locks with

advisory_locks: false

@vlad-pisanov
Copy link

@vlad-pisanov vlad-pisanov commented Jun 3, 2021

We hack around it in Rails 5.x by adding this monkey patch in config/initializer/advisory_locks.rb:

class ActiveRecord::Migrator
  def use_advisory_lock?
    false
  end
end

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

No branches or pull requests