upgrade: adding is_draining to system.sql_instance can fail #135737

fqazi · 2024-11-19T19:56:18Z

Previously, the logic to add the is_draining column to the system.sql_instance descriptor would copy the bootstrap descriptor directly. While this works fine for non-multiregion system databases, this approach breaks for multi-region system databases. This is because bootstrap descriptors do not have multi-region modifications applied on top. To address this, this change modifies the upgrade to use ALTER TABLE ADD COLUMN.

Fixes: #135736

Release note: None

cockroach-teamcity · 2024-11-19T19:56:30Z

This change is

rafiss

can we write a test that tests this upgrade with a multiregion system DB?

rafiss · 2024-11-19T20:02:14Z

pkg/upgrade/upgrades/v24_3_sql_instances_add_draining.go

 ) error {
-	finalDescriptor := systemschema.SQLInstancesTable()
 	// Replace the stored descriptor with the bootstrap descriptor.


is this comment outdated now?

fqazi

@rafiss This PR reintroduces TestMrSystemDatabaseUpgrade which does that for MR system databases. 24.3 has the same test but for some reason it didn't use Latest as the final version.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @stevendanna)

fqazi · 2024-11-19T20:08:01Z

pkg/upgrade/upgrades/v24_3_sql_instances_add_draining.go

 ) error {
-	finalDescriptor := systemschema.SQLInstancesTable()
 	// Replace the stored descriptor with the bootstrap descriptor.


rafiss · 2024-11-19T21:48:00Z

[w0] 2024/11/19 20:34:18 test_runner.go:964: destroying cluster local [tag:] (1 nodes) because: acceptance/validate-system-schema-after-version-upgrade (1) - (validate_system_schema_after_version_upgrade.go:167).validateSystemSchemaAfterUpgradeTest: After upgrading, `USE system; SHOW CREATE ALL TABLES;` does not match expected output after version upgrade for system tenant: diff:
@@ -305,13 +305,11 @@
 	locality JSONB NULL,
 	sql_addr STRING NULL,
 	crdb_region BYTES NOT NULL,
 	binary_version STRING NULL,
 	is_draining BOOL NULL,
-	CONSTRAINT "primary" PRIMARY KEY (crdb_region ASC, id ASC),
-	FAMILY "primary" (id, addr, session_id, locality, sql_addr, crdb_region, binary_version),
-	FAMILY fam_8_is_draining (is_draining)
+	CONSTRAINT "primary" PRIMARY KEY (crdb_region ASC, id ASC)
 );

looks like we need to teach the migration to add the column to the correct column family.

fqazi

@rafiss Should be fixed now, made a bad assumption that it would just pick the only family. Re-ran locally to confirm

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss, @shubhamdhama, and @stevendanna)

stevendanna

Thanks for picking this up!

Left some observations but no blocking changes.

stevendanna · 2024-11-20T04:03:07Z

pkg/upgrade/upgrades/v24_3_sql_instances_add_draining.go

-		mutableDesc.Version = version
-		return txn.Descriptors().WriteDesc(ctx, false, mutableDesc, txn.KV())
+		_, err := txn.Exec(ctx, "add-draining-column", txn.KV(),
+			`ALTER TABLE system.sql_instances ADD COLUMN IF NOT EXISTS is_draining BOOL NULL FAMILY "primary"`)


Question: We have a helper function called migrateTable that does a lot of work that I sort of assume the schema changer already does for us. I suppose we only need that when we want to be idempotent based on some property other than the existence of a column.

TIL: I'm rather surprised we had to specify family here since you wouldn't need to for an ordinary table. But it looks like we have a special case in allocateColumnFamilyIDs for the system db.

Done.

Its a bit safer to use that, so I switched the code over to it. The advantage of that one is that it waits for pending schema changes too (in case something crashes).

shubhamdhama · 2024-11-20T05:37:19Z

pkg/ccl/multiregionccl/multiregion_system_table_test.go

+	// Disable license enforcement for this test.
+	for _, s := range cluster.Servers {
+		s.ExecutorConfig().(sql.ExecutorConfig).LicenseEnforcer.Disable(ctx)
+	}


Question: I'm guessing this might be needed outside ccl/multiregionccl package but not here, right?

One of the upgrades in 24.3 validates if a valid license is set, so this bypasses this for the upgrade testing. The other option was generating a license in the unit test, but that seems overkill.

shubhamdhama · 2024-11-20T08:12:13Z

Also, thanks for taking over this.

Previously, the logic to add the is_draining column to the system.sql_instance descriptor would copy the bootstrap descriptor directly. While this works fine for non-multiregion system databases, this approach breaks for multi-region system databases. This is because bootstrap descriptors do not have multi-region modifications applied on top. To address this, this change modifies the upgrade to use ALTER TABLE ADD COLUMN. Fixes: cockroachdb#135736 Release note: None

fqazi

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss, @shubhamdhama, and @stevendanna)

fqazi · 2024-11-20T14:15:47Z

pkg/upgrade/upgrades/v24_3_sql_instances_add_draining.go

-		mutableDesc.Version = version
-		return txn.Descriptors().WriteDesc(ctx, false, mutableDesc, txn.KV())
+		_, err := txn.Exec(ctx, "add-draining-column", txn.KV(),
+			`ALTER TABLE system.sql_instances ADD COLUMN IF NOT EXISTS is_draining BOOL NULL FAMILY "primary"`)


Done.

Its a bit safer to use that, so I switched the code over to it. The advantage of that one is that it waits for pending schema changes too (in case something crashes).

fqazi · 2024-11-20T14:15:48Z

pkg/ccl/multiregionccl/multiregion_system_table_test.go

+	// Disable license enforcement for this test.
+	for _, s := range cluster.Servers {
+		s.ExecutorConfig().(sql.ExecutorConfig).LicenseEnforcer.Disable(ctx)
+	}


One of the upgrades in 24.3 validates if a valid license is set, so this bypasses this for the upgrade testing. The other option was generating a license in the unit test, but that seems overkill.

fqazi · 2024-11-20T15:49:11Z

@rafiss @stevendanna @shubhamdhama TFTR!

bors r+

craig · 2024-11-20T16:41:12Z

Build succeeded:

blathers-crl · 2024-11-20T16:41:17Z

Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches.

Issue #135736: branch-release-24.3.0-rc.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

blathers-crl · 2024-11-20T16:41:22Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from 0509190 to blathers/backport-release-24.3-135737: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 24.3.x failed. See errors above.

error creating merge commit from 0509190 to blathers/backport-release-24.3.0-rc-135737: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 24.3.0-rc failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

fqazi added backport-24.3.x Flags PRs that need to be backported to 24.3 backport-24.3.0-rc labels Nov 19, 2024

fqazi requested review from stevendanna and a team November 19, 2024 19:56

fqazi requested review from a team as code owners November 19, 2024 19:56

rafiss reviewed Nov 19, 2024

View reviewed changes

fqazi force-pushed the fixMigrationForSQLInstance branch from 50584f1 to c36ef72 Compare November 19, 2024 20:06

fqazi commented Nov 19, 2024

View reviewed changes

fqazi requested a review from rafiss November 19, 2024 21:05

rafiss requested a review from shubhamdhama November 19, 2024 21:44

fqazi force-pushed the fixMigrationForSQLInstance branch from c36ef72 to ae3184d Compare November 19, 2024 23:06

fqazi commented Nov 19, 2024

View reviewed changes

stevendanna approved these changes Nov 20, 2024

View reviewed changes

shubhamdhama approved these changes Nov 20, 2024

View reviewed changes

shubhamdhama mentioned this pull request Nov 20, 2024

sql: fix migration to add is_draining in sql_instances #135683

Closed

fqazi force-pushed the fixMigrationForSQLInstance branch from ae3184d to 0509190 Compare November 20, 2024 14:15

fqazi commented Nov 20, 2024

View reviewed changes

craig bot merged commit 873e633 into cockroachdb:master Nov 20, 2024
22 of 23 checks passed

blathers-crl bot mentioned this pull request Nov 20, 2024

upgrade: adding is_draining to sql_instances fails on MR system database #135736

Closed

This was referenced Nov 20, 2024

release-24.3: upgrade: adding is_draining to system.sql_instance can fail #135832

Merged

release-24.3.0-rc: upgrade: adding is_draining to system.sql_instance can fail #135833

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upgrade: adding is_draining to system.sql_instance can fail #135737

upgrade: adding is_draining to system.sql_instance can fail #135737

fqazi commented Nov 19, 2024

cockroach-teamcity commented Nov 19, 2024

rafiss left a comment

rafiss Nov 19, 2024

fqazi Nov 19, 2024

fqazi left a comment

fqazi Nov 19, 2024

rafiss commented Nov 19, 2024

fqazi left a comment

stevendanna left a comment

stevendanna Nov 20, 2024

fqazi Nov 20, 2024

shubhamdhama Nov 20, 2024

fqazi Nov 20, 2024

shubhamdhama commented Nov 20, 2024

fqazi left a comment

fqazi Nov 20, 2024

fqazi Nov 20, 2024

fqazi commented Nov 20, 2024

craig bot commented Nov 20, 2024

blathers-crl bot commented Nov 20, 2024

blathers-crl bot commented Nov 20, 2024

upgrade: adding is_draining to system.sql_instance can fail #135737

upgrade: adding is_draining to system.sql_instance can fail #135737

Conversation

fqazi commented Nov 19, 2024

cockroach-teamcity commented Nov 19, 2024

rafiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fqazi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafiss commented Nov 19, 2024

fqazi left a comment

Choose a reason for hiding this comment

stevendanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubhamdhama commented Nov 20, 2024

fqazi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fqazi commented Nov 20, 2024

craig bot commented Nov 20, 2024

blathers-crl bot commented Nov 20, 2024

blathers-crl bot commented Nov 20, 2024