Merge cockroachdb#100523

100523: upgrades: ensure that create job_info synchronizes with jobs r=adityamaru a=knz

Probably needed for cockroachdb#99087 and cockroachdb#99836.

Prior to this patch, the migration that introduced the new job_info
table did not touch any other descriptor. As such, it couldn't
synchronize properly with any concurrent code that needed to access
the jobs table.

This patch fixes it by forcing a version upgrade on the jobs table
descriptor. This triggers a lease synchronization in the upgrade
manager and results in the proper ordering.

Release note: None
Epic: None

Co-authored-by: Raphael 'kena' Poss <[email protected]>

pkg/upgrade/upgrades/system_job_info.go

@@ -14,18 +14,40 @@ import (
 	"context"
 
 	"github.com/cockroachdb/cockroach/pkg/clusterversion"
+	"github.com/cockroachdb/cockroach/pkg/keys"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/upgrade"
 )
 
-// systemPrivilegesTableMigration creates the system.privileges table.
+// systemPrivilegesTableMigration creates the system.job_info table.
 func systemJobInfoTableMigration(
 	ctx context.Context, _ clusterversion.ClusterVersion, d upgrade.TenantDeps,
 ) error {
-	return createSystemTable(
+	// Create the job_info table proper.
+	if err := createSystemTable(
 		ctx, d.DB.KV(), d.Settings, d.Codec, systemschema.SystemJobInfoTable,
-	)
+	); err != nil {
+		return err
+	}
+
+	// Also bump the version of the descriptor for system.jobs. This
+	// ensures that this migration synchronizes with all other
+	// migrations that touch jobs, and concurrent transactions that
+	// perform DDL operations. We need this because just creating a new
+	// table above does not synchronize with other accesses to
+	// system.jobs on its own.
+	return d.DB.DescsTxn(ctx, func(
+		ctx context.Context, txn descs.Txn,
+	) (err error) {
+		collection := txn.Descriptors()
+		t, err := collection.MutableByID(txn.KV()).Desc(ctx, keys.JobsTableID)
+		if err != nil {
+			return err
+		}
+		return collection.WriteDesc(ctx, true /* kvTracing */, t, txn.KV())
+	})
 }
 
 const alterPayloadToNullableQuery = `

pkg/upgrade/upgrades/system_job_info_test.go

@@ -16,12 +16,15 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/clusterversion"
+	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/server"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
 	"github.com/cockroachdb/cockroach/pkg/upgrade/upgrades"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+	"github.com/stretchr/testify/require"
 )
 
 func TestSystemJobInfoMigration(t *testing.T) {
@@ -51,14 +54,30 @@ func TestSystemJobInfoMigration(t *testing.T) {
 	db := tc.ServerConn(0)
 	defer db.Close()
 
-	// NB: this isn't actually doing anything, since the table is baked into the
-	// bootstrap schema, so this is really just showing the upgrade is idempotent,
-	// but this is in line with the other tests of createSystemTable upgrades.
+	// Advance to the version immediately before the one that
+	// interests us.
+	upgrades.Upgrade(
+		t,
+		db,
+		clusterversion.V23_1CreateSystemJobInfoTable-1,
+		nil,
+		false,
+	)
+
+	// We verify that the jobs table gets its version upgraded through
+	// the upgrade, to ensure the creation of job_info synchronizes with
+	// concurrent accesses to the jobs table.
+	kvDB := tc.Server(0).(*server.TestServer).DB()
+	tblBefore := desctestutils.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "system", "public", "jobs")
+
 	upgrades.Upgrade(
 		t,
 		db,
 		clusterversion.V23_1CreateSystemJobInfoTable,
 		nil,
 		false,
 	)
+
+	tblAfter := desctestutils.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "system", "public", "jobs")
+	require.Greater(t, tblAfter.GetVersion(), tblBefore.GetVersion())
 }