weavejester / ragtime

Database-independent migration library
Eclipse Public License 1.0
612 stars 85 forks source link

Support turning off transaction #124

Closed chopmo closed 6 years ago

chopmo commented 6 years ago

This change allows a migration to declare that it should not be run in a transaction, or that a transaction should only be applied in one direction.

Only supported when defining migrations in EDN. Example:

{:up            ["CREATE INDEX CONCURRENTLY foo ON bar(baz);"]
 :down          ["DROP INDEX foo;"]
 :transactions false}

My motivation for suggesting this feature is that I need to add an index with the CONCURRENTLY option as in the example above. This currently fails with the error PSQLException: ERROR: CREATE INDEX CONCURRENTLY cannot run inside a transaction block.

This is also an attempt to address #119.

I was unsure about how to include tests for this, but given a hint or two I'd obviously be happy to give it a try.

EDIT: At the suggestion of @danielcompton , added suport for only applying transactions in one direction. The transaction? option has been renamed to transactions and now accepts the values :up, :down, :both or true (any other value, like false, turns off transactions in both directions).

weavejester commented 6 years ago

Thanks for the PR. I'm inclined to merge it, but give me a little time to think about whether this is the best way to achieve it.

As for tests, we can at least check to see if migrations work regardless of whether or not :transaction? is true or not. Presumably you've also manually tested to see if it works in Postgres?

chopmo commented 6 years ago

Sure, thanks for considering.

I've added a few test cases to at least ensure that existing functionality does not break.

And yes, I did manually test this with Postgres 9.5.10.

taojoe commented 6 years ago

hi @weavejester,

when does this feature release?

weavejester commented 6 years ago

Apologies for letting this lie dorment. I think this is fine for merging, it just needs the commit messages to be updated as per the contributing guidelines.

danielcompton commented 6 years ago

Should there be separate settings for disabling transactions for up and down?

weavejester commented 6 years ago

Maybe:

:transactions :up
:transactions :down
:transactions :both  ;; (or true)
chopmo commented 6 years ago

@weavejester No problem, and apologies for not checking the contribution guidelines

@danielcompton Yes, I think that makes sense!

I'll see when I can find time to finish this up.

weavejester commented 6 years ago

@weavejester No problem, and apologies for not checking the contribution guidelines

Your PR predates the contributing guidelines, so that's fine :)

chopmo commented 6 years ago

@weavejester Haha, I guess that is a good excuse then :) I have now changed the PR to contain a single commit, adding support for applying transactions in only one direction. I've also tried my best to follow the guidelines for the new commit message.

@danielcompton Thanks for the idea - your thoughts are very welcome too of course.

weavejester commented 6 years ago

Thanks! I think the example in the commit message can be removed, as the previous paragraphs cover the change adequately. Also the triple backtick isn't necessary in this case, as a commit message is plaintext, rather than markdown.

Could you also ensure that the lines you added don't exceed 80 characters in length?

chopmo commented 6 years ago

Sure! I'm not sure how you prefer the code in the defrecord to be formatted, but I gave it a shot.

weavejester commented 6 years ago

Thanks, sorry this took so long to get merged.

chopmo commented 6 years ago

No problem, thanks for merging!