sql: insert missing public schema entry migration

[RichardJCai]

When restoring a database, a namespace entry for the public
schema was not created.

Release note: None

[cockroach-teamcity]

This change is 

[ajwerner]

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

---

pkg/clusterversion/cockroach_versions.go, line 524 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

@ajwerner not sure if this is okay, I put this ahead since we want the migration keyed on InsertPublicSchemaNamespaceEntryOnRestore to go before PublicSchemasWithDescriptors. Figured it might be okay since v22 is not released at all yet, however playing it safe and just rekeying PublicSchemasWithDescriptor after InsertPublicSchemaNamespaceEntryOnRestore without rearranging the other versions might be better

One option is to use this version for this migration and then just stick PublicSchemaWithDescriptors at the end.

[RichardJCai]

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

pkg/clusterversion/cockroach_versions.go, line 524 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…
One option is to use this version for this migration and then just stick PublicSchemaWithDescriptors at the end.

Yeah did this instead.

[RichardJCai]

Relevant issue: #73535
A separate fix has to be made for actually inserting the entry in restores.

[ajwerner]

I wonder if we can do is make it so that debug doctor will find this. It doesn't seem like a difficult addition and it'd give me some peace of mind.

Reviewed 16 of 21 files at r3, 10 of 10 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai and @shermanCRL)

---

pkg/ccl/backupccl/insert_missing_public_schema_namespace_entry_restore_test.go, line 28 at r5 (raw file):

)

func TestPublicSchemaMigration(t *testing.T) {

I'd like to see a lower level test which injects the missing entry.

---

pkg/ccl/backupccl/insert_missing_public_schema_namespace_entry_restore_test.go, line 80 at r5 (raw file):

	// Restore is bugged, the public schemas will not have entries in
	// system.namespace.

Did we not fix this bug to have the restore create the entry? Should we separately fix that bug and backport that fix?

---

pkg/ccl/backupccl/restore_old_versions_test.go, line 263 at r5 (raw file):

Quoted 4 lines of code…

		fmt.Println(publicSchemaDirs)
		require.NoError(t, err)
		for _, dir := range dirs {
			fmt.Println(dir.Name())

detritus

---

pkg/clusterversion/cockroach_versions.go, line 296 at r5 (raw file):

	// InsertPublicSchemaNamespaceEntryOnRestore ensures all public schemas
	// have an entry in system.namespace upon being restored.
	InsertPublicSchemaNamespaceEntryOnRestore

is there a TODO to use this in restore?

[RichardJCai]

I'll make a note to add this to debug doctor and actually create the entry during restore

#73535

This PR will be isolated to the migration itself

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

---

pkg/ccl/backupccl/insert_missing_public_schema_namespace_entry_restore_test.go, line 28 at r5 (raw file):

Previously, ajwerner wrote…

I'd like to see a lower level test which injects the missing entry.

Updated this test

---

pkg/ccl/backupccl/insert_missing_public_schema_namespace_entry_restore_test.go, line 80 at r5 (raw file):

Previously, ajwerner wrote…

	// Restore is bugged, the public schemas will not have entries in
	// system.namespace.

Did we not fix this bug to have the restore create the entry? Should we separately fix that bug and backport that fix?

Not yet - will do separately

---

pkg/clusterversion/cockroach_versions.go, line 296 at r5 (raw file):

Previously, ajwerner wrote…

is there a TODO to use this in restore?

Don't think we need a todo since we don't close this issue until we're done.

---

pkg/ccl/backupccl/restore_old_versions_test.go, line 263 at r5 (raw file):

Previously, ajwerner wrote…

		fmt.Println(publicSchemaDirs)
		require.NoError(t, err)
		for _, dir := range dirs {
			fmt.Println(dir.Name())

detritus

Done.

[RichardJCai]

Also I guess I don't need the backup files to be added in this Pr anymore, will remove after.

[ajwerner]

once you remove the hack from the test and the .DS_Store file.

Reviewed 4 of 12 files at r4, 5 of 5 files at r6, 14 of 14 files at r7, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @RichardJCai and @shermanCRL)

---

pkg/ccl/backupccl/insert_missing_public_schema_namespace_entry_restore_test.go, line 61 at r7 (raw file):

	// Give root the ability to delete from system.namespace.
	sqlDB.Exec(t, `INSERT INTO system.users VALUES ('node', '', false);`)
	sqlDB.Exec(t, `GRANT node TO root`)

I'm not in love with this. It doesn't bring me joy. Somebody is going to see this later and then use it to insert instead of delete, which is buggy. Can you use catalog and kv APIs to muck with these keys?

Code quote:

	var db1ID, db2ID int
	row := sqlDB.QueryRow(t, `SELECT id FROM system.namespace WHERE name = 'db1'`)
	row.Scan(&db1ID)
	row = sqlDB.QueryRow(t, `SELECT id FROM system.namespace WHERE name = 'db2'`)
	row.Scan(&db2ID)

	// Remove public schema namespace entries from system.namespace.
	sqlDB.Exec(t, fmt.Sprintf(`DELETE FROM system.namespace WHERE name='public' AND id=29 AND "parentID"=%d`, db1ID))
	sqlDB.Exec(t, fmt.Sprintf(`DELETE FROM system.namespace WHERE name='public' AND id=29 AND "parentID"=%d`, db2ID))

---

pkg/ccl/backupccl/testdata/restore_old_versions/.DS_Store, line 0 at r7 (raw file):
Added accidentally

[ajwerner]

Reviewed 1 of 9 files at r8.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @RichardJCai, and @shermanCRL)

[RichardJCai]

TFTR
bors r=ajwerner

[craig]

Already running a review

[RichardJCai]

Oops hit the wrong button

TFTR
bors r=ajwerner

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

This PR was included in a batch that was canceled, it will be automatically retried

[craig]

Build succeeded:

• GitHub CI (Cockroach)