Getting pq: syntax error at or near "transaction" when running migration with transaction:false

[brookback]

Description

Having

--migrate:up transaction:false

in a dbmate migration file produces a syntax error.

• Version: dbmate version 2.4.0
• Database: postgres v13
• Operating System: macOS 12.6.3

Steps To Reproduce

1. Do a postgres migration with CREATE INDEX CONCURRENTLY ... in a dbmate migrations file:

-- migrate:up transaction:false

drop index concurrently my_index;

create index concurrently my_index on my_table (
    some_column
);

2. Run with dbmate --no-schema-dump up.

Actual Behavior

Applying: 20230621121155_my_migration.sql
Error: pq: syntax error at or near "transaction"

It seems this is some bug/limitation with the pq lib, but I just wanted to give a heads up.

[docapotamus]

Taking a look at this, I've created similar migration:

-- migrate:up transaction:false
drop index concurrently idx;

create index concurrently idx on test (id);

I get a slightly different error which actually states it can't work within a transaction:

% dbmate up
Applying: 20230831142614_error.sql
Error: pq: DROP INDEX CONCURRENTLY cannot run inside a transaction block

Details:

• Version: 2.6.0
• Database: PostgreSQL 15.3
• System: MacOS 13.4.1

Not got my test enviornment setup back up yet, I'll investigate more soon.

[dossy]

I was able to reproduce this issue with the latest dbmate 2.7.0-main, against PostgreSQL 10.21:

root@c2a4e68d4451:/src# cat testdata/db/migrations/455_test_transaction_concurrently.sql 
-- migrate:up transaction:false

drop index concurrently my_index;

create index concurrently my_index on my_table (
    some_column
);

-- migrate:down

root@c2a4e68d4451:/src# dist/dbmate -u $POSTGRES_TEST_URL -d testdata/db/migrations up
Applying: 455_test_transaction_concurrently.sql
Error: pq: DROP INDEX CONCURRENTLY cannot be executed from a function or multi-command string

Searching for that particular error turns up lib/pq issue #820, and our own #126 that references it, where @amacneil wrote:

It sounds like this is a limitation of the lib/pq driver, and I'm not aware of any other golang drivers for postgresql. Therefore, I will close this with the workaround of putting each concurrent index creation in a separate migration file.

And this issue came up again in #182.

It looks like this is ultimately an issue with how lib/pq and Postgres handle transactions and support for multi-command queries, and isn't something that can be fixed from dbmate's end.

The workaround is to create separate migration files for each CREATE INDEX CONCURRENTLY command, which isn't great but it's simple and it works.

Perhaps the migrations option section of the README could document this, including the various error string responses above so that people searching may be able to find the information?

This is certainly a problem that has occurred repeatedly over the years, so I think there should be value in mentioning it clearly in the documentation.

[dossy]

Duplicate of #126.

[RoiGlinik]

does moving to pgx can solve this ?

Or using some kind of (not perfect ) regex ( ;[end of line]) to separate statements and run them one by one?

I think it can support a lot of use cases

[dossy]

does moving to pgx can solve this ?

Or using some kind of (not perfect ) regex ( ;[end of line]) to separate statements and run them one by one?

I think it can support a lot of use cases

Moving to pgx is unlikely to solve this.

The author of pgx, @jackc, created their own SQL migration tool named tern. And, they encountered this very same issue, because the problem is not one that can be safely resolved at the driver level, in pgx.

Regular expressions alone cannot be used to create a fully correct SQL parser that could safely split any arbitrary SQL statement, although strictly limiting the splitting to ;\n might be a little safer, but still not 100% safe:

insert into t (data) values (
  'this is a;
long string'
);

You can see what @jackc did in tern, implementing their own minimal SQL parser in order to split statements, in this commit, b23e02f6. If you examine the tests they wrote, you can begin to see the complexity of possible cases that even a "simple" implementation must handle correctly.

[RoiGlinik]

@dossy , I believe it can solve a lot of use cases for transaction:false, which is better then the current state.

The Author states he want to keep things simple, supporting just basic statements could be good enough. Also, in Goose they use a custom annotation for complex statements. This also can be done on every statement in the transaction:false case.

-- +goose StatementBegin
CREATE OR REPLACE FUNCTION histories_partition_creation( DATE, DATE )
returns void AS $$
DECLARE
  create_query text;
BEGIN
  FOR create_query IN SELECT
      'CREATE TABLE IF NOT EXISTS histories_'
      || TO_CHAR( d, 'YYYY_MM' )
      || ' ( CHECK( created_at >= timestamp '''
      || TO_CHAR( d, 'YYYY-MM-DD 00:00:00' )
      || ''' AND created_at < timestamp '''
      || TO_CHAR( d + INTERVAL '1 month', 'YYYY-MM-DD 00:00:00' )
      || ''' ) ) inherits ( histories );'
    FROM generate_series( $1, $2, '1 month' ) AS d
  LOOP
    EXECUTE create_query;
  END LOOP;  -- LOOP END
END;         -- FUNCTION END
$$
language plpgsql;
-- +goose StatementEnd

For me its blocking this simple transaction:false migration:

 set statement_timeout = '60min';
 CREATE INDEX Concurrently  IF NOT EXISTS...

If a few lines of code can add 70% statement support dont you think its a good addition? And of course not supporting multiline statements or harder statements in this mode will be added to the readme

[dossy]

If a few lines of code can add 70% statement support dont you think its a good addition? And of course not supporting multiline statements or harder statements in this mode will be added to the readme

My personal preference is to not add anything that will break what currently is known to work, even if it's a really good addition. Personally, I value stability and reliability over feature completeness. I understand others feel differently; that's why there are so many varieties of products that do similar things.

If someone can provide an implementation of a few lines of code that will not break every user's current migrations, with tests to prove such, we could all review it together and evaluate whether it is a good addition or not.

[guacs]

Would there be interest in using pg_query to handle this? If that is used, then there should be no worry to statement complexity since that's using the actual Postgres parser itself under the hood.