Merge #109630 #110069

109630: backupccl: Refactor allocateDescriptorRewrites in restore_planning.go r=benbardin a=benbardin

backupccl: Refactor allocateDescriptorRewrites in restore_planning.go, for easier reasoning and more comprehensive unit-testability.

Most of this code ought to be a no-op from the user perspective. There are, however, two changes:
1. Allocation of new DB IDs has been moved later, to make the algorithm clearer. Previously they were assigned earlier in the process, in a for-loop that was a bit surprising.
2. Descriptor objects for databases are no longer updated with SetName() prior to allocation. These updates were not persisted to the database, and so not passed to the processor. That meant we had one method for tracking such an update during planning, and another during processing. Now we use the same method - NewDBName on a rewrite object - for both.

Note that this PR does not necessarily complete the unit-testing work here. Some of the new `remap*()` functions are themselves quite long, and can likely be broken up cogently; and all of them could use their own independent unit testing. Nonetheless this PR marks a good checkpoint.

Release note: None
Part of: https://cockroachlabs.atlassian.net/browse/CRDB-31024

110069: base: make TestValidateAddrs work without a network connection r=nvanbenschoten a=arulajmani

Looking up an invalid host name may result in a different error depending on whether the test has access to a network connection or not. This patch modifies the test to catch that.

Fixes: #109052

Release note: None

Co-authored-by: Ben Bardin <[email protected]>
Co-authored-by: Arul Ajmani <[email protected]>

Author: 3 people

pkg/base/addr_validation_test.go

@@ -64,12 +64,17 @@ func TestValidateAddrs(t *testing.T) {
 		t.Fatal("expected port resolution failure, got no error")
 	}
 	portExpectedErr := err.Error()
-	// For the host name resolution error we can reliably expect "no such host"
-	// below, but before we test anything we need to ensure we indeed have
-	// a reliably non-resolvable host name.
-	_, err = net.DefaultResolver.LookupIPAddr(context.Background(), "nonexistent.example.com")
-	if err == nil {
-		t.Fatal("expected host resolution failure, got no error")
+	// ValidateAddrs uses a net.DefaultResolver for host name resolution.
+	// Normally, when given a non-existent host name, we expect it to return a
+	// "no such host" error. However, it may return a different error if there is
+	// no network access. See
+	// https://github.com/cockroachdb/cockroach/issues/109052.
+	noSuchHostErr := func(host string) string {
+		_, err := net.DefaultResolver.LookupIPAddr(context.Background(), host)
+		if err == nil { // the supplied host is expected to be reliably non-resolvable
+			t.Fatal("expected host resolution failure, got no error")
+		}
+		return err.Error()
 	}
 
 	// The test cases.
@@ -149,8 +154,8 @@ func TestValidateAddrs(t *testing.T) {
 		{addrs{"localhost:-1231", "", "", "", "", ""}, "invalid port", addrs{}},
 		{addrs{"localhost:nonexistent", "", "", "", "", ""}, portExpectedErr, addrs{}},
 		// Invalid address.
-		{addrs{"nonexistent.example.com:26257", "", "", "", "", ""}, "no such host", addrs{}},
-		{addrs{"333.333.333.333:26257", "", "", "", "", ""}, "no such host", addrs{}},
+		{addrs{"nonexistent.example.com:26257", "", "", "", "", ""}, noSuchHostErr("nonexistent.example.com"), addrs{}},
+		{addrs{"333.333.333.333:26257", "", "", "", "", ""}, noSuchHostErr("333.333.333.333"), addrs{}},
 	}
 
 	for i, test := range testData {

pkg/ccl/backupccl/BUILD.bazel

@@ -157,6 +157,7 @@ go_library(
         "@com_github_gogo_protobuf//types",
         "@com_github_kr_pretty//:pretty",
         "@com_github_robfig_cron_v3//:cron",
+        "@org_golang_x_exp//maps",
     ],
 )
 
@@ -265,12 +266,16 @@ go_test(
         "//pkg/sql/catalog/bootstrap",
         "//pkg/sql/catalog/catalogkeys",
         "//pkg/sql/catalog/catenumpb",
+        "//pkg/sql/catalog/dbdesc",
         "//pkg/sql/catalog/descbuilder",
         "//pkg/sql/catalog/descpb",
         "//pkg/sql/catalog/descs",
         "//pkg/sql/catalog/desctestutils",
+        "//pkg/sql/catalog/funcdesc",
+        "//pkg/sql/catalog/schemadesc",
         "//pkg/sql/catalog/systemschema",
         "//pkg/sql/catalog/tabledesc",
+        "//pkg/sql/catalog/typedesc",
         "//pkg/sql/execinfra",
         "//pkg/sql/execinfrapb",
         "//pkg/sql/importer",
@@ -339,6 +344,7 @@ go_test(
         "@com_github_robfig_cron_v3//:cron",
         "@com_github_stretchr_testify//assert",
         "@com_github_stretchr_testify//require",
+        "@org_golang_x_exp//slices",
         "@org_golang_x_sync//errgroup",
     ],
 )

pkg/ccl/backupccl/backup_test.go

@@ -10837,7 +10837,7 @@ $$;
 	err = sql.TestingDescsTxn(ctx, tgtServer, func(ctx context.Context, txn isql.Txn, col *descs.Collection) error {
 		dbDesc, err := col.ByNameWithLeased(txn.KV()).Get().Database(ctx, "db1")
 		require.NoError(t, err)
-		require.Equal(t, 107, int(dbDesc.GetID()))
+		require.Equal(t, 123, int(dbDesc.GetID()))
 
 		scDesc, err := col.ByNameWithLeased(txn.KV()).Get().Schema(ctx, dbDesc, "sc1")
 		require.NoError(t, err)
@@ -10861,7 +10861,7 @@ $$;
 		fnDesc, err := col.ByIDWithLeased(txn.KV()).WithoutNonPublic().Get().Function(ctx, descpb.ID(udfID))
 		require.NoError(t, err)
 		require.Equal(t, 130, int(fnDesc.GetID()))
-		require.Equal(t, 107, int(fnDesc.GetParentID()))
+		require.Equal(t, 123, int(fnDesc.GetParentID()))
 		require.Equal(t, 125, int(fnDesc.GetParentSchemaID()))
 		// Make sure db name and IDs are rewritten in function body.
 		require.Equal(t, "SELECT a FROM db1.sc1.tbl1;\nSELECT nextval(129:::REGCLASS);", fnDesc.GetFunctionBody())