sql: fix temporary schema cleaner to use transaction correctly #86301
Conversation
|
This change is |
ajwerner
force-pushed
the
ajwerner/fix-temp-schema
branch
from
f1b9f29
to
5685f05
Compare
Release justification: Does not change production code, only comments. Release note: None
This error was never what anybody wanted. It has the odd side-effect of leaving the transaction usable -- and, worse, the (*kv.Txn).exec loop would retry but would not restart or replace the transaction if such an error were returned. Release justification: important correctness change as part of making the SQL-over-HTTP APIs work for index recommendations. Release note: None
This thing before was buggy in some way I don't really understand. Release justification: Fixes tests and improves code. Release note: None
It now can now share all the pre-commit logic from the SQL package. Release justification: non-functional change which improves code Release note: None
We need to track the jobs records and we need to run the jobs. Release note: None
We need to construct the internal executor in the context of the transaction so that we can make sure that its side-effects are properly managed. Without this change, we'd be throwing away all of the extraTxnState between each statement. We'd fail to create the jobs (which we defer to the end of the transaction), and we'd fail to run those jobs and check for errors. We'd also fail to validate the two-version invariant or wait for one version. Fixes cockroachdb#86332 Release justification: Fixes critical bugs in new functionality. Release note: None
ajwerner
force-pushed
the
ajwerner/fix-temp-schema
branch
from
5685f05
to
3119b10
Compare
| @@ -147,6 +147,7 @@ func newInternalExecutorWithTxn( | |||
| extraTxnState: &extraTxnState{ | |||
| txn: txn, | |||
| descCollection: descCol, | |||
| jobs: new(jobsCollection), | |||
| schemaChangeJobRecords: schemaChangeJobRecords, | |||
There was a problem hiding this comment.
This is a confusing part that I introduced 😓 We should have had schemaChangeJobRecords: make(map[descpb.ID]*jobs.Record) here and remove it from the parameter list. We should also have added a comment that except descriptor collection, all metadata (such as jobs) is this internal executor's own set, thus their lifecycle is managed by the internal executor itself, and independent of the caller.
There was a problem hiding this comment.
The truth of the matter is that now that we're here, it seems like the InternalExecutorFactory should also be constructing the descs.Collection and should own its lifecycle. There's no reason for descs.CollectionFactory.Txn anymore. Honestly, that's sort of beautiful. We'll leave that for later. I moved the map construction for now.
| return err | ||
| } | ||
|
|
||
| if err := descsCol.ValidateUncommittedDescriptors(ctx, txn); err != nil { |
There was a problem hiding this comment.
Not a blocker, but why are some of the checks here no longer needed?
|
superseded by #86427 |
This thing before was buggy in some way I don't really understand.
Release justification: Fixes tests and improves code.
Release note: None