From 9edbed950e574c3c0def3890a8be196b76ded66c Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Fri, 2 Feb 2018 15:52:12 -0500 Subject: [PATCH] sql: don't ignore job status update errors Fixes #21828. Fixes #21846. Fixes #21844. Fixes #21808. See discussion in #21802. We were previously ignoring the errors returned by job operations in a few places. This meant that we would fail to propagate retryable txn errors when using the jobs API and we could think a txn succeeded when it didn't. This change fixes that. In doing so, it makes schema changes fail when they see errors due to job status updates. I don't see a compelling reason why the old behavior was better and I suspect it was allowing subtle inconsistencies. Release note: None --- pkg/sql/backfill.go | 3 +-- pkg/sql/schema_changer.go | 3 +-- pkg/sql/table.go | 2 +- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/pkg/sql/backfill.go b/pkg/sql/backfill.go index 3a037ed6df2d..b097cbd6b814 100644 --- a/pkg/sql/backfill.go +++ b/pkg/sql/backfill.go @@ -577,8 +577,7 @@ func (sc *SchemaChanger) backfillIndexes( if details.ReadAsOf == (hlc.Timestamp{}) { details.ReadAsOf = txn.OrigTimestamp() if err := sc.job.WithTxn(txn).SetDetails(ctx, details); err != nil { - log.Warningf(ctx, "failed to store readAsOf on job %v after completing state machine: %v", - sc.job.ID(), err) + return errors.Wrapf(err, "failed to store readAsOf on job %d", *sc.job.ID()) } } sc.readAsOf = details.ReadAsOf diff --git a/pkg/sql/schema_changer.go b/pkg/sql/schema_changer.go index 36815a6b8167..b28e56ea80e9 100644 --- a/pkg/sql/schema_changer.go +++ b/pkg/sql/schema_changer.go @@ -626,8 +626,7 @@ func (sc *SchemaChanger) done(ctx context.Context, isRollback bool) (*sqlbase.De return nil }, func(txn *client.Txn) error { if err := sc.job.WithTxn(txn).Succeeded(ctx, jobs.NoopFn); err != nil { - log.Warningf(ctx, "schema change ignoring error while marking job %d as successful: %+v", - *sc.job.ID(), err) + return errors.Wrapf(err, "failed to mark job %d as as successful", *sc.job.ID()) } schemaChangeEventType := EventLogFinishSchemaChange diff --git a/pkg/sql/table.go b/pkg/sql/table.go index c3379e58f7de..0ae7b5cd059c 100644 --- a/pkg/sql/table.go +++ b/pkg/sql/table.go @@ -713,7 +713,7 @@ func (p *planner) createSchemaChangeJob( } job := p.ExecCfg().JobRegistry.NewJob(jobRecord) if err := job.WithTxn(p.txn).Created(ctx); err != nil { - return sqlbase.InvalidMutationID, nil + return sqlbase.InvalidMutationID, err } tableDesc.MutationJobs = append(tableDesc.MutationJobs, sqlbase.TableDescriptor_MutationJob{ MutationID: mutationID, JobID: *job.ID()})