opt: FoldAssignmentCast ignores transaction errors

[fqazi]

Describe the problem

Please describe the issue you observed, and any steps we can take to reproduce it:
Currently, if a transaction error is hit inside FoldAssignmentCast when calling PerformAssignmentCast the errors are silently swallowed. This can happen when dealing with OIDs which will potentially involve running SQL using the internal executor. When this scenario is encountered we continue execution like normal with a potentially aborted transaction causing failures in other code paths.

To Reproduce

The original issue was reproduced on the randomized schema changer workload, when inserts with OID casts are executed. However, this issue can be easily faked by forcing a transaction to abort when an OID involved in an insert is being resolved.

Jira issue: CRDB-18380

[mgartner]

@fqazi Do you have reproduction steps for this?

[mgartner]

Yahor says it's difficult to reproduce, but it's possible to fake an error to see the problems this creates.

[fqazi]

The transaction retry is hard to repro standalone if you take the ResolveOID* and inject a transaction retry error or generate a real one with a paused thread. If we need to validate the fix the random-load variant of the schema changer workload is pretty easy to run, and we can see it in 10 minutes at most.

[ajwerner]

cockroach/pkg/sql/opt/norm/fold_constants_funcs.go

Lines 397 to 400 in 31ffb42

	result, err := eval.PerformAssignmentCast(c.f.evalCtx, datum, typ)
	if err != nil {
	return nil, false
	}

[mgartner]

Great find! This is a bad one. I want to take some time to audit all these casts - any cast that has to read some mutable state should not be folded during normalization. @ajwerner and I also discussed how labelling these casts as IMMUTABLE is deceiving at best, and incorrect at worst. I'll have to think more about these cases. This relates to #74470.

[ajwerner]

any cast that has to read some mutable state should not be folded during normalization

I remain meh on this take. I suspect that folding regclass casts is probably important for a bunch of queries. I think that for all intents and purposes, these casts are stable and for the usage of the memo, are even immutable. Ideally we'd just propagate errors which relate to concurrency control. I know that's a bit tricky.

[ajwerner]

Alright in light of #74470, I agree that immutable is not fitting. Stable is fitting and folding these stable values may be important.

[rafiss]

I thought we had a linter for not returning errors (returnerrcheck)? But it looks like this problem (i.e. not returning an error that could have caused the txn to abort) has been happening in a few places.

[ajwerner]

The returnerrcheck linter doesn't do anything if the function doesn't itself return an error. The optimizer doesn't really return errors, it panics them, so the linter does not trigger.

[rafiss]

The returnerrcheck linter doesn't do anything if the function doesn't itself return an error. The optimizer doesn't really return errors, it panics them, so the linter does not trigger.

I see, thanks.

---

So do we know if this issue is another cause of #82034 and #80764?

[ajwerner]

So do we know if this issue is another cause of #82034 and #80764?

Yes, I believe we do know that this is a cause of #80764 (#80764 (comment)).

[rafiss]

@mgartner regarding your earlier comment:

I want to take some time to audit all these casts - any cast that has to read some mutable state should not be folded during normalization. @ajwerner and I also discussed how labelling these casts as IMMUTABLE is deceiving at best, and incorrect at worst. I'll have to think more about these cases.

does that work you're describing to audit all the casts need to happen before making the change to stop swallowing the error at

cockroach/pkg/sql/opt/norm/fold_constants_funcs.go

Lines 397 to 400 in 31ffb42

	result, err := eval.PerformAssignmentCast(c.f.evalCtx, datum, typ)
	if err != nil {
	return nil, false
	}

[mgartner]

No, I can fix that now, I think.

[mgartner]

I'll point out that the pattern of swallowing an evaluation error is used in other cases:

cockroach/pkg/sql/opt/norm/fold_constants_funcs.go

Lines 469 to 472 in 31ffb42

	result, err := eval.BinaryOp(c.f.evalCtx, o.EvalOp, lDatum, rDatum)
	if err != nil {
	return nil, false
	}

cockroach/pkg/sql/opt/norm/fold_constants_funcs.go

Line 514 in 31ffb42

result, err := eval.Expr(c.f.evalCtx, texpr)

cockroach/pkg/sql/opt/norm/fold_constants_funcs.go

Lines 618 to 621 in 31ffb42

	result, err := eval.Expr(c.f.evalCtx, fn)
	if err != nil {
	return nil, false
	}

The reason we can do this is because we only try to evaluate immutable or stable expressions. I am hesitant to change all the places where we swallow errors - folding expressions is best-effort, and if it fails for any reason, we'd prefer to continue with optimization and leave the expression as-is. So I think changing this assignment cast case is a temporary solution - ideally we can fix the volatility of the cast and then revert to swallowing errors.

[ajwerner]

An approach we've taken elsewhere is to classify the errors which can be swallowed safely. We don't have a unified way to do this presently. In the cases I'm thinking of, we literally passed a boolean with the error. That wasn't great.

[mgartner]

Looks like this was fixed by #87614.