From a87fa36dda1cd8213ebf62a4bffbdc9536777bc0 Mon Sep 17 00:00:00 2001 From: Andrew Farries Date: Thu, 16 May 2024 12:08:58 +0100 Subject: [PATCH] Support create/drop index with uppercase names (#356) Fixes https://github.com/xataio/pgroll/issues/355 Postgres stores index names with uppercase characters in the `pg_index` catalog using the quoted version of the name. For example: ``` "idx_USERS_name" ``` whereas a lowercase index name would be stored as: ``` idx_users_name ``` This is different to how other object types are stored in their respective catalogs. For example, table names are stored in the`pg_class` catalog without quotes, regardless of whether they contain uppercase characters. This makes it necessary to strip quotes from index names when retrieving them from the `pg_index` catalog when building the internal schema representation. --- pkg/migrations/op_create_index.go | 3 +- pkg/migrations/op_create_index_test.go | 122 ++++++++++++++------- pkg/migrations/op_drop_index.go | 4 +- pkg/migrations/op_drop_index_test.go | 142 +++++++++++++++++-------- pkg/state/state.go | 2 +- 5 files changed, 190 insertions(+), 83 deletions(-) diff --git a/pkg/migrations/op_create_index.go b/pkg/migrations/op_create_index.go index d2aff864..1d8df7f3 100644 --- a/pkg/migrations/op_create_index.go +++ b/pkg/migrations/op_create_index.go @@ -30,7 +30,8 @@ func (o *OpCreateIndex) Complete(ctx context.Context, conn db.DB, tr SQLTransfor func (o *OpCreateIndex) Rollback(ctx context.Context, conn db.DB, tr SQLTransformer) error { // drop the index concurrently - _, err := conn.ExecContext(ctx, fmt.Sprintf("DROP INDEX CONCURRENTLY IF EXISTS %s", o.Name)) + _, err := conn.ExecContext(ctx, fmt.Sprintf("DROP INDEX CONCURRENTLY IF EXISTS %s", + pq.QuoteIdentifier(o.Name))) return err } diff --git a/pkg/migrations/op_create_index_test.go b/pkg/migrations/op_create_index_test.go index 24ee9986..a233f847 100644 --- a/pkg/migrations/op_create_index_test.go +++ b/pkg/migrations/op_create_index_test.go @@ -12,52 +12,100 @@ import ( func TestCreateIndex(t *testing.T) { t.Parallel() - ExecuteTests(t, TestCases{{ - name: "create index", - migrations: []migrations.Migration{ - { - Name: "01_add_table", - Operations: migrations.Operations{ - &migrations.OpCreateTable{ - Name: "users", - Columns: []migrations.Column{ - { - Name: "id", - Type: "serial", - Pk: ptr(true), - }, - { - Name: "name", - Type: "varchar(255)", - Nullable: ptr(false), + ExecuteTests(t, TestCases{ + { + name: "create index", + migrations: []migrations.Migration{ + { + Name: "01_add_table", + Operations: migrations.Operations{ + &migrations.OpCreateTable{ + Name: "users", + Columns: []migrations.Column{ + { + Name: "id", + Type: "serial", + Pk: ptr(true), + }, + { + Name: "name", + Type: "varchar(255)", + Nullable: ptr(false), + }, }, }, }, }, - }, - { - Name: "02_create_index", - Operations: migrations.Operations{ - &migrations.OpCreateIndex{ - Name: "idx_users_name", - Table: "users", - Columns: []string{"name"}, + { + Name: "02_create_index", + Operations: migrations.Operations{ + &migrations.OpCreateIndex{ + Name: "idx_users_name", + Table: "users", + Columns: []string{"name"}, + }, }, }, }, + afterStart: func(t *testing.T, db *sql.DB, schema string) { + // The index has been created on the underlying table. + IndexMustExist(t, db, schema, "users", "idx_users_name") + }, + afterRollback: func(t *testing.T, db *sql.DB, schema string) { + // The index has been dropped from the the underlying table. + IndexMustNotExist(t, db, schema, "users", "idx_users_name") + }, + afterComplete: func(t *testing.T, db *sql.DB, schema string) { + // Complete is a no-op. + }, }, - afterStart: func(t *testing.T, db *sql.DB, schema string) { - // The index has been created on the underlying table. - IndexMustExist(t, db, schema, "users", "idx_users_name") - }, - afterRollback: func(t *testing.T, db *sql.DB, schema string) { - // The index has been dropped from the the underlying table. - IndexMustNotExist(t, db, schema, "users", "idx_users_name") - }, - afterComplete: func(t *testing.T, db *sql.DB, schema string) { - // Complete is a no-op. + { + name: "create index with a mixed case name", + migrations: []migrations.Migration{ + { + Name: "01_add_table", + Operations: migrations.Operations{ + &migrations.OpCreateTable{ + Name: "users", + Columns: []migrations.Column{ + { + Name: "id", + Type: "serial", + Pk: ptr(true), + }, + { + Name: "name", + Type: "varchar(255)", + Nullable: ptr(false), + }, + }, + }, + }, + }, + { + Name: "02_create_index", + Operations: migrations.Operations{ + &migrations.OpCreateIndex{ + Name: "idx_USERS_name", + Table: "users", + Columns: []string{"name"}, + }, + }, + }, + }, + afterStart: func(t *testing.T, db *sql.DB, schema string) { + // The index has been created on the underlying table. + IndexMustExist(t, db, schema, "users", "idx_USERS_name") + }, + afterRollback: func(t *testing.T, db *sql.DB, schema string) { + // The index has been dropped from the the underlying table. + IndexMustNotExist(t, db, schema, "users", "idx_USERS_name") + }, + afterComplete: func(t *testing.T, db *sql.DB, schema string) { + // Complete is a no-op. + }, }, - }}) + }) } func TestCreateIndexOnMultipleColumns(t *testing.T) { diff --git a/pkg/migrations/op_drop_index.go b/pkg/migrations/op_drop_index.go index dab42ed8..77d1197c 100644 --- a/pkg/migrations/op_drop_index.go +++ b/pkg/migrations/op_drop_index.go @@ -6,6 +6,7 @@ import ( "context" "fmt" + "github.com/lib/pq" "github.com/xataio/pgroll/pkg/db" "github.com/xataio/pgroll/pkg/schema" ) @@ -19,7 +20,8 @@ func (o *OpDropIndex) Start(ctx context.Context, conn db.DB, stateSchema string, func (o *OpDropIndex) Complete(ctx context.Context, conn db.DB, tr SQLTransformer, s *schema.Schema) error { // drop the index concurrently - _, err := conn.ExecContext(ctx, fmt.Sprintf("DROP INDEX CONCURRENTLY IF EXISTS %s", o.Name)) + _, err := conn.ExecContext(ctx, fmt.Sprintf("DROP INDEX CONCURRENTLY IF EXISTS %s", + pq.QuoteIdentifier(o.Name))) return err } diff --git a/pkg/migrations/op_drop_index_test.go b/pkg/migrations/op_drop_index_test.go index 8866c56e..a69e9718 100644 --- a/pkg/migrations/op_drop_index_test.go +++ b/pkg/migrations/op_drop_index_test.go @@ -12,58 +12,114 @@ import ( func TestDropIndex(t *testing.T) { t.Parallel() - ExecuteTests(t, TestCases{{ - name: "drop index", - migrations: []migrations.Migration{ - { - Name: "01_add_table", - Operations: migrations.Operations{ - &migrations.OpCreateTable{ - Name: "users", - Columns: []migrations.Column{ - { - Name: "id", - Type: "serial", - Pk: ptr(true), - }, - { - Name: "name", - Type: "varchar(255)", - Nullable: ptr(false), + ExecuteTests(t, TestCases{ + { + name: "drop index", + migrations: []migrations.Migration{ + { + Name: "01_add_table", + Operations: migrations.Operations{ + &migrations.OpCreateTable{ + Name: "users", + Columns: []migrations.Column{ + { + Name: "id", + Type: "serial", + Pk: ptr(true), + }, + { + Name: "name", + Type: "varchar(255)", + Nullable: ptr(false), + }, }, }, }, }, - }, - { - Name: "02_create_index", - Operations: migrations.Operations{ - &migrations.OpCreateIndex{ - Name: "idx_users_name", - Table: "users", - Columns: []string{"name"}, + { + Name: "02_create_index", + Operations: migrations.Operations{ + &migrations.OpCreateIndex{ + Name: "idx_users_name", + Table: "users", + Columns: []string{"name"}, + }, }, }, - }, - { - Name: "03_drop_index", - Operations: migrations.Operations{ - &migrations.OpDropIndex{ - Name: "idx_users_name", + { + Name: "03_drop_index", + Operations: migrations.Operations{ + &migrations.OpDropIndex{ + Name: "idx_users_name", + }, }, }, }, + afterStart: func(t *testing.T, db *sql.DB, schema string) { + // The index has not yet been dropped. + IndexMustExist(t, db, schema, "users", "idx_users_name") + }, + afterRollback: func(t *testing.T, db *sql.DB, schema string) { + // Rollback is a no-op. + }, + afterComplete: func(t *testing.T, db *sql.DB, schema string) { + // The index has been removed from the underlying table. + IndexMustNotExist(t, db, schema, "users", "idx_users_name") + }, }, - afterStart: func(t *testing.T, db *sql.DB, schema string) { - // The index has not yet been dropped. - IndexMustExist(t, db, schema, "users", "idx_users_name") - }, - afterRollback: func(t *testing.T, db *sql.DB, schema string) { - // Rollback is a no-op. - }, - afterComplete: func(t *testing.T, db *sql.DB, schema string) { - // The index has been removed from the underlying table. - IndexMustNotExist(t, db, schema, "users", "idx_users_name") + { + name: "drop index with a mixed case name", + migrations: []migrations.Migration{ + { + Name: "01_add_table", + Operations: migrations.Operations{ + &migrations.OpCreateTable{ + Name: "users", + Columns: []migrations.Column{ + { + Name: "id", + Type: "serial", + Pk: ptr(true), + }, + { + Name: "name", + Type: "varchar(255)", + Nullable: ptr(false), + }, + }, + }, + }, + }, + { + Name: "02_create_index", + Operations: migrations.Operations{ + &migrations.OpCreateIndex{ + Name: "idx_USERS_name", + Table: "users", + Columns: []string{"name"}, + }, + }, + }, + { + Name: "03_drop_index", + Operations: migrations.Operations{ + &migrations.OpDropIndex{ + Name: "idx_USERS_name", + }, + }, + }, + }, + afterStart: func(t *testing.T, db *sql.DB, schema string) { + // The index has not yet been dropped. + IndexMustExist(t, db, schema, "users", "idx_USERS_name") + }, + afterRollback: func(t *testing.T, db *sql.DB, schema string) { + // Rollback is a no-op. + }, + afterComplete: func(t *testing.T, db *sql.DB, schema string) { + // The index has been removed from the underlying table. + IndexMustNotExist(t, db, schema, "users", "idx_USERS_name") + }, }, - }}) + }) } diff --git a/pkg/state/state.go b/pkg/state/state.go index b5f55dc2..825d95b3 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -174,7 +174,7 @@ BEGIN )), '{}'::json) FROM ( SELECT - reverse(split_part(reverse(pi.indexrelid::regclass::text), '.', 1)) as name, + replace(reverse(split_part(reverse(pi.indexrelid::regclass::text), '.', 1)), '"', '') as name, pi.indisunique, array_agg(a.attname) AS columns FROM pg_index pi