From 6817eb67bcf5daf163334b5abc2c38b471f07952 Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Tue, 26 Jul 2022 16:35:13 -0400 Subject: [PATCH] eval: stop ignoring all ResolveOIDFromOID errors 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. --- pkg/sql/faketreeeval/evalctx.go | 8 ++++---- pkg/sql/resolve_oid.go | 16 ++++++++-------- pkg/sql/sem/builtins/pg_builtins.go | 4 ++-- pkg/sql/sem/eval/cast.go | 5 ++++- pkg/sql/sem/eval/deps.go | 4 ++-- pkg/sql/sem/eval/parse_doid.go | 16 ++++++++++++---- 6 files changed, 32 insertions(+), 21 deletions(-) diff --git a/pkg/sql/faketreeeval/evalctx.go b/pkg/sql/faketreeeval/evalctx.go index fc2f609b72ee..4ef646506bc4 100644 --- a/pkg/sql/faketreeeval/evalctx.go +++ b/pkg/sql/faketreeeval/evalctx.go @@ -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. diff --git a/pkg/sql/resolve_oid.go b/pkg/sql/resolve_oid.go index 7c755be98447..7aa67eb8f588 100644 --- a/pkg/sql/resolve_oid.go +++ b/pkg/sql/resolve_oid.go @@ -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(), @@ -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(), @@ -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, @@ -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. diff --git a/pkg/sql/sem/builtins/pg_builtins.go b/pkg/sql/sem/builtins/pg_builtins.go index 8bbca082120b..19f91e953c99 100644 --- a/pkg/sql/sem/builtins/pg_builtins.go +++ b/pkg/sql/sem/builtins/pg_builtins.go @@ -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 diff --git a/pkg/sql/sem/eval/cast.go b/pkg/sql/sem/eval/cast.go index e861ee5ddf46..7fedc6246598 100644 --- a/pkg/sql/sem/eval/cast.go +++ b/pkg/sql/sem/eval/cast.go @@ -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 diff --git a/pkg/sql/sem/eval/deps.go b/pkg/sql/sem/eval/deps.go index dfedf9b1778f..04a0e3fba986 100644 --- a/pkg/sql/sem/eval/deps.go +++ b/pkg/sql/sem/eval/deps.go @@ -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. @@ -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. diff --git a/pkg/sql/sem/eval/parse_doid.go b/pkg/sql/sem/eval/parse_doid.go index 86dd82229d4d..d8b7c69601b3 100644 --- a/pkg/sql/sem/eval/parse_doid.go +++ b/pkg/sql/sem/eval/parse_doid.go @@ -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) } @@ -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 @@ -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 } }