node-pg-migrate icon indicating copy to clipboard operation
node-pg-migrate copied to clipboard

createIndex `concurrently` by default or enforced

Open benjamine opened this issue 4 years ago • 3 comments

assuming this tool most common use case is for running zero-downtime migrations, would it make sense for concurrently true be the default option on .createIndex? not sure this can be easily introduced without a breaking change on this library, but it could prevent potentially catastrophic mistakes :)

maybe another option that could be even nicer, is to have an initialization configuration that ensures create index concurrently is enforced.

references:

  • ruby gem that validates against errors like create index non-concurrently: https://github.com/LendingHome/zero_downtime_migrations#usage

benjamine avatar Oct 21 '20 22:10 benjamine

I do not think it should be a default option. But sort of configuration is a good idea. Taking it further, I can imagine it can sort of work like shorthands, so it is defined in the migration file and all following migrations take it as default. You would be able to change the defaults later and all would be stored in code. You would be able to define these defaults for all migration methods. e.g.

export const defaults = {
  createIndex: { concurrently: true, method: 'btree' },
  createTable: { ifNotExists: true },
}

I need to think about it...

dolezel avatar Oct 29 '20 12:10 dolezel

It seems Postgres does not allow CONCURRENTLY in transactions

pgm.createIndex(
    "User",
    [
      {
        opclass: "gin_trgm_ops",
        name: "index_users_on_email_trigram"
      }
    ],
    {
      method: "gin",
      concurrently: true
    }
  );

gives

Rolling back attempted migration ... error: CREATE INDEX CONCURRENTLY cannot run inside a transaction block

teebot avatar Nov 20 '20 11:11 teebot

@teebot that's correct, I think tools like ruby one I mentioned will warn you so you always create an index in a separate migration step because of that.

benjamine avatar Nov 20 '20 12:11 benjamine