Merge #135994

135994: crosscluster/logical: only return jobID if planner txn succeeded r=kev-cao a=msbutler PR #134997 introduced a bug where CREATE LOGICAL REPLICATION could return early with a bogus job id if the txn which writes the job record retried. This patch prevents this. Note: I plan to open a follow up PR that moves the ldr planning txn into its own helper func. Fixes #135176 Fixes #135490 Release note: none Co-authored-by: Michael Butler <[email protected]>
cockroachdb · Nov 22, 2024 · cc71884 · cc71884
2 parents 1ae2d3d + 38db713
commit cc71884
Showing 1 changed file with 10 additions and 5 deletions.
diff --git a/pkg/ccl/crosscluster/logical/create_logical_replication_stmt.go b/pkg/ccl/crosscluster/logical/create_logical_replication_stmt.go
@@ -280,7 +280,9 @@ func createLogicalReplicationStreamPlanHook(
 		if cr, ok := options.GetDefaultFunction(); ok {
 			defaultConflictResolution = *cr
 		}
-		return p.ExecCfg().InternalDB.DescsTxn(ctx, func(ctx context.Context, txn descs.Txn) error {
+
+		var jobID jobspb.JobID
+		if err := p.ExecCfg().InternalDB.DescsTxn(ctx, func(ctx context.Context, txn descs.Txn) error {
 			dstTableDescs := make([]*tabledesc.Mutable, 0, len(srcTableDescs))
 			for _, pair := range repPairs {
 				dstTableDesc, err := txn.Descriptors().MutableByID(txn.KV()).Table(ctx, catid.DescID(pair.DstDescriptorID))
@@ -320,16 +322,19 @@ func createLogicalReplicationStreamPlanHook(
 				},
 				Progress: progress,
 			}
-
+			jobID = jr.JobID
 			if err := replicationutils.LockLDRTables(ctx, txn, dstTableDescs, jr.JobID); err != nil {
 				return err
 			}
 			if _, err := p.ExecCfg().JobRegistry.CreateAdoptableJobWithTxn(ctx, jr, jr.JobID, txn); err != nil {
 				return err
 			}
-			resultsCh <- tree.Datums{tree.NewDInt(tree.DInt(jr.JobID))}
-			return nil
-		})
+			return err
+		}); err != nil {
+			return err
+		}
+		resultsCh <- tree.Datums{tree.NewDInt(tree.DInt(jobID))}
+		return nil
 	}
 
 	return fn, streamCreationHeader, nil, false, nil