sql: don't ignore job status update errors #22337
Conversation
|
This change is |
pkg/sql/table.go
Outdated
| @@ -650,7 +651,14 @@ func (p *planner) createSchemaChangeJob( | |||
| } | |||
| job := p.ExecCfg().JobRegistry.NewJob(jobRecord) | |||
| if err := job.WithTxn(p.txn).Created(ctx); err != nil { | |||
| return sqlbase.InvalidMutationID, nil | |||
| if pgErr, ok := pgerror.GetPGCause(err); ok { | |||
| // Ignore error if system.jobs table does not exist. This will be | |||
There was a problem hiding this comment.
I'm actually not sure about this. @mjibson were you returning nil before specifically to avoid this issue?
There was a problem hiding this comment.
Ooh, I have a much better solution for you, @nvanbenschoten. Gimme a sec.
There was a problem hiding this comment.
I don't remember ever dealing with this. If I'm in the blame output it's probably a result of a refactoring that happened.
There was a problem hiding this comment.
It's now impossible for system.jobs not to exist!
|
Also, here's a question. Suppose I have the following code: err := db.Txn(func (ctx context.Context, txn *client.Txn) error {
if err := txn.Put(ctx, "key", "val"); err != nil {
return err
}
// Don't really care if this succeeds.
_ := txn.Put(...)
return nil
})Is it possible that |
Fixes cockroachdb#21828. Fixes cockroachdb#21846. Fixes cockroachdb#21844. Fixes cockroachdb#21808. See discussion in cockroachdb#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
nvanbenschoten
force-pushed
the
nvanbenschoten/stopIgnoringErrors
branch
from
65622ff to
9edbed9
Compare
Yes, this currently is possible. See #22615. |
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