Skip to content

Commit 186f260

Browse files
craig[bot]Nukittjbowensjeffswenson
committed
151870: sql,multiregion: prevent DROP REGION in user DB from cleaning up `system.sql_instances` r=fqazi a=Nukitt Previously, DROP REGION cleanup logic for system.sql_instances was unintentionally applied to all databases when in multiregion setup, not just the system database. The original intent was to only perform this cleanup when extending `DROP REGION` for the system database. However, the logic leaked through, affecting user databases as well. This PR implements the fix described in #151789, and after a quick check of these changes with the blocked tests in the #150730, this fixes them plus passes the original tests created for testing drop region in system database. Epic: None Fixes: #151789 Release note: None 151936: go.mod: bump Pebble to eaca9d0cc5ea r=jbowens a=jbowens Changes: * [`eaca9d0c`](cockroachdb/pebble@eaca9d0c) strparse: add (*Parser).Offset * [`f8b8a139`](cockroachdb/pebble@f8b8a139) db: include size of input, output files for blob file rewrites * [`d143b0d8`](cockroachdb/pebble@d143b0d8) compact: convert MERGE keys to SET when bottommost Release note: none. Epic: none. 152014: roachtest: fix client retry token bucket setting r=jeffswenson a=jeffswenson Previously, backup fixtures were failing to generate due to a misnamed setting. The setting was originally correct, but became incorrect when I renamed the setting to fix the setting name lint. We don't have to backport this because the roachtest change hit a merge conflict during the backport and I removed the roachtest changes from the backport since they were non-essential. Release note: none Informs: #148334 Co-authored-by: Nukitt <[email protected]> Co-authored-by: Jackson Owens <[email protected]> Co-authored-by: Jeff Swenson <[email protected]>
4 parents 5e0e6bc + bb61a22 + ca90f3e + be29b6b commit 186f260

File tree

7 files changed

+54
-11
lines changed

7 files changed

+54
-11
lines changed

DEPS.bzl

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1850,10 +1850,10 @@ def go_deps():
18501850
patches = [
18511851
"@com_github_cockroachdb_cockroach//build/patches:com_github_cockroachdb_pebble.patch",
18521852
],
1853-
sha256 = "668f4a5ade4ee097a7f6f504672e202032329b11f1d0191361d75223922b1986",
1854-
strip_prefix = "github.com/cockroachdb/[email protected]20250812190039-9be04468f598",
1853+
sha256 = "2a9f6fbb608f3db62bdce84934a7707cddb3ba1191dcb6978a8c78395c0ec0db",
1854+
strip_prefix = "github.com/cockroachdb/[email protected]20250813233102-eaca9d0cc5ea",
18551855
urls = [
1856-
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/pebble/com_github_cockroachdb_pebble-v0.0.0-20250812190039-9be04468f598.zip",
1856+
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/pebble/com_github_cockroachdb_pebble-v0.0.0-20250813233102-eaca9d0cc5ea.zip",
18571857
],
18581858
)
18591859
go_repository(

build/bazelutil/distdir_files.bzl

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -359,7 +359,7 @@ DISTDIR_FILES = {
359359
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/gostdlib/com_github_cockroachdb_gostdlib-v1.19.0.zip": "c4d516bcfe8c07b6fc09b8a9a07a95065b36c2855627cb3514e40c98f872b69e",
360360
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/logtags/com_github_cockroachdb_logtags-v0.0.0-20241215232642-bb51bb14a506.zip": "920068af09e3846d9ebb4e4a7787ff1dd10f3989c5f940ad861b0f6a9f824f6e",
361361
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/metamorphic/com_github_cockroachdb_metamorphic-v0.0.0-20231108215700-4ba948b56895.zip": "28c8cf42192951b69378cf537be5a9a43f2aeb35542908cc4fe5f689505853ea",
362-
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/pebble/com_github_cockroachdb_pebble-v0.0.0-20250812190039-9be04468f598.zip": "668f4a5ade4ee097a7f6f504672e202032329b11f1d0191361d75223922b1986",
362+
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/pebble/com_github_cockroachdb_pebble-v0.0.0-20250813233102-eaca9d0cc5ea.zip": "2a9f6fbb608f3db62bdce84934a7707cddb3ba1191dcb6978a8c78395c0ec0db",
363363
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/redact/com_github_cockroachdb_redact-v1.1.6.zip": "018eccb5fb9ca52d43ec9eaf213539d01c1f2b94e0e822406ebfb2e9321ef6cf",
364364
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/returncheck/com_github_cockroachdb_returncheck-v0.0.0-20200612231554-92cdbca611dd.zip": "ce92ba4352deec995b1f2eecf16eba7f5d51f5aa245a1c362dfe24c83d31f82b",
365365
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/stress/com_github_cockroachdb_stress-v0.0.0-20220803192808-1806698b1b7b.zip": "3fda531795c600daf25532a4f98be2a1335cd1e5e182c72789bca79f5f69fcc1",

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ require (
136136
github.com/cockroachdb/errors v1.12.0
137137
github.com/cockroachdb/gostdlib v1.19.0
138138
github.com/cockroachdb/logtags v0.0.0-20241215232642-bb51bb14a506
139-
github.com/cockroachdb/pebble v0.0.0-20250812190039-9be04468f598
139+
github.com/cockroachdb/pebble v0.0.0-20250813233102-eaca9d0cc5ea
140140
github.com/cockroachdb/redact v1.1.6
141141
github.com/cockroachdb/returncheck v0.0.0-20200612231554-92cdbca611dd
142142
github.com/cockroachdb/tokenbucket v0.0.0-20250429170803-42689b6311bb

go.sum

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -583,8 +583,8 @@ github.com/cockroachdb/logtags v0.0.0-20241215232642-bb51bb14a506 h1:ASDL+UJcILM
583583
github.com/cockroachdb/logtags v0.0.0-20241215232642-bb51bb14a506/go.mod h1:Mw7HqKr2kdtu6aYGn3tPmAftiP3QPX63LdK/zcariIo=
584584
github.com/cockroachdb/metamorphic v0.0.0-20231108215700-4ba948b56895 h1:XANOgPYtvELQ/h4IrmPAohXqe2pWA8Bwhejr3VQoZsA=
585585
github.com/cockroachdb/metamorphic v0.0.0-20231108215700-4ba948b56895/go.mod h1:aPd7gM9ov9M8v32Yy5NJrDyOcD8z642dqs+F0CeNXfA=
586-
github.com/cockroachdb/pebble v0.0.0-20250812190039-9be04468f598 h1:38IyfrdbyCapHZZS2A6uCBROisJ3Cy0IirleW955AD0=
587-
github.com/cockroachdb/pebble v0.0.0-20250812190039-9be04468f598/go.mod h1:86lLSKhilQEdaYPIVAG2mIlDGhxlKX1CUkr4Z09nCzA=
586+
github.com/cockroachdb/pebble v0.0.0-20250813233102-eaca9d0cc5ea h1:tKjg6T5xxVQTTOZP9f7pgRcjy7LVhyYpIeijndvApPY=
587+
github.com/cockroachdb/pebble v0.0.0-20250813233102-eaca9d0cc5ea/go.mod h1:86lLSKhilQEdaYPIVAG2mIlDGhxlKX1CUkr4Z09nCzA=
588588
github.com/cockroachdb/redact v1.1.3/go.mod h1:BVNblN9mBWFyMyqK1k3AAiSxhvhfK2oOZZ2lK+dpvRg=
589589
github.com/cockroachdb/redact v1.1.6 h1:zXJBwDZ84xJNlHl1rMyCojqyIxv+7YUpQiJLQ7n4314=
590590
github.com/cockroachdb/redact v1.1.6/go.mod h1:BVNblN9mBWFyMyqK1k3AAiSxhvhfK2oOZZ2lK+dpvRg=

pkg/ccl/multiregionccl/multiregion_system_table_test.go

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -598,3 +598,46 @@ func TestMrSystemDatabaseUpgrade(t *testing.T) {
598598
{"ALTER PARTITION \"us-east3\" OF INDEX system.public.lease@primary CONFIGURE ZONE USING\n\tnum_voters = 3,\n\tvoter_constraints = '[+region=us-east3]',\n\tlease_preferences = '[[+region=us-east3]]'"},
599599
})
600600
}
601+
602+
// TestDropRegionFromUserDatabaseCleansUpSystemTables verifies that
603+
// dropping a region from a user database doesn't remove rows
604+
// from the system.sql_instances table.
605+
func TestDropRegionFromUserDatabaseCleansUpSystemTables(t *testing.T) {
606+
defer leaktest.AfterTest(t)()
607+
defer log.Scope(t).Close(t)
608+
609+
ctx := context.Background()
610+
611+
makeSettings := func() *cluster.Settings {
612+
cs := cluster.MakeTestingClusterSettings()
613+
instancestorage.ReclaimLoopInterval.Override(ctx, &cs.SV, 150*time.Millisecond)
614+
return cs
615+
}
616+
617+
_, systemSQL, cleanup := multiregionccltestutils.TestingCreateMultiRegionCluster(t, 3,
618+
base.TestingKnobs{},
619+
multiregionccltestutils.WithSettings(makeSettings()))
620+
defer cleanup()
621+
622+
sDB := sqlutils.MakeSQLRunner(systemSQL)
623+
624+
// Create a user database with multiple regions
625+
sDB.Exec(t, `CREATE DATABASE userdb PRIMARY REGION "us-east1" REGIONS "us-east2", "us-east3" SURVIVE ZONE FAILURE`)
626+
627+
// Verify sql_instances has data before the operation
628+
initialCount := sDB.QueryStr(t, `SELECT count(*) FROM system.sql_instances`)
629+
require.NotEmpty(t, initialCount)
630+
require.Equal(t, "13", initialCount[0][0], "sql_instances should have data initially")
631+
632+
// Drop a region from the USER database (not system database)
633+
sDB.Exec(t, `ALTER DATABASE userdb DROP REGION "us-east2"`)
634+
635+
// BUG: sql_instances table is now empty, but it shouldn't be
636+
// The region still exists in the system database, so instances should remain
637+
finalCount := sDB.QueryStr(t, `SELECT count(*) FROM system.sql_instances`)
638+
require.NotEmpty(t, finalCount)
639+
// This assertion will fail, demonstrating the bug.
640+
// The count should remain the same since we only dropped from userdb, not system
641+
require.Equal(t, initialCount[0][0], finalCount[0][0],
642+
"sql_instances count should not change when dropping region from user database")
643+
}

pkg/cmd/roachtest/tests/backup_fixtures.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -188,9 +188,9 @@ func (bd *backupDriver) prepareCluster(ctx context.Context) {
188188
// there is a snapshot backlog, which makes the snapshot backlog worse
189189
// because add sst causes ranges to fall behind and need recovery snapshots
190190
// to catch up.
191-
"kv.snapshot_rebalance.max_rate": "256 MiB",
192-
"server.debug.default_vmodule": "s3_storage=2",
193-
"cloudstorage.s3.enable_client_retry_token_bucket": "false",
191+
"kv.snapshot_rebalance.max_rate": "256 MiB",
192+
"server.debug.default_vmodule": "s3_storage=2",
193+
"cloudstorage.s3.client_retry_token_bucket.enabled": "false",
194194
},
195195
install.EnvOption{
196196
fmt.Sprintf("COCKROACH_AZURE_APPLICATION_CREDENTIALS_FILE=%s", azureCredentialsFilePath),

pkg/sql/type_change.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -407,7 +407,7 @@ func (t *typeSchemaChanger) exec(ctx context.Context) error {
407407
var idsToRemove []int
408408
populateIDsToRemove := func(holder context.Context, txn descs.Txn) error {
409409
typeDesc, err := txn.Descriptors().MutableByID(txn.KV()).Type(ctx, t.typeID)
410-
if err != nil {
410+
if err != nil || typeDesc.GetParentID() != keys.SystemDatabaseID {
411411
return err
412412
}
413413
for _, member := range typeDesc.EnumMembers {

0 commit comments

Comments
 (0)