Skip to content

Commit

Permalink
upgrades: ensure that create job_info synchronizes with jobs
Browse files Browse the repository at this point in the history
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
  • Loading branch information
knz committed Apr 3, 2023
1 parent 8e9273c commit 858d874
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 6 deletions.
28 changes: 25 additions & 3 deletions pkg/upgrade/upgrades/system_job_info.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 = `
Expand Down
25 changes: 22 additions & 3 deletions pkg/upgrade/upgrades/system_job_info_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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())
}

0 comments on commit 858d874

Please sign in to comment.