Welcome back!

Your commit message needs some editing:

1. For a start there's a "mend" string that made its way in there by mistake.
2. Next, your use of the word "migration" instead of "upgrade" is quite confusing. Clusters upgrade from one version to the next, and in so doing they might run migrations, but they're not the same thing.
3. If we don't have an issue for this (which is possible) can you please create one and link it to the jira epic?
4. Can you flesh out your release note some more? This is very terse considering the impact on our users, never mind our documentations team.

Unrelated but why are there changes to migrations.pb.go checked in this commit?

Reviewed 1 of 9 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi)

pkg/migration/migrations/interleaved_tables.go, line 36 at r1 (raw file):

	}
	return nil
}

What is the purpose of this migration? My understanding is you need to version-gate the creation of new interleaves, such that when the migration runs in the next version, we are guaranteed that there are no concurrent interleave creations. Such as it is, we're checking that there are no interleaves twice, which isn't cheap, for no apparent good reason (to me).

pkg/migration/migrations/interleaved_tables.go, line 47 at r1 (raw file):

	txn := d.DB.NewTxn(ctx, "interleaved-check")
	rows, err := d.InternalExecutor.QueryRowEx(ctx, "check-for-interleaved", txn, sessiondata.InternalExecutorOverride{User: security.RootUserName()},
		"select * from crdb_internal.interleaved")

I may be wrong, but the way this table is built, this query seems awfully expensive. The table generator follows the interleave references to get the interleaved table names, etc. Plus we don't actually care about all the rows in this table only whether there are none or not. Perhaps consider adding a LIMIT 1? Not sure if this helps. This is worth testing for.

Alternatively, and arguably preferably, consider bypassing the table and instead reimplement the logic from the table generator fn in this migration. It's simple enough. That way you don't need to change the semantics of the table by including dropped indexes like you did. Besides, it seems to me that we'll want to remove that table sooner rather than later. In fact, why not do so in this PR?

pkg/migration/migrations/interleaved_tables_external_test.go, line 122 at r1 (raw file):

	// Migration should succeed.
	tdb.ExecSucceedsSoon(t, "SET CLUSTER SETTING version = $1",
		clusterversion.ByKey(clusterversion.InterleavedTablesRemovedMigration).String())

perhaps also add an assertion on a CREATE TABLE ... INTERLEAVE IN PARENT expecting an interleavedTableDisabledMigrationError, post-upgrade?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @postamar)

pkg/migration/migrations/interleaved_tables.go, line 36 at r1 (raw file):

So, the first stage is sort of prone to race conditions it only validates no interleaves exist at the current point in time, without blocking CREATE TABLE / CREATE INDEXES or other operations. So, the first stage disables those operations only, the second once again validates no interleaved objects exist.

pkg/migration/migrations/interleaved_tables.go, line 47 at r1 (raw file):

ably preferably, consider bypassing the table and instead reimplement the logi
We can't drop the internal table yet? If you haven't gone past this migration you still need that function to figure out the source of your problem. I think once your past this point on the next release we can drop all the code right?

Let me dig into using the code from the CRDB function directly, or confirm if the limit clause is sufficient.

pkg/migration/migrations/interleaved_tables_external_test.go, line 122 at r1 (raw file):

Good point, let me confirm that new error shows up :)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @irfansharif, @otan, and @postamar)

pkg/migration/migrations/interleaved_tables.go, line 47 at r1 (raw file):

Done.

pkg/sql/crdb_internal.go, line 4538 at r15 (raw file):

Done.

Another thing that came up was the possibility of silently ignoring INTERLEAVE statements at some point. We can do that as a follow-up but let's make sure we track it if that's the decision.

Did we track this?

Reviewed 1 of 34 files at r16, 40 of 76 files at r20, 4 of 14 files at r22, 1 of 8 files at r23, 1 of 8 files at r25.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @fqazi, @irfansharif, @otan, and @postamar)

pkg/ccl/kvccl/kvtenantccl/tenant_upgrade_test.go, line 260 at r25 (raw file):

						if !cv.Version.Equal(v1) && !cv.Version.Equal(v2) {
							return nil, false
						}

nit: just make this the default case.

pkg/ccl/kvccl/kvtenantccl/tenant_upgrade_test.go, line 324 at r25 (raw file):

		// will fail on v1.
		initialTenantRunner.ExpectErr(t,
			".*(sql: database is closed)*(failed to connect)*.*",

This will match literally any string. What about sql: database is closed|failed to connect.

pkg/migration/migrations/interleaved_tables_external_test.go, line 84 at r25 (raw file):

	})
	// Check that creation of interleaved tables is fully disabled.
	tdb.ExecSucceedsSoon(t, "CREATE TABLE customers2 (id INT PRIMARY KEY, name STRING(50));")

Does this needs a succeeds soon?

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @irfansharif, @otan, and @postamar)

pkg/ccl/kvccl/kvtenantccl/tenant_upgrade_test.go, line 324 at r25 (raw file):

Done.

pkg/clusterversion/cockroach_versions.go, line 272 at r8 (raw file):

Done.