Skip to content

Commit

Permalink
eval: stop ignoring all ResolveOIDFromOID errors
Browse files Browse the repository at this point in the history
The decision about whether an error is safe to ignore is made at the
place where the error is created/returned. This way, the callers don't
need to be aware of any new error codes that the implementation may
start returning in the future.

Release note (bug fix): Fixed incorrect error handling that could cause
casts to OID types to fail in some cases.
  • Loading branch information
rafiss committed Jul 26, 2022
1 parent 9495e9c commit 6817eb6
Show file tree
Hide file tree
Showing 6 changed files with 32 additions and 21 deletions.
8 changes: 4 additions & 4 deletions pkg/sql/faketreeeval/evalctx.go
Original file line number Diff line number Diff line change
Expand Up @@ -159,15 +159,15 @@ type DummyEvalPlanner struct{}
// ResolveOIDFromString is part of the Planner interface.
func (ep *DummyEvalPlanner) ResolveOIDFromString(
ctx context.Context, resultType *types.T, toResolve *tree.DString,
) (*tree.DOid, error) {
return nil, errors.WithStack(errEvalPlanner)
) (*tree.DOid, bool, error) {
return nil, false, errors.WithStack(errEvalPlanner)
}

// ResolveOIDFromOID is part of the Planner interface.
func (ep *DummyEvalPlanner) ResolveOIDFromOID(
ctx context.Context, resultType *types.T, toResolve *tree.DOid,
) (*tree.DOid, error) {
return nil, errors.WithStack(errEvalPlanner)
) (*tree.DOid, bool, error) {
return nil, false, errors.WithStack(errEvalPlanner)
}

// UnsafeUpsertDescriptor is part of the Planner interface.
Expand Down
16 changes: 8 additions & 8 deletions pkg/sql/resolve_oid.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import (
// ResolveOIDFromString is part of tree.TypeResolver.
func (p *planner) ResolveOIDFromString(
ctx context.Context, resultType *types.T, toResolve *tree.DString,
) (*tree.DOid, error) {
) (_ *tree.DOid, errSafeToIgnore bool, _ error) {
ie := p.ExecCfg().InternalExecutorFactory(ctx, p.SessionData())
return resolveOID(
ctx, p.Txn(),
Expand All @@ -40,7 +40,7 @@ func (p *planner) ResolveOIDFromString(
// ResolveOIDFromOID is part of tree.TypeResolver.
func (p *planner) ResolveOIDFromOID(
ctx context.Context, resultType *types.T, toResolve *tree.DOid,
) (*tree.DOid, error) {
) (_ *tree.DOid, errSafeToIgnore bool, _ error) {
ie := p.ExecCfg().InternalExecutorFactory(ctx, p.SessionData())
return resolveOID(
ctx, p.Txn(),
Expand All @@ -55,10 +55,10 @@ func resolveOID(
ie sqlutil.InternalExecutor,
resultType *types.T,
toResolve tree.Datum,
) (*tree.DOid, error) {
) (_ *tree.DOid, errSafeToIgnore bool, _ error) {
info, ok := regTypeInfos[resultType.Oid()]
if !ok {
return nil, pgerror.Newf(
return nil, true, pgerror.Newf(
pgcode.InvalidTextRepresentation,
"invalid input syntax for type %s: %q",
resultType,
Expand All @@ -78,20 +78,20 @@ func resolveOID(
sessiondata.NoSessionDataOverride, q, toResolve)
if err != nil {
if errors.HasType(err, (*tree.MultipleResultsError)(nil)) {
return nil, pgerror.Newf(pgcode.AmbiguousAlias,
return nil, false, pgerror.Newf(pgcode.AmbiguousAlias,
"more than one %s named %s", info.objName, toResolve)
}
return nil, err
return nil, false, err
}
if results.Len() == 0 {
return nil, pgerror.Newf(info.errType,
return nil, true, pgerror.Newf(info.errType,
"%s %s does not exist", info.objName, toResolve)
}
return tree.NewDOidWithName(
results[0].(*tree.DOid).Oid,
resultType,
tree.AsStringWithFlags(results[1], tree.FmtBareStrings),
), nil
), true, nil
}

// regTypeInfo contains details on a pg_catalog table that has a reg* type.
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/sem/builtins/pg_builtins.go
Original file line number Diff line number Diff line change
Expand Up @@ -772,13 +772,13 @@ var pgBuiltins = map[string]builtinDefinition{
// The session has not yet created a temporary schema.
return tree.NewDOid(0), nil
}
oid, err := ctx.Planner.ResolveOIDFromString(
oid, errSafeToIgnore, err := ctx.Planner.ResolveOIDFromString(
ctx.Ctx(), types.RegNamespace, tree.NewDString(schema))
if err != nil {
// If the OID lookup returns an UndefinedObject error, return 0
// instead. We can hit this path if the session created a temporary
// schema in one database and then changed databases.
if pgerror.GetPGCode(err) == pgcode.UndefinedObject {
if errSafeToIgnore && pgerror.GetPGCode(err) == pgcode.UndefinedObject {
return tree.NewDOid(0), nil
}
return nil, err
Expand Down
5 changes: 4 additions & 1 deletion pkg/sql/sem/eval/cast.go
Original file line number Diff line number Diff line change
Expand Up @@ -955,8 +955,11 @@ func performIntToOidCast(
return tree.WrapAsZeroOid(t), nil
}

dOid, err := res.ResolveOIDFromOID(ctx, t, tree.NewDOid(o))
dOid, errSafeToIgnore, err := res.ResolveOIDFromOID(ctx, t, tree.NewDOid(o))
if err != nil {
if !errSafeToIgnore {
return nil, err
}
dOid = tree.NewDOidWithType(o, t)
}
return dOid, nil
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/sem/eval/deps.go
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ type TypeResolver interface {
// query, an error will be returned.
ResolveOIDFromString(
ctx context.Context, resultType *types.T, toResolve *tree.DString,
) (*tree.DOid, error)
) (_ *tree.DOid, errSafeToIgnore bool, _ error)

// ResolveOIDFromOID looks up the populated value of the oid with the
// desired resultType which matches the provided oid.
Expand All @@ -181,7 +181,7 @@ type TypeResolver interface {
// query, an error will be returned.
ResolveOIDFromOID(
ctx context.Context, resultType *types.T, toResolve *tree.DOid,
) (*tree.DOid, error)
) (_ *tree.DOid, errSafeToIgnore bool, _ error)
}

// Planner is a limited planner that can be used from EvalContext.
Expand Down
16 changes: 12 additions & 4 deletions pkg/sql/sem/eval/parse_doid.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,11 @@ func ParseDOid(ctx *Context, s string, t *types.T) (*tree.DOid, error) {
if err != nil {
return nil, err
}
oidRes, err := ctx.Planner.ResolveOIDFromOID(ctx.Ctx(), t, tmpOid)
oidRes, errSafeToIgnore, err := ctx.Planner.ResolveOIDFromOID(ctx.Ctx(), t, tmpOid)
if err != nil {
if !errSafeToIgnore {
return nil, err
}
oidRes = tmpOid
*oidRes = tree.MakeDOid(tmpOid.Oid, t)
}
Expand Down Expand Up @@ -104,9 +107,13 @@ func ParseDOid(ctx *Context, s string, t *types.T) (*tree.DOid, error) {
// Trim type modifiers, e.g. `numeric(10,3)` becomes `numeric`.
s = pgSignatureRegexp.ReplaceAllString(s, "$1")

dOid, missingTypeErr := ctx.Planner.ResolveOIDFromString(ctx.Ctx(), t, tree.NewDString(tree.Name(s).Normalize()))
dOid, errSafeToIgnore, missingTypeErr := ctx.Planner.ResolveOIDFromString(
ctx.Ctx(), t, tree.NewDString(tree.Name(s).Normalize()),
)
if missingTypeErr == nil {
return dOid, missingTypeErr
return dOid, nil
} else if !errSafeToIgnore {
return nil, missingTypeErr
}
// Fall back to some special cases that we support for compatibility
// only. Client use syntax like 'sometype'::regtype to produce the oid
Expand Down Expand Up @@ -137,7 +144,8 @@ func ParseDOid(ctx *Context, s string, t *types.T) (*tree.DOid, error) {
return tree.NewDOidWithTypeAndName(oid.Oid(id), t, tn.ObjectName.String()), nil

default:
return ctx.Planner.ResolveOIDFromString(ctx.Ctx(), t, tree.NewDString(s))
d, _ /* errSafeToIgnore */, err := ctx.Planner.ResolveOIDFromString(ctx.Ctx(), t, tree.NewDString(s))
return d, err
}
}

Expand Down

0 comments on commit 6817eb6

Please sign in to comment.