86b649d · reshape · git.iko.soy

git.iko.soy/reshape

Copy indices to temporary columns for alter_column

Until now, using alter_column would cause indices to be lost as they weren't copied to the temporary column. This fixes that for indices that consist of one or more actual columns. It won't work for indices on expressions.

commit86b649d14a8ddbe4d8325d2c5f42e6b763b8dd12 authorfabianlindfors <fabian@flapplabs.se> dateThu 13 Jan 2022, 18:19 (5 years ago) parentb86e6d5

4 files changed+268−16

compare with main

unifiedsplit

src/migrations/alter_column.rs+87−1 src/migrations/common.rs+77 tests/alter_column.rs+90−1 tests/create_table.rs+14−14

src/migrations/alter_column.rs+87−1

@@ -150,6 +150,33 @@ impl Action for AlterColumn {

150150 common::batch_touch_rows(db, &table.real_name, &column.real_name)

151151 .context("failed to batch update existing rows")?;

152152

153+ // Duplicate any indices to the temporary column

154+ let indices = common::get_indices_for_column(db, &table.real_name, &column.real_name)?;

155+ for (index_name, index_oid) in indices {

156+ let index_columns: Vec<String> = common::get_index_columns(db, &index_name)?

157+ .into_iter()

158+ .map(|idx_column| {

159+ // Replace column with temporary column for new index

160+ if idx_column == column.real_name {

161+ temporary_column_name.to_string()

162+ } else {

163+ idx_column

164+ }

165+ })

166+ .collect();

167+ let temp_index_name = self.temp_index_name(ctx, index_oid);

168+

169+ db.query(&format!(

170+ r#"

171+ CREATE INDEX CONCURRENTLY IF NOT EXISTS "{new_index_name}" ON "{table}" ({columns})

172+ "#,

173+ new_index_name = temp_index_name,

174+ table = table.real_name,

175+ columns = index_columns.join(", "),

176+ ))

177+ .context("failed to create temporary index")?;

178+ }

179+

153180 // Add a temporary NOT NULL constraint if the column shouldn't be nullable.

154181 // This constraint is set as NOT VALID so it doesn't apply to existing rows and

155182 // the existing rows don't need to be scanned under an exclusive lock.

@@ -246,10 +273,52 @@ impl Action for AlterColumn {

246273 .context("failed to drop NOT NULL constraint")?;

247274 }

248275

276+ // Replace old indices with the new temporary ones created for the temporary column

277+ let indices = common::get_indices_for_column(db, &self.table, &self.column)?;

278+ for (current_index_name, index_oid) in indices {

279+ // To keep the index handling idempotent, we need to do the following:

280+ // 1. Add a prefix to the existing index

281+ // 2. Rename temporary index to its final name

282+ // 3. Drop existing index concurrently

283+

284+ // Add prefix (if not already added) to existing index

285+ let prefix = "__reshape_old";

286+ let target_index_name = current_index_name.trim_start_matches(prefix);

287+ let old_index_name = format!("{}_{}", prefix, target_index_name);

288+ db.query(&format!(

289+ r#"

290+ ALTER INDEX IF EXISTS "{current_name}" RENAME TO "{new_name}"

291+ "#,

292+ current_name = target_index_name,

293+ new_name = old_index_name,

294+ ))

295+ .context("failed to rename old index")?;

296+

297+ // Rename temporary index to real name

298+ let temp_index_name = self.temp_index_name(ctx, index_oid);

299+ db.query(&format!(

300+ r#"

301+ ALTER INDEX IF EXISTS "{temp_index_name}" RENAME TO "{target_index_name}"

302+ "#,

303+ temp_index_name = temp_index_name,

304+ target_index_name = target_index_name,

305+ ))

306+ .context("failed to rename temporary index")?;

307+

308+ // Drop old index concurrently

309+ db.query(&format!(

310+ r#"

311+ DROP INDEX CONCURRENTLY IF EXISTS "{old_index_name}"

312+ "#,

313+ old_index_name = old_index_name,

314+ ))

315+ .context("failed to drop old index")?;

316+ }

317+

249318 // Remove old column

250319 let query = format!(

251320 r#"

252− ALTER TABLE "{table}" DROP COLUMN "{column}" CASCADE

321+ ALTER TABLE "{table}" DROP COLUMN IF EXISTS "{column}" CASCADE

253322 "#,

254323 table = self.table,

255324 column = self.column,

@@ -311,6 +380,19 @@ impl Action for AlterColumn {

311380 }

312381

313382 fn abort(&self, ctx: &MigrationContext, db: &mut dyn Conn) -> anyhow::Result<()> {

383+ // Safely remove any indices created for the temporary column

384+ let temp_column_name = self.temporary_column_name(ctx);

385+ let indices = common::get_indices_for_column(db, &self.table, &temp_column_name)?;

386+ for (_, index_oid) in indices {

387+ let temp_index_name = self.temp_index_name(ctx, index_oid);

388+ db.query(&format!(

389+ r#"

390+ DROP INDEX CONCURRENTLY IF EXISTS "{index_name}"

391+ "#,

392+ index_name = temp_index_name,

393+ ))?;

394+ }

395+

314396 // Drop temporary column

315397 let query = format!(

316398 r#"

@@ -359,6 +441,10 @@ impl AlterColumn {

359441 format!("{}_alter_column_temporary", ctx.prefix())

360442 }

361443

444+ fn temp_index_name(&self, ctx: &MigrationContext, index_oid: u32) -> String {

445+ format!("{}_alter_column_temp_index_{}", ctx.prefix(), index_oid)

446+ }

447+

362448 fn can_short_circuit(&self) -> bool {

363449 self.changes.name.is_some()

364450 && self.changes.data_type.is_none()

src/migrations/common.rs+77

@@ -1,3 +1,4 @@

1+use anyhow::anyhow;

12use derive_builder::Builder;

23use postgres::types::{FromSql, ToSql};

34use serde::{Deserialize, Serialize};

@@ -180,3 +181,79 @@ fn get_primary_key_columns_for_table(

180181

181182 Ok(primary_key_columns)

182183}

184+

185+pub fn get_indices_for_column(

186+ db: &mut dyn Conn,

187+ table: &str,

188+ column: &str,

189+) -> anyhow::Result<Vec<(String, u32)>> {

190+ let indices = db

191+ .query(&format!(

192+ "

193+ SELECT i.relname AS index_name, i.oid AS index_oid

194+ FROM pg_index ix

195+ JOIN pg_class t ON t.oid = ix.indrelid

196+ JOIN pg_class i ON i.oid = ix.indexrelid

197+ JOIN pg_attribute a ON

198+ a.attrelid = t.oid AND

199+ a.attnum = ANY(ix.indkey)

200+ WHERE

201+ t.relname = '{table}' AND

202+ a.attname = '{column}'

203+ ",

204+ table = table,

205+ column = column,

206+ ))?

207+ .iter()

208+ .map(|row| (row.get("index_name"), row.get("index_oid")))

209+ .collect();

210+

211+ Ok(indices)

212+}

213+

214+pub fn get_index_columns(db: &mut dyn Conn, index_name: &str) -> anyhow::Result<Vec<String>> {

215+ // Get all columns which are part of the index in order

216+ let (table_oid, column_nums) = db

217+ .query(&format!(

218+ "

219+ SELECT t.oid AS table_oid, ix.indkey::INTEGER[] AS columns

220+ FROM pg_index ix

221+ JOIN pg_class t ON t.oid = ix.indrelid

222+ JOIN pg_class i ON i.oid = ix.indexrelid

223+ WHERE

224+ i.relname = '{index_name}'

225+ ",

226+ index_name = index_name,

227+ ))?

228+ .first()

229+ .map(|row| {

230+ (

231+ row.get::<'_, _, u32>("table_oid"),

232+ row.get::<'_, _, Vec<i32>>("columns"),

233+ )

234+ })

235+ .ok_or_else(|| anyhow!("failed to get columns for index"))?;

236+

237+ // Get the name of each of the columns, still in order

238+ column_nums

239+ .iter()

240+ .map(|column_num| -> anyhow::Result<String> {

241+ let name: String = db

242+ .query(&format!(

243+ "

244+ SELECT attname AS name

245+ FROM pg_attribute

246+ WHERE attrelid = {table_oid}

247+ AND attnum = {column_num};

248+ ",

249+ table_oid = table_oid,

250+ column_num = column_num,

251+ ))?

252+ .first()

253+ .map(|row| row.get("name"))

254+ .ok_or_else(|| anyhow!("expected to find column"))?;

255+

256+ Ok(name)

257+ })

258+ .collect::<anyhow::Result<Vec<String>>>()

259+}

tests/alter_column.rs+90−1

@@ -1,5 +1,5 @@

11use reshape::migrations::{

2− AlterColumn, ColumnBuilder, ColumnChanges, CreateTableBuilder, Migration,

2+ AddIndex, AlterColumn, ColumnBuilder, ColumnChanges, CreateTableBuilder, Migration,

33};

44use reshape::Status;

@@ -531,3 +531,92 @@ fn alter_column_default() {

531531 reshape.complete_migration().unwrap();

532532 common::assert_cleaned_up(&mut new_db);

533533}

534+

535+#[test]

536+fn alter_column_with_index() {

537+ let (mut reshape, mut db, _) = common::setup();

538+

539+ let create_users_table = Migration::new("create_user_table", None)

540+ .with_action(

541+ CreateTableBuilder::default()

542+ .name("users")

543+ .primary_key(vec!["id".to_string()])

544+ .columns(vec![

545+ ColumnBuilder::default()

546+ .name("id")

547+ .data_type("INTEGER")

548+ .build()

549+ .unwrap(),

550+ ColumnBuilder::default()

551+ .name("first_name")

552+ .data_type("TEXT")

553+ .build()

554+ .unwrap(),

555+ ColumnBuilder::default()

556+ .name("last_name")

557+ .data_type("TEXT")

558+ .build()

559+ .unwrap(),

560+ ])

561+ .build()

562+ .unwrap(),

563+ )

564+ .with_action(AddIndex {

565+ table: "users".to_string(),

566+ name: "users_name_idx".to_string(),

567+ columns: vec!["first_name".to_string(), "last_name".to_string()],

568+ });

569+ let uppercase_name = Migration::new("uppercase_name", None).with_action(AlterColumn {

570+ table: "users".to_string(),

571+ column: "last_name".to_string(),

572+ up: Some("UPPER(last_name)".to_string()),

573+ down: Some("LOWER(last_name)".to_string()),

574+ changes: ColumnChanges {

575+ data_type: None,

576+ nullable: None,

577+ name: None,

578+ default: None,

579+ },

580+ });

581+

582+ let first_migrations = vec![create_users_table.clone()];

583+ let second_migrations = vec![create_users_table.clone(), uppercase_name.clone()];

584+

585+ // Run first migration, should automatically finish

586+ reshape.migrate(first_migrations.clone()).unwrap();

587+ assert!(matches!(reshape.state.status, Status::Idle));

588+ assert_eq!(

589+ Some(&create_users_table.name),

590+ reshape.state.current_migration.as_ref()

591+ );

592+

593+ // Run second migration

594+ reshape.migrate(second_migrations.clone()).unwrap();

595+ db.simple_query(&reshape::schema_query_for_migration(

596+ &second_migrations.last().unwrap().name,

597+ ))

598+ .unwrap();

599+

600+ // Complete the second migration which should replace the existing column

601+ // with the temporary one

602+ reshape.complete_migration().unwrap();

603+

604+ // Make sure index still exists

605+ let result: i64 = db

606+ .query(

607+ "

608+ SELECT COUNT(*)

609+ FROM pg_catalog.pg_index

610+ JOIN pg_catalog.pg_class ON pg_index.indexrelid = pg_class.oid

611+ WHERE pg_class.relname = 'users_name_idx'

612+ ",

613+ &[],

614+ )

615+ .unwrap()

616+ .first()

617+ .map(|row| row.get(0))

618+ .unwrap();

619+ assert_eq!(1, result, "expected index to still exist");

620+

621+ common::assert_cleaned_up(&mut db);

622+}

tests/create_table.rs+14−14

@@ -176,20 +176,20 @@ fn create_table_with_foreign_keys() {

176176 let foreign_key_columns: Vec<(String, String, String)> = db

177177 .query(

178178 "

179− SELECT

180− kcu.column_name,

181− ccu.table_name AS foreign_table_name,

182− ccu.column_name AS foreign_column_name

183− FROM

184− information_schema.table_constraints AS tc

185− JOIN information_schema.key_column_usage AS kcu

186− ON tc.constraint_name = kcu.constraint_name

187− AND tc.table_schema = kcu.table_schema

188− JOIN information_schema.constraint_column_usage AS ccu

189− ON ccu.constraint_name = tc.constraint_name

190− AND ccu.table_schema = tc.table_schema

191− WHERE tc.constraint_type = 'FOREIGN KEY' AND tc.table_name='items';

192− ",

179+ SELECT

180+ kcu.column_name,

181+ ccu.table_name AS foreign_table_name,

182+ ccu.column_name AS foreign_column_name

183+ FROM

184+ information_schema.table_constraints AS tc

185+ JOIN information_schema.key_column_usage AS kcu

186+ ON tc.constraint_name = kcu.constraint_name

187+ AND tc.table_schema = kcu.table_schema

188+ JOIN information_schema.constraint_column_usage AS ccu

189+ ON ccu.constraint_name = tc.constraint_name

190+ AND ccu.table_schema = tc.table_schema

191+ WHERE tc.constraint_type = 'FOREIGN KEY' AND tc.table_name='items';

192+ ",

193193 &[],

194194 )

195195 .unwrap()