sql: public schema long running migration

[RichardJCai]

sql: public schema long running migration

Release note: None

[cockroach-teamcity]

This change is 

[postamar]

Thanks for doing this! I have a few comments, nothing major I don't think.

[postamar]

This looks good to me, good work! I'm hoping @ajwerner will also do a review pass, migrations are quite sensitive and are worth being careful about.

[RichardJCai]

Gonna add one more test using a backup.

[ajwerner]

I'm generally good on this. Just minor things. This is a classic example of something that'd be nice to exercise against real store dirs from the previous version.

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

---

pkg/migration/migrations/public_schema_migration.go, line 72 at r8 (raw file):

	for _, dbID := range databaseIDs {
		fmt.Println("dbID:", dbID)

detritus

---

pkg/migration/migrations/public_schema_migration.go, line 86 at r8 (raw file):

	return d.CollectionFactory.Txn(ctx, d.InternalExecutor, d.DB, func(
		ctx context.Context, txn *kv.Txn, descriptors *descs.Collection,
	) error {

nit: buy yourself an extra layer of indentation by making the body of this a function all on its own that maybe takes just the codec, settings, txn, collection, dbID? Generally good practice to scope down your deps when it's easy.

---

pkg/migration/migrations/public_schema_migration.go, line 129 at r8 (raw file):

		if desc.DatabaseDesc().Schemas == nil {
			dbDesc.Schemas = map[string]descpb.DatabaseDescriptor_SchemaInfo{

while you're here, thoughts on just adding a method to the dbdesc.Mutable which will construct the map if it doesn't exist and then add the entry? Seems bad that we have to do this.

---

pkg/migration/migrations/public_schema_migration.go, line 190 at r8 (raw file):

	}
	for _, modified := range modifiedDescs {
		err := descriptors.WriteDescToBatch(

I'm not sure how real of a concern this is, but it seems to me like we could end up with a large batch here. It's unlikely to be larger than the maximum size of 16 MiB because, we'll generally folks don't have hundreds of thousands of descriptors in their database, but, it's still a concern that would suck to run into. Thoughts on passing in the *Txn and then also having this return a batch, and then, when the batch reaches some size, send it and create a new one. You can test this by lowering the KV max command size to something quite small and then making some descriptors with some biggish strings. This is definitely an edge case concern.

[RichardJCai]

We got another small issue I think, restoring a database doesn't actually add an entry for the public schema in the namespace table.

See:
https://github.com/RichardJCai/cockroach/blob/backup_test_missing_public_schema_namespace_entry/pkg/ccl/backupccl/restore_old_version_public_schema_test.go

Notice theres no 52 0 public 29 entry.

Output:

0 0 system 1
1 29 descriptor 3
1 29 users 4
1 29 zones 5
1 29 settings 6
1 29 tenants 8
1 29 lease 11
1 29 eventlog 12
1 29 rangelog 13
1 29 ui 14
1 29 jobs 15
1 29 web_sessions 19
1 29 table_statistics 20
1 29 locations 21
1 29 role_members 23
1 29 comments 24
1 29 replication_constraint_stats 25
1 29 replication_critical_localities 26
1 29 replication_stats 27
1 29 reports_meta 28
51 0 public 29
50 0 public 29
1 0 public 29
1 29 namespace 30
1 29 protected_ts_meta 31
1 29 protected_ts_records 32
1 29 role_options 33
1 29 statement_bundle_chunks 34
1 29 statement_diagnostics_requests 35
1 29 statement_diagnostics 36
1 29 scheduled_jobs 37
1 29 sqlliveness 39
1 29 migrations 40
1 29 join_tokens 41
1 29 statement_statistics 42
1 29 transaction_statistics 43
1 29 database_role_settings 44
1 29 tenant_usage 45
1 29 sql_instances 46
1 29 span_configurations 47
0 0 defaultdb 50
0 0 postgres 51
0 0 d 52
52 0 s 53
52 53 t 54
52 29 t 55

cc @adityamaru

[ajwerner]

We got another small issue I think, restoring a database doesn't actually add an entry for the public schema in the namespace table.

Oof, bet that's going to lead to some odd bugs.

[RichardJCai]

We got another small issue I think, restoring a database doesn't actually add an entry for the public schema in the namespace table.

Oof, bet that's going to lead to some odd bugs.

I think we should make a migration for this as well?

[ajwerner]

We got another small issue I think, restoring a database doesn't actually add an entry for the public schema in the namespace table.

Oof, bet that's going to lead to some odd bugs.

I think we should make a migration for this as well?

Definitely.

[RichardJCai]

Created an issue for the missing system.namespace entry.
#73535

[RichardJCai]

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

---

pkg/migration/migrations/public_schema_migration.go, line 136 at r5 (raw file):

Previously, postamar (Marius Posta) wrote…

nit: because dbDesc is mutable, you can actually do dbDesc.Schemas = .... If you'd rather not access the embedded struct's field directly, dbDesc.DatabaseDescriptor().Schemas = ... is the way to go.

Done

---

pkg/migration/migrations/public_schema_migration.go, line 165 at r5 (raw file):

Previously, postamar (Marius Posta) wrote…

Yes this is what I meant, sorry for not being clear.

Done

---

pkg/migration/migrations/public_schema_migration.go, line 72 at r8 (raw file):

Previously, ajwerner wrote…

detritus

Done

---

pkg/migration/migrations/public_schema_migration.go, line 86 at r8 (raw file):

Previously, ajwerner wrote…

	return d.CollectionFactory.Txn(ctx, d.InternalExecutor, d.DB, func(
		ctx context.Context, txn *kv.Txn, descriptors *descs.Collection,
	) error {

nit: buy yourself an extra layer of indentation by making the body of this a function all on its own that maybe takes just the codec, settings, txn, collection, dbID? Generally good practice to scope down your deps when it's easy.

Done.

---

pkg/migration/migrations/public_schema_migration.go, line 129 at r8 (raw file):

Previously, ajwerner wrote…

		if desc.DatabaseDesc().Schemas == nil {
			dbDesc.Schemas = map[string]descpb.DatabaseDescriptor_SchemaInfo{

while you're here, thoughts on just adding a method to the dbdesc.Mutable which will construct the map if it doesn't exist and then add the entry? Seems bad that we have to do this.

Did this in a separate PR since we have other places where we do this
#73942

---

pkg/migration/migrations/public_schema_migration.go, line 190 at r8 (raw file):

Previously, ajwerner wrote…

I'm not sure how real of a concern this is, but it seems to me like we could end up with a large batch here. It's unlikely to be larger than the maximum size of 16 MiB because, we'll generally folks don't have hundreds of thousands of descriptors in their database, but, it's still a concern that would suck to run into. Thoughts on passing in the *Txn and then also having this return a batch, and then, when the batch reaches some size, send it and create a new one. You can test this by lowering the KV max command size to something quite small and then making some descriptors with some biggish strings. This is definitely an edge case concern.

Addressed all but this one so far, trying to figure out exactly how to do this.

[RichardJCai]

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

---

pkg/migration/migrations/public_schema_migration.go, line 190 at r8 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Addressed all but this one so far, trying to figure out exactly how to do this.

Added batching PTAL.

[postamar]

Just one minor comment. Looking forward to stamping this. Good work!

Reviewed 210 of 213 files at r5, 4 of 4 files at r6, 1 of 3 files at r9, 4 of 4 files at r10, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @RichardJCai)

---

pkg/migration/migrations/migrations.go, line 62 at r10 (raw file):

	),
	migration.NewTenantMigration(
		"seed system.span_configurations with configs for existing for existing tenants",

s/for existing for existing tenants/for existing tenants/

---

pkg/migration/migrations/public_schema_migration.go, line 101 at r10 (raw file):

	if !found {
		return errors.Newf("expected to find database with id %d", dbID)
	}

Piggybacking on Andrew's comment: you don't need to check found if you pass the Required flag to the descriptors API call above. Use tree.DatabaseLookupFlags{Required: true} and if it's not found, err will be set appropriately, meaning you can ignore found. This will also bypass leasing and all that, so it's what you want to do here.

---

pkg/migration/migrations/public_schema_migration.go, line 175 at r10 (raw file):

	batch := txn.NewBatch()
	for _, desc := range allDescriptors {
		b := desc.NewBuilder()

You'll want to move this further down, below the if-continue block which will eliminate most descriptors from further consideration. Under the hood, creating a descriptor builder does a protoutil.Clone, which isn't cheap. We've ran into perf issues from this kind of thing before, which is why I'm being careful.

[RichardJCai]

Updated, also made a minor change such that we don't run the test that creates 500 tables under race.

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

---

pkg/migration/migrations/migrations.go, line 62 at r10 (raw file):

Previously, postamar (Marius Posta) wrote…

s/for existing for existing tenants/for existing tenants/

Done.

---

pkg/migration/migrations/public_schema_migration.go, line 101 at r10 (raw file):

Previously, postamar (Marius Posta) wrote…

Piggybacking on Andrew's comment: you don't need to check found if you pass the Required flag to the descriptors API call above. Use tree.DatabaseLookupFlags{Required: true} and if it's not found, err will be set appropriately, meaning you can ignore found. This will also bypass leasing and all that, so it's what you want to do here.

Done.

---

pkg/migration/migrations/public_schema_migration.go, line 175 at r10 (raw file):

Previously, postamar (Marius Posta) wrote…

You'll want to move this further down, below the if-continue block which will eliminate most descriptors from further consideration. Under the hood, creating a descriptor builder does a protoutil.Clone, which isn't cheap. We've ran into perf issues from this kind of thing before, which is why I'm being careful.

Thanks good call

[postamar]

Reviewed 4 of 4 files at r11, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)

[RichardJCai]

Thanks for the reviews 😃

bors r=postamar

[craig]

Build succeeded:

• GitHub CI (Cockroach)