Skip to content

Commit

Permalink
Merge #70274
Browse files Browse the repository at this point in the history 
70274: sql: address some cases of flattened error objects r=rafiss,ajwerner a=e-mbrown

refs #42510

There are some cases of error objects being flattened, which
strips them of all detail such as stack traces and user-directed
hints. This change address a few of the cases by wrapping the
error object.

Release note: None

Co-authored-by: e-mbrown <[email protected]>
  • Loading branch information
@e-mbrown
craig[bot] and e-mbrown committed Sep 16, 2021
2 parents 6139634 + b518475 commit 23c5fba
Show file tree
Hide file tree
Showing 4 changed files with 25 additions and 23 deletions.
2 changes: 1 addition & 1 deletion pkg/sql/alter_role.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,7 @@ func (p *planner) processSetOrResetClause(
ctx, expr, nil, tree.IndexedVarHelper{}, types.String, false, "ALTER ROLE ... SET ",
)
if err != nil {
return unknown, "", sessionVar{}, nil, wrapSetVarError(varName, expr.String(), "%v", err)
return unknown, "", sessionVar{}, nil, wrapSetVarError(err, varName, expr.String())
}
typedValues = append(typedValues, typedValue)
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/sql/sem/tree/type_check.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2398,8 +2398,8 @@ func typeCheckTupleComparison(
leftSubExprTyped, rightSubExprTyped, _, _, err := typeCheckComparisonOp(ctx, semaCtx, op, leftSubExpr, rightSubExpr)
if err != nil {
exps := Exprs([]Expr{left, right})
return nil, nil, pgerror.Newf(pgcode.DatatypeMismatch, "tuples %s are not comparable at index %d: %s",
&exps, elemIdx+1, err)
return nil, nil, pgerror.Wrapf(err, pgcode.DatatypeMismatch, "tuples %s are not comparable at index %d",
&exps, elemIdx+1)
}
left.Exprs[elemIdx] = leftSubExprTyped
left.typ.TupleContents()[elemIdx] = leftSubExprTyped.ResolvedType()
Expand Down Expand Up @@ -2457,7 +2457,7 @@ func typeCheckSameTypedTupleExprs(
}
typedSubExprs, resType, err := TypeCheckSameTypedExprs(ctx, semaCtx, desiredElem, sameTypeExprs...)
if err != nil {
return nil, nil, pgerror.Newf(pgcode.DatatypeMismatch, "tuples %s are not the same type: %v", Exprs(exprs), err)
return nil, nil, pgerror.Wrapf(err, pgcode.DatatypeMismatch, "tuples %s are not the same type", Exprs(exprs))
}
for j, typedExpr := range typedSubExprs {
tupleIdx := sameTypeExprsIndices[j]
Expand Down
34 changes: 18 additions & 16 deletions pkg/sql/set_var.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/util/duration"
"github.com/cockroachdb/cockroach/pkg/util/timeutil"
"github.com/cockroachdb/errors"
"github.com/cockroachdb/redact"
)

// setVarNode represents a SET {SESSION | LOCAL} statement.
Expand Down Expand Up @@ -76,7 +77,7 @@ func (p *planner) SetVar(ctx context.Context, n *tree.SetVar) (planNode, error)
typedValue, err := p.analyzeExpr(
ctx, expr, nil, dummyHelper, types.String, false, "SET SESSION "+name)
if err != nil {
return nil, wrapSetVarError(name, expr.String(), "%v", err)
return nil, wrapSetVarError(err, name, expr.String())
}
typedValues[i] = typedValue
}
Expand Down Expand Up @@ -223,14 +224,13 @@ func timeZoneVarGetStringVal(
timeutil.TimeZoneStringToLocationISO8601Standard,
)
if err != nil {
return "", wrapSetVarError("timezone", values[0].String(),
"cannot find time zone %q: %v", location, err)
return "", wrapSetVarError(errors.Wrapf(err, "cannot find time zone %q", location), "timezone", values[0].String())
}

case *tree.DInterval:
offset, _, _, err = v.Duration.Encode()
if err != nil {
return "", wrapSetVarError("timezone", values[0].String(), "%v", err)
return "", wrapSetVarError(err, "timezone", values[0].String())
}
offset /= int64(time.Second)

Expand All @@ -247,8 +247,7 @@ func timeZoneVarGetStringVal(
ed.Mul(sixty, sixty, &v.Decimal)
offset = ed.Int64(sixty)
if ed.Err() != nil {
return "", wrapSetVarError("timezone", values[0].String(),
"time zone value %s would overflow an int64", sixty)
return "", wrapSetVarError(errors.Newf("time zone value %s would overflow an int64", sixty), "timezone", values[0].String())
}

default:
Expand All @@ -267,7 +266,7 @@ func timeZoneVarSet(_ context.Context, m sessionDataMutator, s string) error {
timeutil.TimeZoneStringToLocationISO8601Standard,
)
if err != nil {
return wrapSetVarError("TimeZone", s, "%v", err)
return wrapSetVarError(err, "TimeZone", s)
}

m.SetLocation(loc)
Expand Down Expand Up @@ -296,7 +295,7 @@ func makeTimeoutVarGetter(
case *tree.DInterval:
timeout, err = intervalToDuration(v)
if err != nil {
return "", wrapSetVarError(varName, values[0].String(), "%v", err)
return "", wrapSetVarError(err, varName, values[0].String())
}
case *tree.DInt:
timeout = time.Duration(*v) * time.Millisecond
Expand All @@ -318,16 +317,15 @@ func validateTimeoutVar(
},
)
if err != nil {
return 0, wrapSetVarError(varName, timeString, "%v", err)
return 0, wrapSetVarError(err, varName, timeString)
}
timeout, err := intervalToDuration(interval)
if err != nil {
return 0, wrapSetVarError(varName, timeString, "%v", err)
return 0, wrapSetVarError(err, varName, timeString)
}

if timeout < 0 {
return 0, wrapSetVarError(varName, timeString,
"%v cannot have a negative duration", varName)
return 0, wrapSetVarError(errors.Newf("%v cannot have a negative duration", redact.SafeString(varName)), varName, timeString)
}

return timeout, nil
Expand Down Expand Up @@ -404,10 +402,14 @@ func newSingleArgVarError(varName string) error {
"SET %s takes only one argument", varName)
}

func wrapSetVarError(varName, actualValue string, fmt string, args ...interface{}) error {
err := pgerror.Newf(pgcode.InvalidParameterValue,
"invalid value for parameter %q: %q", varName, actualValue)
return errors.WithDetailf(err, fmt, args...)
func wrapSetVarError(cause error, varName, actualValue string) error {
return pgerror.Wrapf(
cause,
pgcode.InvalidParameterValue,
"invalid value for parameter %q: %q",
redact.SafeString(varName),
actualValue,
)
}

func newVarValueError(varName, actualVal string, allowedVals ...string) (err error) {
Expand Down
6 changes: 3 additions & 3 deletions pkg/sql/vars.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ var varGen = map[string]sessionVar{
Set: func(ctx context.Context, m sessionDataMutator, val string) error {
i, err := strconv.ParseInt(val, 10, 64)
if err != nil {
return wrapSetVarError("default_int_size", val, "%v", err)
return wrapSetVarError(err, "default_int_size", val)
}
if i != 4 && i != 8 {
return pgerror.New(pgcode.InvalidParameterValue,
Expand Down Expand Up @@ -813,7 +813,7 @@ var varGen = map[string]sessionVar{
) error {
i, err := strconv.ParseInt(s, 10, 64)
if err != nil {
return wrapSetVarError("extra_float_digits", s, "%v", err)
return wrapSetVarError(err, "extra_float_digits", s)
}
// Note: this is the range allowed by PostgreSQL.
// See also the documentation around (DataConversionConfig).GetFloatPrec()
Expand Down Expand Up @@ -1113,7 +1113,7 @@ var varGen = map[string]sessionVar{
Set: func(_ context.Context, _ sessionDataMutator, s string) error {
i, err := strconv.ParseInt(s, 10, 64)
if err != nil {
return wrapSetVarError("ssl_renegotiation_limit", s, "%v", err)
return wrapSetVarError(err, "ssl_renegotiation_limit", s)
}
if i != 0 {
// See pg src/backend/utils/misc/guc.c: non-zero values are not to be supported.
Expand Down

0 comments on commit 23c5fba

Please sign in to comment.