MM-68613: avoid locking posts while resizing message column (#38815)

* MM-68613: avoid locking posts while resizing message column

* Explain why 000217 drops idx_posts_message_txt

* Allow concurrentIndex:ignore to justify a blocking index statement

---------

Co-authored-by: Mattermost Build <build@mattermost.com>

server/channels/db/migrations/migrations.list

@@ -461,3 +461,5 @@ channels/db/migrations/postgres/000232_create_healthfindings_lastseenat_index.do
 channels/db/migrations/postgres/000232_create_healthfindings_lastseenat_index.up.sql
 channels/db/migrations/postgres/000233_strip_sync_attrs_from_non_user_fields.down.sql
 channels/db/migrations/postgres/000233_strip_sync_attrs_from_non_user_fields.up.sql
+channels/db/migrations/postgres/000234_recreate_posts_message_txt_index.down.sql
+channels/db/migrations/postgres/000234_recreate_posts_message_txt_index.up.sql

server/channels/db/migrations/postgres/000217_resize_message_columns.up.sql

@@ -6,6 +6,12 @@ BEGIN
     FROM information_schema.columns
     WHERE table_name = 'posts' AND column_name = 'message';
     IF col_len IS NOT NULL AND col_len < 1048576 THEN
+        -- Postgres rebuilds expression indexes on an altered column while holding
+        -- an ACCESS EXCLUSIVE lock on posts. Drop it here so the ALTER below is
+        -- metadata-only, and let 000234 recreate it with CREATE INDEX CONCURRENTLY.
+        -- concurrentIndex:ignore DROP INDEX CONCURRENTLY would not reduce blocking
+        -- because the ALTER below takes an ACCESS EXCLUSIVE lock regardless.
+        DROP INDEX IF EXISTS idx_posts_message_txt;
         ALTER TABLE posts ALTER COLUMN message TYPE VARCHAR(1048576);
     END IF;
 

server/channels/db/migrations/postgres/000234_recreate_posts_message_txt_index.down.sql

@@ -0,0 +1,2 @@
+-- The index predates this migration, so leave it in place on rollback.
+SELECT 1;

server/channels/db/migrations/postgres/000234_recreate_posts_message_txt_index.up.sql

@@ -0,0 +1,2 @@
+-- morph:nontransactional
+CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_posts_message_txt ON posts USING gin(to_tsvector('english', message));

server/channels/store/sqlstore/migration_000217_test.go

@@ -30,6 +30,7 @@ func TestMigration000217(t *testing.T) {
 
 	upSQL := readMigrationSQL(t, "000217_resize_message_columns.up.sql")
 	downSQL := readMigrationSQL(t, "000217_resize_message_columns.down.sql")
+	recreateIndexSQL := readMigrationSQL(t, "000234_recreate_posts_message_txt_index.up.sql")
 
 	type tableCol struct {
 		table  string
@@ -59,12 +60,21 @@ func TestMigration000217(t *testing.T) {
 		require.NoError(t, alterErr)
 	}
 
+	hasMessageIndex := func(t *testing.T) bool {
+		t.Helper()
+		var count int
+		require.NoError(t, master.Get(&count, "SELECT COUNT(*) FROM pg_indexes WHERE tablename = 'posts' AND indexname = 'idx_posts_message_txt'"))
+		return count == 1
+	}
+
 	restoreTo := func(t *testing.T, size int) {
 		t.Helper()
 		for _, tc := range targets {
 			_, alterErr := master.ExecNoTimeout(fmt.Sprintf("ALTER TABLE %s ALTER COLUMN %s TYPE VARCHAR(%d)", tc.table, tc.column, size))
 			require.NoError(t, alterErr)
 		}
+		_, indexErr := master.ExecNoTimeout(recreateIndexSQL)
+		require.NoError(t, indexErr)
 	}
 
 	t.Run("NormalUpThenDown", func(t *testing.T) {
@@ -74,12 +84,19 @@ func TestMigration000217(t *testing.T) {
 			setColLength(t, tc.table, tc.column, 65535)
 		}
 
+		require.True(t, hasMessageIndex(t), "idx_posts_message_txt should exist before up migration")
+
 		_, err := master.ExecNoTimeout(upSQL)
 		require.NoError(t, err, "up migration should succeed")
 
 		for _, tc := range targets {
 			assert.Equal(t, 1048576, colLength(t, tc.table, tc.column), "%s.%s after up migration", tc.table, tc.column)
 		}
+		assert.False(t, hasMessageIndex(t), "up migration should drop idx_posts_message_txt to avoid rebuilding it under lock")
+
+		_, err = master.ExecNoTimeout(recreateIndexSQL)
+		require.NoError(t, err, "index recreation migration should succeed")
+		assert.True(t, hasMessageIndex(t), "idx_posts_message_txt should be recreated")
 
 		_, err = master.ExecNoTimeout(downSQL)
 		require.NoError(t, err, "down migration should succeed")
@@ -98,6 +115,7 @@ func TestMigration000217(t *testing.T) {
 
 		_, err := master.ExecNoTimeout(upSQL)
 		require.NoError(t, err, "up migration should succeed even when columns are already larger")
+		assert.True(t, hasMessageIndex(t), "idx_posts_message_txt should be kept when posts.message is not altered")
 
 		for _, tc := range targets {
 			assert.Equal(t, 2097152, colLength(t, tc.table, tc.column), "%s.%s should be unchanged by up migration", tc.table, tc.column)

tools/mattermost-govet/concurrentIndex/concurrentIndex.go

@@ -26,6 +26,7 @@ var (
 	concurrentlyRegex     = regexp.MustCompile(`(?i)\bCREATE\s+(UNIQUE\s+)?INDEX\s+CONCURRENTLY\b`)
 	dropIndexRegex        = regexp.MustCompile(`(?i)\bDROP\s+INDEX\b`)
 	dropConcurrentlyRegex = regexp.MustCompile(`(?i)\bDROP\s+INDEX\s+CONCURRENTLY\b`)
+	ignoreRegex           = regexp.MustCompile(`--\s*concurrentIndex:ignore\b`)
 )
 
 var sqlPath string
@@ -45,6 +46,9 @@ func init() {
 }
 
 func checkStatement(stmt string) string {
+	if ignoreRegex.MatchString(stmt) {
+		return ""
+	}
 	if createIndexRegex.MatchString(stmt) && !concurrentlyRegex.MatchString(stmt) {
 		return diagCreateIndex
 	}

tools/mattermost-govet/concurrentIndex/concurrentIndex_test.go

@@ -189,6 +189,15 @@ func TestCheckLine(t *testing.T) {
 			line:    "DROP INDEX CONCURRENTLY IF EXISTS idx_a; DROP INDEX IF EXISTS idx_b;",
 			wantMsg: diagDropIndex,
 		},
+		{
+			name: "ignore comment suppresses statement",
+			line: "-- concurrentIndex:ignore lock is taken anyway\nDROP INDEX IF EXISTS idx_foo;",
+		},
+		{
+			name:    "ignore comment only applies to its own statement",
+			line:    "-- concurrentIndex:ignore lock is taken anyway\nDROP INDEX IF EXISTS idx_a; DROP INDEX IF EXISTS idx_b;",
+			wantMsg: diagDropIndex,
+		},
 		{
 			name: "multi-statement all safe",
 			line: "CREATE INDEX CONCURRENTLY idx_a ON t (c); DROP INDEX CONCURRENTLY IF EXISTS idx_b;",

tools/mattermost-govet/concurrentIndex/testdata/migrations/000006_ignored.up.sql

@@ -0,0 +1,6 @@
+DO $$
+BEGIN
+    -- concurrentIndex:ignore the ALTER below takes an ACCESS EXCLUSIVE lock regardless
+    DROP INDEX IF EXISTS idx_foo_bar;
+    ALTER TABLE foo ALTER COLUMN bar TYPE VARCHAR(1024);
+END $$;