Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: an alias for creating postgre indexes CONCURRENTLY in a migration #3913

Open
capaj opened this issue Jul 6, 2020 · 5 comments

Comments

@capaj
Copy link
Contributor

capaj commented Jul 6, 2020

currently if I want to create an index in postgre CONCURRENTLY it requires some config and using the db.raw, like this:

import Knex from 'knex'

const table = 'scheduled_campaign_actions'

exports.up = async (db: Knex) => {
  await db.schema.raw(`
    CREATE INDEX CONCURRENTLY IF NOT EXISTS ${table}_notification_id_index
	  ON ${table} (notification_id);
  `)
}

exports.down = async (db: Knex) => {
  await db.schema.alterTable(table, (table) => {
    table.dropIndex('notification_id')
  })
}

exports.config = {
  transaction: false
}

it sure would be nice, if this could be achievable with some config alias, for example like this:

import Knex from 'knex'

const tableName = 'scheduled_campaign_actions'

exports.up = async (db: Knex) => {
  await db.schema.alterTable(tableName, (table) => {
    table.index('notification_id')
  })
}

exports.down = async (db: Knex) => {
  await db.schema.alterTable(tableName, (table) => {
    table.dropIndex('notification_id')
  })
}

exports.config = {
  indexConcurrently: false
}
@elhigu
Copy link
Member

elhigu commented Jul 15, 2020

Having table.index(..., opts) would be knexy way to implement support for this.

@capaj
Copy link
Contributor Author

capaj commented Jul 15, 2020

That would be. Problem is you can't have it in transaction so you'd have to always explicitly specify this like so:

import Knex from 'knex'

const tableName = 'scheduled_campaign_actions'

exports.up = async (db: Knex) => {
  await db.schema.alterTable(tableName, (table) => {
    table.index('notification_id', {concurrently: true})
  })
}

exports.down = async (db: Knex) => {
  await db.schema.alterTable(tableName, (table) => {
    table.dropIndex('notification_id', {concurrently: true})
  })
}

exports.config = {
  transaction: false
}

which for me would be okay too, just a little more verbose.

@capaj capaj changed the title Feature request: an alias for creating postgre indexes CONCURRENTLY Feature request: an alias for creating postgre indexes CONCURRENTLY in a migration Jul 15, 2020
@raijinsetsu
Copy link

@capaj I think it would work out fine. Knex does not execute those schema operations until the function returns. It then orders those operations: table creation, constraint creation, index creation - they have to occur in this order even when inside a transaction because you cannot create an index on a column that does not exist. When creating these types of indexes, the first transaction would create the table, etc., but not those "concurrent" indexes. It would then not use a transaction for those.

Side note:
I just want to point out that you should never use string templates or constructed strings in the queries. Re-write your raw queries to use bindings:

await db.schema.raw('
    CREATE INDEX CONCURRENTLY IF NOT EXISTS :index:
	  ON :table: (notification_id);
  ', {index: `${table}_notification_id_index`, table})

This makes it impossible for SQL injection attacks to occur as the bindings are properly escaped. Reference: http://knexjs.org/#Raw-Bindings

Sorry if that seems preachy and you already know... but I've seen too many security vulnerabilities in software to overlook it.

@capaj
Copy link
Contributor Author

capaj commented Jul 16, 2020

@raijinsetsu migrations don't deal with user input at all. There's no possibility of SQL injection there.

Right, knex schema builder could skip the transacting() call. That could theoretically work.

@elhigu
Copy link
Member

elhigu commented Jul 19, 2020

@raijinsetsu actually identifier bindings are not correctly escaped by knex, but knex just adds siple quoting. Knex also may try to escape it to be safe, but doesn’t guarantee anything. I wouldn’t trust it being safe.

So one must always do manual validation for identifiers read from user input.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants