Merge cockroachdb#62379

62379: importccl,backupccl: create job record in planner txn r=pbardea a=pbardea This commit updates the way that we create the job record in import and applies the same logic to backup and restore. Instead of committing the planner transaction before creating the job record in a separate transaction, the job record creation now _uses_ the planner transaction. The planner transaction is then committed after creating the job record. This should not result in any change in functionality. The change was intended for increased readability: since the main goal of the plan hook is to create a job record, it makes sense that it is done in the planner's transaction. The same logic is applied to planning backup and restore jobs. Release note: None Co-authored-by: Paul Bardea <[email protected]>
andreimatei · Apr 2, 2021 · df928e4 · df928e4
2 parents eef29d0 + 5a906b4
commit df928e4
Show file tree

Hide file tree

Showing 3 changed files with 46 additions and 47 deletions.
diff --git a/pkg/ccl/backupccl/backup_planning.go b/pkg/ccl/backupccl/backup_planning.go
@@ -47,7 +47,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/storage/cloudimpl"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/interval"
-	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/cockroach/pkg/util/tracing"
@@ -1268,24 +1267,27 @@ func backupPlanHook(
 			return nil
 		}
 
+		// We create the job record in the planner's transaction to ensure that
+		// the job record creation happens transactionally.
+		plannerTxn := p.ExtendedEvalContext().Txn
+
 		var sj *jobs.StartableJob
 		jobID := p.ExecCfg().JobRegistry.MakeJobID()
-		if err := p.ExecCfg().DB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) (err error) {
-			if err := p.ExecCfg().JobRegistry.CreateStartableJobWithTxn(ctx, &sj, jobID, txn, jr); err != nil {
-				return err
-			}
-			if err := doWriteBackupManifestCheckpoint(ctx, jobID); err != nil {
-				return err
-			}
+		if err := p.ExecCfg().JobRegistry.CreateStartableJobWithTxn(ctx, &sj, jobID, plannerTxn, jr); err != nil {
+			return err
+		}
+		if err := doWriteBackupManifestCheckpoint(ctx, jobID); err != nil {
+			return err
+		}
+		if err := protectTimestampForBackup(ctx, p, plannerTxn, jobID, spans, startTime, endTime, backupDetails); err != nil {
+			return err
+		}
 
-			return protectTimestampForBackup(ctx, p, txn, jobID, spans, startTime, endTime,
-				backupDetails)
-		}); err != nil {
-			if sj != nil {
-				if cleanupErr := sj.CleanupOnRollback(ctx); cleanupErr != nil {
-					log.Warningf(ctx, "failed to cleanup StartableJob: %v", cleanupErr)
-				}
-			}
+		// We commit the transaction here so that the job can be started. This
+		// is safe because we're in an implicit transaction. If we were in an
+		// explicit transaction the job would have to be run with the detached
+		// option and would have been handled above.
+		if err := plannerTxn.Commit(ctx); err != nil {
 			return err
 		}
 

diff --git a/pkg/ccl/backupccl/restore_planning.go b/pkg/ccl/backupccl/restore_planning.go
@@ -1887,16 +1887,21 @@ func doRestorePlan(
 		return nil
 	}
 
+	// We create the job record in the planner's transaction to ensure that
+	// the job record creation happens transactionally.
+	plannerTxn := p.ExtendedEvalContext().Txn
+
 	var sj *jobs.StartableJob
 	jobID := p.ExecCfg().JobRegistry.MakeJobID()
-	if err := p.ExecCfg().DB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) (err error) {
-		return p.ExecCfg().JobRegistry.CreateStartableJobWithTxn(ctx, &sj, jobID, txn, jr)
-	}); err != nil {
-		if sj != nil {
-			if cleanupErr := sj.CleanupOnRollback(ctx); cleanupErr != nil {
-				log.Warningf(ctx, "failed to cleanup StartableJob: %v", cleanupErr)
-			}
-		}
+	if err := p.ExecCfg().JobRegistry.CreateStartableJobWithTxn(ctx, &sj, jobID, plannerTxn, jr); err != nil {
+		return err
+	}
+
+	// We commit the transaction here so that the job can be started. This is
+	// safe because we're in an implicit transaction. If we were in an explicit
+	// transaction the job would have to be created with the detached option and
+	// would have been handled above.
+	if err := plannerTxn.Commit(ctx); err != nil {
 		return err
 	}
 

diff --git a/pkg/ccl/importccl/import_stmt.go b/pkg/ccl/importccl/import_stmt.go
@@ -965,33 +965,25 @@ func importPlanHook(
 			return nil
 		}
 
-		// We're about to have side-effects not tied to this tranaction
-		// (creating the job record below). This is okay as long as the
-		// transaction eventually commits. To ensure that the transaction
-		// commits, we commit it here. This is allowed since we know we're in an
-		// implicit transaction.
-		// We know we're in an implicit transaction because we would have
-		// already returned if it were a detached job, and we check at the start
-		// of the plan hooks that the transaction is implicit if the detached
-		// option was not specified.
-		if err := p.ExtendedEvalContext().Txn.Commit(ctx); err != nil {
-			return err
-		}
+		// We create the job record in the planner's transaction to ensure that
+		// the job record creation happens transactionally.
+		plannerTxn := p.ExtendedEvalContext().Txn
 
 		var sj *jobs.StartableJob
 		jobID := p.ExecCfg().JobRegistry.MakeJobID()
-		if err := p.ExecCfg().DB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) (err error) {
-			if err := p.ExecCfg().JobRegistry.CreateStartableJobWithTxn(ctx, &sj, jobID, txn, jr); err != nil {
-				return err
-			}
+		if err := p.ExecCfg().JobRegistry.CreateStartableJobWithTxn(ctx, &sj, jobID, plannerTxn, jr); err != nil {
+			return err
+		}
 
-			return protectTimestampForImport(ctx, p, txn, jobID, spansToProtect, walltime, importDetails)
-		}); err != nil {
-			if sj != nil {
-				if cleanupErr := sj.CleanupOnRollback(ctx); cleanupErr != nil {
-					log.Warningf(ctx, "failed to cleanup StartableJob: %v", cleanupErr)
-				}
-			}
+		if err := protectTimestampForImport(ctx, p, plannerTxn, jobID, spansToProtect, walltime, importDetails); err != nil {
+			return err
+		}
+
+		// We commit the transaction here so that the job can be started. This
+		// is safe because we're in an implicit transaction. If we were in an
+		// explicit transaction the job would have to be run with the detached
+		// option and would have been handled above.
+		if err := plannerTxn.Commit(ctx); err != nil {
 			return err
 		}