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

[Idea] Stop migrations with disable_ddl_transaction! and add_reference #202

Closed
rui-sebastiao opened this issue Oct 10, 2022 · 17 comments
Closed

Comments

@rui-sebastiao
Copy link

rui-sebastiao commented Oct 10, 2022

Hi,

Recently we had an issue at my company. Our stack: Rails 7 and PG13, deployed on Heroku.

Scenario

  1. Like we had done so many times, we added a migration with disable_ddl_transaction! and add_reference ... index: { algorithm: :concurrently} on some PR.
  2. During deploy, the migration created the column, however, since it ran at a time when the db was under stress, it failed to create the index so the migration was not created on the rails migrations table.
  3. This prevented further deploys and caused a lot of chaos until we understood what the issue was, because basically the migrations mechanism was permafailing due to the fact that the column already existed, but the migration was still marked as down.

Root causes

  1. The db was under stress
  2. The reference and index creation occurred in a migration with disable_ddl_transaction!, which meant it allowed part of it to go through instead of reverting it all in case of failure.
  3. The migration was never marked as done, even though part of it (the column creation) went through.

What we did to prevent further chaos

  1. Create a cop for our dear friend rubocop that prevents the usage of disable_ddl_transaction! and add_reference in the same migration
  2. Create a custom check for StrongMigrations™️ to prevent add_reference with index creation.

Given that this is a scenario that may happen to more people, do you think it would it be valuable to include this out of the box on the gem? Do you want me to submit a PR?

For reference, our custom check source code:

StrongMigrations.add_check do |method, args|
  if method == :add_reference
    kwargs = args[2]
    if kwargs[:index] != false
      stop! %(
        Don't add the index in the same migration as the reference.
        Example of desired usage:
        Migration 1: `add_reference :table, :column, index: false`
        Migration 2: `disable_ddl_transaction! ... add_index :table, :column, algorithm: :concurrently`.)
    end
  end
end
@ankane
Copy link
Owner

ankane commented Oct 19, 2022

Hey @rui-sebastiao, thanks for the suggestion and all the context. In either case, someone will need to manually resolve if creating the index fails, so I (personally) don't think it's worth the hassle of splitting into separate migrations (plus it's safe in either case). However, seems like a good use for custom checks for those who want it.

@jdufresne
Copy link
Contributor

In either case, someone will need to manually resolve if creating the index fails, so I (personally) don't think it's worth the hassle of splitting into separate migrations (plus it's safe in either case).

Wouldn't there be less total work to solve in this situation?

If the migrations are split, the column will create successfully and upon rerunning migrations, the user doesn't need to worry about a "the column already exists" error. That migration is done. The only concern is with fixing the index.

When the migrations are one, the user is unable to rerun the migration without modifying the migration itself or first blowing away the new column.

@rui-sebastiao
Copy link
Author

In either case, someone will need to manually resolve if creating the index fails, so I (personally) don't think it's worth the hassle of splitting into separate migrations (plus it's safe in either case).

Wouldn't there be less total work to solve in this situation?

Yes, from our experience with the issue that raised this suggestion 😄

When the migrations are one, the user is unable to rerun the migration without modifying the migration itself or first blowing away the new column.

Exactly, hence why I think it would be valuable to integrate this. Single line instructions with more than one outcome and disabled transactions can yield inconsistent results.

@ankane
Copy link
Owner

ankane commented Oct 30, 2022

I think there's more total work with the proposed change.

With the change, users need to spend more time writing add_reference migrations, even though most should succeed. If the add_index migration fails, users need to manually drop the invalid index and re-run the migration.

Without the change, users don't need to change how they write add_reference migrations. If the migration fails, users need to manually drop the new column(s) and re-run the migration.

@ngouy
Copy link

ngouy commented Nov 11, 2022

Kinda in the same vibe: (not sure if I should open a new ticket or surf on this one)

  • I have a migration let's say that create 2 indexes
  • I have disable_ddl_transaction! because hey, that's what we should do when we create an index
  • The first index is created, for some reason the second fails (not linked to the migration code itself, but let's say circumstances)

Now all of a sudden everything is broken:

  • I fix the circumstances
  • I have to run the migration (again)
  • I try to run the migration (again), but : the first index was already created, and wasn't in a transaction (obviously) so is already persisted/existing.
  • so the migration errors out with "relation 'index_whatever' already exists"

What if we "strongly suggest" (pun intended) to create indexes with a if_not_exist: true

@ankane
Copy link
Owner

ankane commented Nov 11, 2022

If creating an index concurrently fails, you'll always need to manually fix. I don't think it's much more work to handle two indexes rather than one, but teams who don't feel the same can use custom checks. See #205 (comment) for an example.

if_not_exists doesn't check if the index is valid, so running the migration a second time will succeed, even if the index isn't in a good state (but you can use custom checks to enforce this as well if you'd like).

@ngouy
Copy link

ngouy commented Nov 11, 2022

if_not_exists doesn't check if the index is valid, so running the migration a second time will succeed, even if the index isn't in a good state

Wait... the index could be created and not checked?
dang 😨

you'll always need to manually fix

Not super practical.
Not sure how this is more safe than not disabling the transaction in a first place.
But all good. As you said we have the tools to pick one or the other and make this decision ourselves

Thanks for the fast reply!!! and valuable advice 🙏

PS about the 205#comment:
even if there is only one index, and the global db:migrate is failing, I think (I'm not 100% sure) I already had the same issue where I end up in a weird state for the exact same reasons because:

  • the migration outside the transaction where index was created succeded
  • another migration fails
  • but the rails part that stores the migration version of the index creation in the shema_versions table IS part of the transaction. So never got "saved" (might be wrong on this one)
  • so rails has virtually no idea the index was created. And just try to re-create next attempt of db:migrate

@rui-sebastiao
Copy link
Author

if_not_exists doesn't check if the index is valid, so running the migration a second time will succeed, even if the index isn't in a good state

Wait... the index could be created and not checked? dang 😨

At our company we also patched add_index to remove the index if for some reason concurrent creation fails and it raises an error (but it still didn't save us from the problem reported on this issue).

@ngouy
Copy link

ngouy commented Nov 11, 2022

@rui-sebastiao would be curious to check how it looks like?
Monkey patching with:

  • a rescue
  • that check if index is existing
  • remove it if it does
  • re-raise the error?

@fatkodima
Copy link
Contributor

In my "fork" of strong_migrations I made such operations idempotent, for example - https://github.com/fatkodima/online_migrations/blob/bcc4881d2cadb24443ed2d11495f492ab44e882c/lib/online_migrations/schema_statements.rb#L683-L720

@rui-sebastiao
Copy link
Author

@rui-sebastiao would be curious to check how it looks like? Monkey patching with:

  • a rescue
  • that check if index is existing
  • remove it if it does
  • re-raise the error?

Yes, exactly.

@jjb
Copy link

jjb commented Nov 16, 2022

@rui-sebastiao I'd be interested in see the cop if you don't mind sharing

@rui-sebastiao
Copy link
Author

sure @jjb, here you go:

    class EnsureProperAddReference < Base
      MSG = <<~TXT
        When using add_reference, don't use disable_ddl_transaction! and define the index on a separate migration.
        Migration 1: `add_reference :table, :column, index: false`
        Migration 2: `disable_ddl_transaction! add_index :table, :column, algorithm: :concurrently`.
      TXT
      RESTRICT_ON_SEND = %i[disable_ddl_transaction!].freeze

      def_node_matcher :wrong_usage?, <<~PATTERN
        (begin
          ...
          (def _
            (args)
            (send nil? :add_reference _ _ (hash $...))
          )
        )
      PATTERN

      def on_send(node)
        add_offense(node, message: MSG) if wrong_usage?(node.parent)
      end
    end

@rui-sebastiao
Copy link
Author

Closing the issue as I see we're not going to add it to source, but already had a healthy discussion about it.
Thank you everyone for your thoughts and inputs!

@jjb
Copy link

jjb commented Nov 17, 2023

@rui-sebastiao is RESTRICT_ON_SEND some sort of internal API (i didn't find it in github code search though) or is that leftover from a previous implementation?

@fatkodima
Copy link
Contributor

RESTRICT_ON_SEND is an optimization from rubocop - rubocop/rubocop#8365

@jjb
Copy link

jjb commented Nov 17, 2023

Thanks! 🤦 and now I realize that Rui's code is a cop and not a strong_migrations custom check.

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

No branches or pull requests

6 participants