Implement safe locking in unsafe_* methods when appropriate #113

rkrage · 2024-11-27T20:02:32Z

Closes #63

Leaving the below context, but just a note that most of that refactoring was actually done in: #123

Okay, so this is a massive one, but the diff is so large because I used this as an opportunity to reorganize the tests and moved all of the unsafe_* methods into unsafe_statements.rb. It became incredibly difficult to locate and modify tests to add locking expectations (and surprise some of those methods didn't even have tests), so I attempted to organize the tests into four categories:

raw methods that are simply delegated to the underlying Rails method via delegate_raw_method_to_migration_base_class
unsafe methods that are simply delegated to underlying Rails method via delegate_unsafe_method_to_migration_base_class_with_lock or delegate_unsafe_method_to_migration_base_class
unsafe methods that we define ourselves
safe methods that we define ourselves

This additionally removes support for Rails 7.0 (in a separate commit) which gets rid of a fifth type of method:

raw methods that we define ourselves

lib/pg_ha_migrations/unsafe_statements.rb

rkrage · 2025-04-09T15:18:39Z

lib/pg_ha_migrations/safe_statements.rb

-  # constraint name, we are converting name to string explicitly to solve that.
-  def raw_remove_check_constraint(table, name:, **options)
-    super(table, name: name.to_s, **options)
-  end


As I was looking to organize methods and their tests, I discovered this unicorn. Rails 7.0 is EOL, so my solution was to just remove this method and Rails 7.0 from our gemspec / build matrix

lib/pg_ha_migrations/unsafe_statements.rb

jcoleman

Do we want to have unsafe_statements_spec.rb now?

I left a few comments on the code, but the spec file diff is unreadable; I think the issue is that the removal of something deprecated/defaults is getting confused with the move of the unsafe methods testing. But that no tonly means that the things it thinks are changes aren’t necessarily changes, it means all of the moved stuff is just shown as all net new…Maybe the answer is a commit that moves the tests around in the file (like a pure refactor) and then a separate commit with actual changes?

jcoleman · 2025-04-09T17:54:01Z

lib/pg_ha_migrations/unsafe_statements.rb

+  # Direct dispatch to underlying Rails method as unsafe_<method_name> with dependent object check / no locking
+  #
+  # We don't do locking on execute because it's so generic.
+  # For drop_table and rename_table, it's implied that the table is unused.


This makes sense for drop_table, although I don't really see what blocks having safe locking on it -- it seems like it would work as expected out of the box just like the other methods, so why not have it?

Re: rename_table: that does not imply the table is unused, necessarily, given it's often used to compose swapping live tables, right?

So both drop_table and rename_table are generally used together to swap live tables. I actually think if we added locking to these, a recent BT migration would not have worked...

The general operation is something like:

acquire lock on old table (and implicitly open a transaction)

create new table

do some manipulation of the data to move stuff from old to new

drop old

rename new to old

release lock (and commit transaction)

If we added locking to these methods, I think (5) would fail with a "nested lock detected" error.

However, if you're writing a migration that complex you might just use the raw_* methods to get around these limitations. So I guess I'm indifferent about adding locking to these methods. If something is actually still using a table when you go to drop / rename, the end result is still the same (it would just delay the catastrophe if it takes a bit of time to acquire the lock).

If we added locking to these methods, I think (5) would fail with a "nested lock detected" error.

It'd fail in development though. Since we support locking multiple tables with safely_acquire_lock_for_table I think it'd be easy to fix, right?

Basically my thought comes down to this: we want to be safe by default, and safely acquiring a lock on something that's safe to drop is safe, but failing to acquire that lock safely opens us to the possibility that a mistaken table drop also locks the system up.

I think it'd be easy to fix, right?

Sure but then you would have to move the table creation outside the lock. Can't lock a table that doesn't exist!

But yeah, I see your point. I don't feel too strongly about all this so I'll go ahead and make this change.

A somewhat related question... would it add safety to have some kind of timeout / retry limit for lock acquisition? As it stands safely_acquire_lock_for_table just keeps trying indefinitely, correct?

A somewhat related question... would it add safety to have some kind of timeout / retry limit for lock acquisition? As it stands safely_acquire_lock_for_table just keeps trying indefinitely, correct?

Yeah, I think so. That sounds like a good candidate for an issue to discuss further.

lib/pg_ha_migrations/unsafe_statements.rb

jcoleman · 2025-04-10T18:42:15Z

I think the diff will be actually readable if rebased on #123

spec/safe_statements_spec.rb

jcoleman · 2025-04-11T17:48:28Z

lib/pg_ha_migrations/unsafe_statements.rb

+  # Direct dispatch to underlying Rails method as unsafe_<method_name> with dependent object check / safe lock acquisition
+  delegate_unsafe_method_to_migration_base_class_with_lock :add_check_constraint
+  delegate_unsafe_method_to_migration_base_class_with_lock :add_column
+  delegate_unsafe_method_to_migration_base_class_with_lock :change_column


It seems like we should treat this like we do change_table?

I guess my thinking was that anything you're trying to do with change_table can be accomplished with a combination of other, more specific, unsafe methods. Do we have that same situation with change_column? Like if you wanted to change a column type would you be forced to use raw_change_column?

Like if you wanted to change a column type would you be forced to use raw_change_column?

Hmm, that's a good point. We probably should have a way to do those things discretely (and that would allow a safe version for types, also, since some are binary compatible).

But the non-existence of those now suggests we can punt that to an issue to track it.

Yeah I was thinking about that a bit too. I can open an issue and we can discuss further, but I'm assuming the list of binary compatible types needs to be whittled down to type changes that Rails can transparently handle. For example, json -> text appears to be binary compatible, but I imagine Rails might not like that

Yeah, sounds good.

spec/safe_statements_spec.rb

rkrage force-pushed the locking-in-unsafe-methods branch from e1b5dc6 to ec7ea92 Compare March 6, 2025 19:36

rkrage force-pushed the locking-in-unsafe-methods branch 2 times, most recently from 67f5753 to a044613 Compare March 20, 2025 14:21

rkrage force-pushed the locking-in-unsafe-methods branch from 6561609 to 4709908 Compare April 9, 2025 15:09

rkrage commented Apr 9, 2025

View reviewed changes

lib/pg_ha_migrations/unsafe_statements.rb Show resolved Hide resolved

rkrage commented Apr 9, 2025

View reviewed changes

lib/pg_ha_migrations/unsafe_statements.rb Show resolved Hide resolved

rkrage force-pushed the locking-in-unsafe-methods branch from 4709908 to 9bbdc6d Compare April 9, 2025 15:33

rkrage changed the title ~~[WIP] Implement safe locking in unsafe_* methods when appropriate~~ Implement safe locking in unsafe_* methods when appropriate Apr 9, 2025

rkrage marked this pull request as ready for review April 9, 2025 15:42

rkrage mentioned this pull request Apr 9, 2025

Release a new version that supports Ruby 3.3 / Rails 8 #115

Closed

rkrage force-pushed the locking-in-unsafe-methods branch from 9bbdc6d to 6efefe0 Compare April 9, 2025 17:45

rkrage mentioned this pull request Apr 9, 2025

Take out SHARE lock in partitioned index creation #121

Merged

jcoleman reviewed Apr 9, 2025

View reviewed changes

rkrage force-pushed the locking-in-unsafe-methods branch from 9adbc2b to a490a8b Compare April 11, 2025 14:57

rkrage commented Apr 11, 2025

View reviewed changes

spec/safe_statements_spec.rb Outdated Show resolved Hide resolved

Implement safe locking in unsafe_* methods when appropriate

da7c9b8

rkrage force-pushed the locking-in-unsafe-methods branch 2 times, most recently from 7e3d6aa to 234de8c Compare April 11, 2025 15:41

Remove support for EOL Rails 7.0

fc6d5c5

rkrage force-pushed the locking-in-unsafe-methods branch from 234de8c to fc6d5c5 Compare April 11, 2025 15:52

jcoleman reviewed Apr 11, 2025

View reviewed changes

add locking to unsafe_{drop,rename}_table

4c6a23c

rkrage force-pushed the locking-in-unsafe-methods branch from 016f0c6 to 4c6a23c Compare April 11, 2025 19:32

rkrage added 2 commits April 15, 2025 18:57

Add note to readme about unsafe_change_table

bee6f73

Replace context blocks with describe

4815e04

jcoleman approved these changes Apr 15, 2025

View reviewed changes

rkrage merged commit fa78a33 into master Apr 15, 2025
48 checks passed

rkrage deleted the locking-in-unsafe-methods branch April 15, 2025 19:23

Implement safe locking in unsafe_* methods when appropriate #113

Implement safe locking in unsafe_* methods when appropriate #113

Uh oh!

Conversation

rkrage commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jcoleman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkrage Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jcoleman commented Apr 10, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rkrage commented Nov 27, 2024 •

edited

Loading

rkrage Apr 9, 2025 •

edited

Loading