`--! no-transaction` is not working properly.

maxkorp commented 4 years ago

Found this on 0.0.11, but confirmed to still be broken on 0.017. For reference, we're using the maxkorp/postgis:11.4-2.5.2 docker image locally to dev against. @johnhaley81 swears he tried this at some point and it was working in our codebase, so there was some sort of regression, but we haven't updated migrate since we first brought in the version with this enabled, so possibly something with a node_module (more on that later).

Specifically, we pasted the following (redacted) into current.sql, after selecting the whole file so we're sure it's at the beginning of the file.

--! no-transaction
ALTER TYPE my_schema.my_type ADD VALUE IF NOT EXISTS 'New Value 1';
ALTER TYPE my_schema.my_type ADD VALUE IF NOT EXISTS 'New Value 2';

Unfortunately we still get errors about alter type in a transaction

[2020-01-14T00:35:47.011Z]: Running current.sql

🛑 Error occurred whilst processing migration
    error: ALTER TYPE ... ADD cannot run inside a transaction block
        at Connection.parseE (/Users/maxkorp/development/work/qwick/node_modules/pg/lib/connection.js:602:11)
        at Connection.parseMessage (/Users/maxkorp/development/work/qwick/node_modules/pg/lib/connection.js:399:19)
        at Socket.<anonymous> (/Users/maxkorp/development/work/qwick/node_modules/pg/lib/connection.js:121:22)

I've tried this both as part of migrate commit, where it runs the committed migrations (which all have transactions) first, as well as migrate watch --once, where no previous transactions have been established, in case it was hanging on to one still.

Where it gets interesting:

The code in migrate ts that determines if there should be a transaction works correctly. Using the latest version for simplicity, https://github.com/graphile/migrate/blob/master/src/migration.ts#L197 correctly assigns false to transaction.

https://github.com/graphile/migrate/blob/master/src/migration.ts#L202 (where it begins the transaction) never runs.

Once it tries to run however, it fails with the above error. I've tried stepping through the instrumentation with node --inspect-brk but couldn't find it starting a transaction anywhere, but it's clearly happening still.

Note that a few weeks back, there was an update to node-postgres (pg) that contained at least one unrelated breaking change (with regards to erroring when there is a cert chain validation error, a good and valid change but a breaking change in a minor version bump none the less). Possibly something else changed there to cause this issue.

benjie commented 4 years ago

This is an annoying bug that’s because node-postgres turns multiple statements into a representation that Postgres executes as a single transaction. The solution currently is to only have one statement per migration for no-transaction mode. I don’t know how to reliably fix this without using a SQL parser to split statements apart, so it’s an open issue.

maxkorp commented 4 years ago

That's... amazing >.<

I can't find a way to send stuff as raw, either. Or disable the transaction. That's great. Perhaps I could wrap it all inside a do block with an exec

benjie commented 4 years ago

That might work?

benjie commented 4 years ago

Added note to the README: https://github.com/graphile/migrate/commit/49dc7923a8ea00fb2843cfc01da04d06e48f5529#diff-04c6e90faac2675aa89e2176d2eec7d8

graphile / migrate

`--! no-transaction` is not working properly. #42

Where it gets interesting: