Allow migrations to be split over multiple Conn.Exec calls

[trashhalo]

We found that postgres does not let you send certain commands with other commands. Example create index concurrenrly. Even if you disable tx this will bomb out if sent with a alter table statement.

This PR allows migration writers to control how tern splits up migrations over multiple Exec calls. To do this you add a special sql comment. -- #SEND at the location you want to split before sending more sql.

[jackc]

One thing that concerns me is the possibility for an error to cause a final state between migrations. For example:

alter table ...;
-- #SEND
do something else that causes an error

The migration would only be half applied with no simple way to roll back or forward.

Would it be possible to instead run create index concurrently in its own migration?

[trashhalo]

Yes that would work. The down side is an proliferation of one line migrations. This is especially annoying when adding a few values to a enum since alter type add value is also affected by this.

ALTER TYPE ... ADD VALUE (the form that adds a new value to an enum type) cannot be executed inside a transaction block.

[jackc]

Yes that would work. The down side is an proliferation of one line migrations. This is especially annoying when adding a few values to a enum since alter type add value is also affected by this.

I agree that is awkward. But I really don't like the idea of making it possible to partially apply a migration. That violates a pretty fundamental guarantee of how tern works -- 1 file / migration is an atomic unit.

[alex]

I'm a coworker of Stephen's. I'm very torn here, the idea of a migration of an atomic unit makes sense to me. But the ergonomics of adding N indexes concurrently, or N enum variants, is very frustrating.

I don't have a particular suggestion for resolving the competing concerns though.

I wonder what was the original notion when DisableTx was added?

[jackc]

I wonder what was the original notion when DisableTx was added?

It looks like it was a similar concern -- though the use case was the tern library embedded in an application rather than the command line tool. https://github.com/jackc/tern/issues/4

That might be a potential solution too -- a CLI argument to disable transactions. Hmm... Maybe not. I just hacked around enough to disable transactions for the CLI and it still didn't work with multiple statements... I suppose it is the implicit transaction that wraps a simple query at the protocol layer...

But wait a second... Does the original solution here work? Because currently tern wraps each the entire migration in a transaction. I would have expected it to fail even when one statement is sent at a time.

And testing it on current tern master it fails even when there is only a single create index concurrently is in the migration.

[alex]

We combine this solution with DisableTx, to achieve the "really, no transactions" result.

On Wed, Nov 27, 2019 at 9:37 PM Jack Christensen notifications@github.com wrote:

I wonder what was the original notion when DisableTx was added?

It looks like it was a similar concern -- though the use case was the tern library embedded in an application rather than the command line tool. #4 https://github.com/jackc/tern/issues/4

That might be a potential solution too -- a CLI argument to disable transactions. Hmm... Maybe not. I just hacked around enough to disable transactions for the CLI and it still didn't work with multiple statements... I suppose it is the implicit transaction that wraps a simple query at the protocol layer...

But wait a second... Does the original solution here work? Because currently tern wraps each the entire migration in a transaction. I would have expected it to fail even when one statement is sent at a time.

And testing it on current tern master it fails even when there is only a single create index concurrently is in the migration.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jackc/tern/pull/17?email_source=notifications&email_token=AAAAGBFTEMDJRULUGJDUEELQV44JBA5CNFSM4JQJCTB2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFLK4LY#issuecomment-559328815, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAAGBHEJ64HU3LJHRI7C2DQV44JBANCNFSM4JQJCTBQ .

-- All that is necessary for evil to succeed is for good people to do nothing.

[jackc]

We combine this solution with DisableTx, to achieve the "really, no transactions" result.

Ah. So I take it that this use case is for when tern is embedded as a library?

[alex]

Yes, we use tern as a library.

On Thu, Nov 28, 2019 at 11:01 AM Jack Christensen notifications@github.com wrote:

We combine this solution with DisableTx, to achieve the "really, no transactions" result.

Ah. So I take it that this use case is for when tern is embedded as a library?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jackc/tern/pull/17?email_source=notifications&email_token=AAAAGBG3AWCLUN67PUSEJNLQV72OXA5CNFSM4JQJCTB2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFNDW5A#issuecomment-559561588, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAAGBE7KM45CGJ36ZAZL3DQV72OXANCNFSM4JQJCTBQ .

-- All that is necessary for evil to succeed is for good people to do nothing.

[jackc]

I've thought about this a bit more and I'm in favor of the concept / functionality but I think the implementation can be improved.

What occurred to me is that this feature is really more of a completion of the DisableTx feature rather than adding a new feature. i.e. The risk of a partially applied migration already exists. So in that case it doesn't cause any harm to make DisableTx more useful.

With that in mind though, it seems this feature should be tied more tightly to DisableTx -- as it will not work unless DisableTx is enabled and DisableTx is only marginally useful without it. With that in mind, there are two changes that I think would make this much more useful.

First, in the first commit statements were split on ; rather than a magic comment. I'm guessing the change to a magic comment was to avoid error in case of a ; in a string literal, identifier, or comment. However, using ; is undoubtedly a more ergonomic interface. Split can't correctly do it, but a small parser could. This feature would then be enabled based on DisableTx alone.

Second, setting DisableTx in the migrator settings is a bit of a blunt instrument. This potentially means that migrations that can run in a transaction are not -- running the risk of a partially applied migration when the risk is avoidable for that migration. I'd like to see a magic comment that disables transactions on a migration by migration basis. This also allows the CLI tool to take advantage of this feature.

This would lead to migrations like the following:

---- disable-tx ----
create index concurrently foo_a_idx on foo (a);
create index concurrently foo_b_idx on foo (b);

---- create above / drop below ----

drop index foo_b_idx;
drop index foo_a_idx;

[torkelrogstad]

Was there any resolution to the issue of doing TX disabling on a per migration basis? I'm trying to alter an enum, which I can't do in a TX.

[trashhalo]

Was there any resolution to the issue of doing TX disabling on a per migration basis? I'm trying to alter an enum, which I can't do in a TX.

The non-profit I worked for used my branch to solve this problem of this up until I left. It was never internally prioritized to do a more holistic implementation like jack through out. As I don't work there anymore I'm no longer keeping my branch up to date with master.

[jackc]

@torkelrogstad I haven't had a chance to work on this, but my current opinion on the correct solution is described in #43.

[jackc]

I've added disabling transactions per migration and sending each statement in the migration separately in b23e02f669b806ba8ac09037893acd6aeab79b1e in the v2-dev branch. This should resolve the issue.