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().

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @michae2 and @nvanbenschoten)

pkg/sql/opt/exec/execbuilder/relational.go line 171 at r1 (raw file):

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)?

mod the resolution to Michael's question.

Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @michae2 and @rafiss)

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?

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @michae2 and @nvanbenschoten)

pkg/sql/table.go line 111 at r1 (raw file):

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.)

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @nvanbenschoten and @rafiss)

pkg/sql/opt/exec/execbuilder/relational.go line 171 at r1 (raw file):

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

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @nvanbenschoten and @rafiss)

pkg/sql/opt/exec/execbuilder/relational.go line 171 at r1 (raw file):

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.

I think we can defer answering this question for now if tests are passing, since this seems to be a test-only concern.

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten and @rafiss)