sql: disallow schema changes for READ COMMITTED transactions #107216
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 3 of 10 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten and @rafiss)
pkg/sql/opt/exec/execbuilder/relational.go line 171 at r1 (raw file):
if opt.IsDDLOp(e) { if b.evalCtx.Txn.IsoLevel().ToleratesWriteSkew() {
I think there are a few cases where we have a nil Txn in execbuilder, so for this reason I've been using b.evalCtx.TxnIsoLevel instead of b.evalCtx.Txn.IsoLevel().
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/exec/execbuilder/relational.go line 171 at r1 (raw file):
Previously, michae2 (Michael Erickson) wrote…
I think there are a few cases where we have a nil
Txnin execbuilder, so for this reason I've been usingb.evalCtx.TxnIsoLevelinstead ofb.evalCtx.Txn.IsoLevel().
ah ok good to know. i was wondering why that was there. but if Txn is nil, wouldn't it be likely that TxnIsoLevel is the zero-value (which is serializable)?
There was a problem hiding this comment.
Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status:
pkg/sql/table.go line 111 at r1 (raw file):
ctx context.Context, tableDesc *tabledesc.Mutable, jobDesc string, mutationID descpb.MutationID, ) error { if p.Txn().IsoLevel().ToleratesWriteSkew() {
Why is this check needed if we already have the execbuilder-level check? For statements that bypass the optimizer?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/table.go line 111 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Why is this check needed if we already have the execbuilder-level check? For statements that bypass the optimizer?
yeah that was the idea. some statements, like CREATE ROLE, cause a schema change (i.e., update the object descriptor), but I wasn't sure how to make the optimizer classify them as DDL. i've tracked that down now, so we no longer need this logic. thanks for the nudge on this.
(it required changing the StatementReturnType() of the statement, which should be safe to do for those operations. i also audited the other statements, and nothing else seemed off.)
rafiss
force-pushed
the
read-committed-schema-changes
branch
from
0c3ffd8
to
6f535ca
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status:
pkg/sql/opt/exec/execbuilder/relational.go line 171 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
ah ok good to know. i was wondering why that was there. but if
Txnis nil, wouldn't it be likely thatTxnIsoLevelis the zero-value (which is serializable)?
I forget exactly why I did this. Maybe I was just seeing ghosts (the ghosts being the other Txn* fields in eval.Context). Let's see if any tests can tell us why: #107363
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/exec/execbuilder/relational.go line 171 at r1 (raw file):
Previously, michae2 (Michael Erickson) wrote…
I forget exactly why I did this. Maybe I was just seeing ghosts (the ghosts being the other
Txn*fields ineval.Context). Let's see if any tests can tell us why: #107363
Ok, I guess our answer is: when using MakeTestingEvalContext in various tests the eval.Context.Txn.Sender is nil, so calling eval.Context.Txn.IsoLevel() will result in a nil pointer exception.
There was a problem hiding this comment.
Reviewable status:
Due to how descriptor leasing works, schema changes are not safe in weaker isolation transactions. Until they are safe, we disallow them. This includes a small change to the grammar so that SET with empty key names is consistently parsed, separate from the special SET ROW syntax. Release note: None
rafiss
force-pushed
the
read-committed-schema-changes
branch
from
6f535ca
to
a30e7b0
Compare
|
tftr! bors r=michae2 |
|
This PR was included in a batch that timed out, it will be automatically retried |
|
This PR was included in a batch that timed out, it will be automatically retried |
|
Build succeeded: |
Due to how descriptor leasing works, schema changes are not safe in weaker isolation transactions. Until they are safe, we disallow them.
fixes #100143
Release note: None