sql: insert missing public schema namespace entry

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

Release note: None

pkg/ccl/backupccl/BUILD.bazel

@@ -141,6 +141,7 @@ go_test(
         "full_cluster_backup_restore_test.go",
         "helpers_test.go",
         "import_spans_test.go",
+        "insert_missing_public_schema_namespace_entry_restore_test.go",
         "key_rewriter_test.go",
         "main_test.go",
         "partitioned_backup_test.go",

pkg/ccl/backupccl/insert_missing_public_schema_namespace_entry_restore_test.go

@@ -0,0 +1,92 @@
+// Copyright 2021 The Cockroach Authors.
+//
+// Licensed as a CockroachDB Enterprise file under the Cockroach Community
+// License (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+//     https://github.com/cockroachdb/cockroach/blob/master/licenses/CCL.txt
+
+package backupccl_test
+
+import (
+	"context"
+	"fmt"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
+	"github.com/cockroachdb/cockroach/pkg/server"
+	"github.com/cockroachdb/cockroach/pkg/testutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
+	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+	"github.com/stretchr/testify/require"
+)
+
+func TestPublicSchemaMigration(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	ctx := context.Background()
+	dir, cleanup := testutils.TempDir(t)
+	defer cleanup()
+	tc := testcluster.StartTestCluster(t, 3, base.TestClusterArgs{
+		ServerArgs: base.TestServerArgs{
+			ExternalIODir: dir,
+			Knobs: base.TestingKnobs{
+				Server: &server.TestingKnobs{
+					DisableAutomaticVersionUpgrade: 1,
+					BinaryVersionOverride:          clusterversion.ByKey(clusterversion.InsertPublicSchemaNamespaceEntryOnRestore - 1),
+				},
+			},
+		},
+	})
+	defer tc.Stopper().Stop(ctx)
+
+	db := tc.ServerConn(0)
+	defer db.Close()
+	sqlDB := sqlutils.MakeSQLRunner(tc.Conns[0])
+
+	/*
+		This backup was created by executing the following commands on a v21.1.1
+		cluster and performing a full cluster backup.
+			CREATE DATABASE db1;
+			CREATE TABLE db1.t();
+			CREATE SCHEMA db1.s;
+			CREATE DATABASE db2;
+			CREATE TABLE db2.t(x INT);
+			INSERT INTO db2.t VALUES (1), (2);
+			CREATE SCHEMA db2.s;
+			CREATE TABLE db2.s.t(x INT);
+			INSERT INTO db2.s.t VALUES (1), (2);
+	*/
+	publicSchemaDir, err := filepath.Abs("./testdata/restore_old_versions/missing-public-schema-namespace/v21.1.1")
+	require.NoError(t, err)
+	err = os.Symlink(publicSchemaDir, filepath.Join(dir, "foo"))
+	require.NoError(t, err)
+
+	localFoo := "nodelocal://0/foo"
+
+	_ = sqlDB.Exec(t, fmt.Sprintf("RESTORE DATABASE db1 FROM '%s'", localFoo))
+	_ = sqlDB.Exec(t, fmt.Sprintf("RESTORE DATABASE db2 FROM '%s'", localFoo))
+
+	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)
+
+	// Restore is bugged, the public schemas will not have entries in
+	// system.namespace.
+	sqlDB.CheckQueryResults(t, fmt.Sprintf(`SELECT id FROM system.namespace WHERE name = 'public' AND "parentID"=%d`, db1ID), [][]string{})
+	sqlDB.CheckQueryResults(t, fmt.Sprintf(`SELECT id FROM system.namespace WHERE name = 'public' AND "parentID"=%d`, db2ID), [][]string{})
+
+	// Migrate to the new version.
+	_ = sqlDB.Exec(t, `SET CLUSTER SETTING version = $1`,
+		clusterversion.ByKey(clusterversion.InsertPublicSchemaNamespaceEntryOnRestore).String())
+	require.NoError(t, err)
+
+	sqlDB.CheckQueryResults(t, fmt.Sprintf(`SELECT id FROM system.namespace WHERE name = 'public' AND "parentID"=%d`, db1ID), [][]string{{"29"}})
+	sqlDB.CheckQueryResults(t, fmt.Sprintf(`SELECT id FROM system.namespace WHERE name = 'public' AND "parentID"=%d`, db2ID), [][]string{{"29"}})
+
+}

pkg/ccl/backupccl/restore_old_versions_test.go

@@ -257,8 +257,10 @@ ORDER BY object_type, object_name`, [][]string{
 
 	t.Run("public_schema_mixed_version", func(t *testing.T) {
 		dirs, err := ioutil.ReadDir(publicSchemaDirs)
+		fmt.Println(publicSchemaDirs)
 		require.NoError(t, err)
 		for _, dir := range dirs {
+			fmt.Println(dir.Name())
 			require.True(t, dir.IsDir())
 			exportDir, err := filepath.Abs(filepath.Join(publicSchemaDirs, dir.Name()))
 			require.NoError(t, err)

pkg/ccl/backupccl/testdata/restore_old_versions/missing-public-schema-namespace/v21.1.1/BACKUP-CHECKPOINT-716846479146844161-CHECKSUM

@@ -0,0 +1 @@
+����

pkg/ccl/backupccl/testdata/restore_old_versions/missing-public-schema-namespace/v21.1.1/BACKUP-CHECKPOINT-CHECKSUM

@@ -0,0 +1 @@
+��3

pkg/ccl/backupccl/testdata/restore_old_versions/missing-public-schema-namespace/v21.1.1/BACKUP_MANIFEST-CHECKSUM

@@ -0,0 +1 @@
+O��,

pkg/clusterversion/cockroach_versions.go

@@ -291,8 +291,9 @@ const (
 	// MVCCAddSSTable supports MVCC-compliant AddSSTable requests via the new
 	// WriteAtRequestTimestamp and DisallowConflicts parameters.
 	MVCCAddSSTable
-	// Public schema is backed by a descriptor.
-	PublicSchemasWithDescriptors
+	// InsertPublicSchemaNamespaceEntryOnRestore ensures all public schemas
+	// have an entry in system.namespace upon being restored.
+	InsertPublicSchemaNamespaceEntryOnRestore
 	// UnsplitRangesInAsyncGCJobs moves ranges unsplitting from transaction of
 	// "drop table"/"truncate table" to async gc jobs
 	UnsplitRangesInAsyncGCJobs
@@ -316,6 +317,8 @@ const (
 	// protos (the client is responsible for filling it in explicitly), and the
 	// server-side handler is responsible for opening a span manually.
 	SelectRPCsTakeTracingInfoInband
+	// Public schema is backed by a descriptor.
+	PublicSchemasWithDescriptors
 
 	// *************************************************
 	// Step (1): Add new versions here.
@@ -530,7 +533,7 @@ var versionsSingleton = keyedVersions{
 		Version: roachpb.Version{Major: 21, Minor: 2, Internal: 16},
 	},
 	{
-		Key:     PublicSchemasWithDescriptors,
+		Key:     InsertPublicSchemaNamespaceEntryOnRestore,
 		Version: roachpb.Version{Major: 21, Minor: 2, Internal: 18},
 	},
 	{
@@ -553,6 +556,10 @@ var versionsSingleton = keyedVersions{
 		Key:     SelectRPCsTakeTracingInfoInband,
 		Version: roachpb.Version{Major: 21, Minor: 2, Internal: 28},
 	},
+	{
+		Key:     PublicSchemasWithDescriptors,
+		Version: roachpb.Version{Major: 21, Minor: 2, Internal: 30},
+	},
 
 	// *************************************************
 	// Step (2): Add new versions here.

pkg/migration/migrations/BUILD.bazel

@@ -10,6 +10,7 @@ go_library(
         "delete_deprecated_namespace_tabledesc.go",
         "ensure_no_draining_names.go",
         "fix_descriptor_migration.go",
+        "insert_missing_public_schema_namespace_entry.go",
         "join_tokens.go",
         "migrations.go",
         "records_based_registry.go",

pkg/migration/migrations/insert_missing_public_schema_namespace_entry.go

@@ -0,0 +1,82 @@
+// Copyright 2021 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package migrations
+
+import (
+	"context"
+
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
+	"github.com/cockroachdb/cockroach/pkg/jobs"
+	"github.com/cockroachdb/cockroach/pkg/keys"
+	"github.com/cockroachdb/cockroach/pkg/kv"
+	"github.com/cockroachdb/cockroach/pkg/migration"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+)
+
+// insertMissingPublicSchemaNamespaceEntry creates a system.namespace entries
+// for public schemas that are missing a system.namespace entry.
+// This arises from restore where we mistakenly did not create system.namespace
+// entries for public schemas when restoring databases.
+func insertMissingPublicSchemaNamespaceEntry(
+	ctx context.Context, _ clusterversion.ClusterVersion, d migration.TenantDeps, _ *jobs.Job,
+) error {
+	// Get the ID of all databases where we're missing a public schema namespace
+	// entry for.
+	query := `
+  SELECT id
+    FROM system.namespace
+   WHERE id
+         NOT IN (
+                SELECT ns_db.id
+                  FROM system.namespace AS ns_db
+                       INNER JOIN system.namespace
+                            AS ns_sc ON (
+                                        ns_db.id
+                                        = ns_sc."parentID"
+                                    )
+                 WHERE ns_db."parentSchemaID" = 0
+                   AND ns_db."parentID" = 0
+                   AND ns_sc."parentSchemaID" = 0
+                   AND ns_sc.name = 'public'
+                   AND ns_sc.id = 29
+            )
+     AND "parentID" = 0
+ORDER BY id ASC;
+`
+	rows, err := d.InternalExecutor.QueryIterator(
+		ctx, "get_databases_without_public_schema_namespace_entry", nil /* txn */, query,
+	)
+	if err != nil {
+		return err
+	}
+	var databaseIDs []descpb.ID
+	for ok, err := rows.Next(ctx); ok; ok, err = rows.Next(ctx) {
+		if err != nil {
+			return err
+		}
+		id := descpb.ID(tree.MustBeDInt(rows.Cur()[0]))
+		databaseIDs = append(databaseIDs, id)
+	}
+
+	return d.CollectionFactory.Txn(ctx, d.InternalExecutor, d.DB, func(
+		ctx context.Context, txn *kv.Txn, descriptors *descs.Collection,
+	) error {
+		b := txn.NewBatch()
+		for _, dbID := range databaseIDs {
+			publicSchemaKey := catalogkeys.MakeSchemaNameKey(d.Codec, dbID, tree.PublicSchema)
+			b.Put(publicSchemaKey, keys.PublicSchemaID)
+		}
+		return txn.Run(ctx, b)
+	})
+}

pkg/migration/migrations/migrations.go

@@ -142,6 +142,12 @@ var migrations = []migration.Migration{
 		NoPrecondition,
 		alterSystemStmtDiagReqs,
 	),
+	migration.NewTenantMigration(
+		"insert missing system.namespace entries for public schemas",
+		toCV(clusterversion.InsertPublicSchemaNamespaceEntryOnRestore),
+		NoPrecondition,
+		insertMissingPublicSchemaNamespaceEntry,
+	),
 }
 
 func init() {