From d93fac3f43c28dd6fb9bfe0abd7b9193317eb02e Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Tue, 19 Mar 2019 00:37:48 +0100 Subject: [PATCH] sql: don't test error identities using reference equality Prior to this patch, the SQL code was expecting to identify errors by addess, ie. something like: ```go // at global scope var errSomething = errors.New(...) // somewhere in the code if err == errSomething { ... } ``` This approach was flawed in that errors can be wrapped (`errors.Wrap`, `errors.WithMessage`) or flattened (`pgerror.Wrap`) or sent over the wire. To make the code more robust, this patch introduces a new API `pgerror.WithMarker()` which instantiates errors with a unique marker. The marker is guaranteed to be preserved through `errors.Wrap` and `pgerror.Wrap` calls. It can be tested with `pgerror.IsMarkedError()`. The patch then updates the various points in the SQL code where errors were tested in that way, to use the new API. (In some places some attempt is made to "work around" with a string comparison on the message instead, but that's arguably even worse for reasons left as an exercise to the reader. This should also be fixed, but that fix is out of scope here.) Release note: None --- pkg/sql/crdb_internal.go | 2 +- pkg/sql/exec_util.go | 2 +- pkg/sql/lease.go | 24 +-- pkg/sql/opt_catalog.go | 2 +- pkg/sql/pgwire/pgerror/errors.go | 6 +- pkg/sql/pgwire/pgerror/errors.pb.go | 107 +++++++++---- pkg/sql/pgwire/pgerror/errors.proto | 6 + pkg/sql/pgwire/pgerror/markers.go | 202 +++++++++++++++++++++++++ pkg/sql/pgwire/pgerror/markers_test.go | 70 +++++++++ pkg/sql/pgwire/pgerror/wrap.go | 8 +- pkg/sql/physical_schema_accessors.go | 3 +- pkg/sql/planner.go | 3 +- pkg/sql/row/fk_existence_base.go | 2 +- pkg/sql/row/fk_existence_delete.go | 3 +- pkg/sql/row/fk_existence_insert.go | 3 +- pkg/sql/schema_changer.go | 78 +++++----- pkg/sql/schema_changer_test.go | 3 +- pkg/sql/set_zone_config.go | 2 +- pkg/sql/show_zone_config.go | 3 +- pkg/sql/sqlbase/structured.go | 4 +- pkg/sql/table.go | 10 +- pkg/sql/zone_config.go | 9 +- 22 files changed, 443 insertions(+), 109 deletions(-) create mode 100644 pkg/sql/pgwire/pgerror/markers.go create mode 100644 pkg/sql/pgwire/pgerror/markers_test.go diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go index c8f35c450ee3..fc0d8c8409a6 100644 --- a/pkg/sql/crdb_internal.go +++ b/pkg/sql/crdb_internal.go @@ -1844,7 +1844,7 @@ CREATE TABLE crdb_internal.zones ( for _, s := range subzones { index, err := table.FindIndexByID(sqlbase.IndexID(s.IndexID)) if err != nil { - if err == sqlbase.ErrIndexGCMutationsList { + if pgerror.IsMarkedError(err, sqlbase.ErrIndexGCMutationsList) { continue } return err diff --git a/pkg/sql/exec_util.go b/pkg/sql/exec_util.go index 4638158743a7..2bc87b99a8f3 100644 --- a/pkg/sql/exec_util.go +++ b/pkg/sql/exec_util.go @@ -976,7 +976,7 @@ func (scc *schemaChangerCollection) execSchemaChanges( log.Warningf(ctx, "error executing schema change: %s", err) } - if err == sqlbase.ErrDescriptorNotFound || err == ctx.Err() { + if pgerror.IsMarkedError(err, sqlbase.ErrDescriptorNotFound) || err == ctx.Err() { // 1. If the descriptor is dropped while the schema change // is executing, the schema change is considered completed. // 2. If the context is canceled the schema changer quits here diff --git a/pkg/sql/lease.go b/pkg/sql/lease.go index 452006916aa1..114271691c4d 100644 --- a/pkg/sql/lease.go +++ b/pkg/sql/lease.go @@ -46,8 +46,8 @@ import ( "github.com/pkg/errors" ) -var errRenewLease = errors.New("renew lease on id") -var errReadOlderTableVersion = errors.New("read older table version from store") +var errRenewLease = pgerror.WithMarker(errors.New("renew lease on id")) +var errReadOlderTableVersion = pgerror.WithMarker(errors.New("read older table version from store")) // A lease stored in system.lease. type storedTableLease struct { @@ -337,7 +337,9 @@ func maybeIncrementVersion( return nil } -var errDidntUpdateDescriptor = errors.New("didn't update the table descriptor") +var errDidntUpdateDescriptor = pgerror.WithMarker(errors.New("didn't update the table descriptor")) + +var errLeaseVersionChanged = pgerror.WithMarker(errors.New("lease version changed")) // Publish updates a table descriptor. It also maintains the invariant that // there are at most two versions of the descriptor out in the wild at any time @@ -355,7 +357,6 @@ func (s LeaseStore) Publish( update func(*sqlbase.MutableTableDescriptor) error, logEvent func(*client.Txn) error, ) (*sqlbase.ImmutableTableDescriptor, error) { - errLeaseVersionChanged := errors.New("lease version changed") // Retry while getting errLeaseVersionChanged. for r := retry.Start(base.DefaultRetryOptions()); r.Next(); { // Wait until there are no unexpired leases on the previous version @@ -428,10 +429,10 @@ func (s LeaseStore) Publish( return txn.CommitInBatch(ctx, b) }) - switch err { - case nil, errDidntUpdateDescriptor: + switch { + case err == nil, pgerror.IsMarkedError(err, errDidntUpdateDescriptor): return sqlbase.NewImmutableTableDescriptor(tableDesc.TableDescriptor), nil - case errLeaseVersionChanged: + case pgerror.IsMarkedError(err, errLeaseVersionChanged): // will loop around to retry default: return nil, err @@ -1061,7 +1062,7 @@ func purgeOldVersions( // active lease, so that it doesn't get released when removeInactives() // is called below. Release this lease after calling removeInactives(). table, _, err := t.findForTimestamp(ctx, m.execCfg.Clock.Now()) - if dropped := err == errTableDropped; dropped || err == nil { + if dropped := pgerror.IsMarkedError(err, errTableDropped); dropped || err == nil { removeInactives(dropped) if table != nil { s, err := t.release(&table.ImmutableTableDescriptor, m.removeOnceDereferenced()) @@ -1531,8 +1532,9 @@ func (m *LeaseManager) Acquire( } return &table.ImmutableTableDescriptor, table.expiration, nil } - switch err { - case errRenewLease: + + switch { + case pgerror.IsMarkedError(err, errRenewLease): // Renew lease and retry. This will block until the lease is acquired. if _, errLease := acquireNodeLease(ctx, m, tableID); errLease != nil { return nil, hlc.Timestamp{}, errLease @@ -1541,7 +1543,7 @@ func (m *LeaseManager) Acquire( m.testingKnobs.LeaseStoreTestingKnobs.LeaseAcquireResultBlockEvent(LeaseAcquireBlock) } - case errReadOlderTableVersion: + case pgerror.IsMarkedError(err, errReadOlderTableVersion): // Read old table versions from the store. This can block while reading // old table versions from the store. versions, errRead := m.readOlderVersionForTimestamp(ctx, tableID, timestamp) diff --git a/pkg/sql/opt_catalog.go b/pkg/sql/opt_catalog.go index 1eff0581417b..d0054e0de995 100644 --- a/pkg/sql/opt_catalog.go +++ b/pkg/sql/opt_catalog.go @@ -155,7 +155,7 @@ func (oc *optCatalog) ResolveDataSourceByID( tableLookup, err := oc.planner.LookupTableByID(ctx, sqlbase.ID(dataSourceID)) if err != nil || tableLookup.IsAdding { - if err == sqlbase.ErrDescriptorNotFound || tableLookup.IsAdding { + if pgerror.IsMarkedError(err, sqlbase.ErrDescriptorNotFound) || tableLookup.IsAdding { return nil, sqlbase.NewUndefinedRelationError(&tree.TableRef{TableID: int64(dataSourceID)}) } return nil, err diff --git a/pkg/sql/pgwire/pgerror/errors.go b/pkg/sql/pgwire/pgerror/errors.go index 04cfa27d439a..0267e856ab09 100644 --- a/pkg/sql/pgwire/pgerror/errors.go +++ b/pkg/sql/pgwire/pgerror/errors.go @@ -64,11 +64,13 @@ func formatMsgHintDetail(prefix, msg, hint, detail string) string { // information at the specified depth level. func NewErrorWithDepthf(depth int, code string, format string, args ...interface{}) *Error { srcCtx := makeSrcCtx(depth + 1) - return &Error{ + err := &Error{ Message: fmt.Sprintf(format, args...), Code: code, Source: &srcCtx, } + err.OriginMarker = getMarker(err) + return err } // NewError creates an Error. @@ -221,7 +223,7 @@ var _ fmt.Formatter = &Error{} // Format implements the fmt.Formatter interface. // -// %v/%s prints the rror as usual. +// %v/%s prints the error as usual. // %#v adds the pg error code at the beginning. // %+v prints all the details, including the embedded stack traces. func (pg *Error) Format(s fmt.State, verb rune) { diff --git a/pkg/sql/pgwire/pgerror/errors.pb.go b/pkg/sql/pgwire/pgerror/errors.pb.go index 2c6e7d49c3d8..0b63fcf5abc7 100644 --- a/pkg/sql/pgwire/pgerror/errors.pb.go +++ b/pkg/sql/pgwire/pgerror/errors.pb.go @@ -37,16 +37,21 @@ type Error struct { TelemetryKey string `protobuf:"bytes,6,opt,name=telemetry_key,json=telemetryKey,proto3" json:"telemetry_key,omitempty"` // complement to the detail field that can be reported // in sentry reports. This is scrubbed of PII. - SafeDetail []*Error_SafeDetail `protobuf:"bytes,7,rep,name=safe_detail,json=safeDetail,proto3" json:"safe_detail,omitempty"` - XXX_NoUnkeyedLiteral struct{} `json:"-"` - XXX_sizecache int32 `json:"-"` + SafeDetail []*Error_SafeDetail `protobuf:"bytes,7,rep,name=safe_detail,json=safeDetail,proto3" json:"safe_detail,omitempty"` + // the origin marker is guaranteed to propagate as-is + // through error wraps, and can be used to recognize + // the original cause of a pgerror. + // This may contain PII and should not be reported. + OriginMarker string `protobuf:"bytes,8,opt,name=origin_marker,json=originMarker,proto3" json:"origin_marker,omitempty"` + XXX_NoUnkeyedLiteral struct{} `json:"-"` + XXX_sizecache int32 `json:"-"` } func (m *Error) Reset() { *m = Error{} } func (m *Error) String() string { return proto.CompactTextString(m) } func (*Error) ProtoMessage() {} func (*Error) Descriptor() ([]byte, []int) { - return fileDescriptor_errors_a71973a7ba445e50, []int{0} + return fileDescriptor_errors_7e07537ae42edeaa, []int{0} } func (m *Error) XXX_Unmarshal(b []byte) error { return m.Unmarshal(b) @@ -83,7 +88,7 @@ func (m *Error_Source) Reset() { *m = Error_Source{} } func (m *Error_Source) String() string { return proto.CompactTextString(m) } func (*Error_Source) ProtoMessage() {} func (*Error_Source) Descriptor() ([]byte, []int) { - return fileDescriptor_errors_a71973a7ba445e50, []int{0, 0} + return fileDescriptor_errors_7e07537ae42edeaa, []int{0, 0} } func (m *Error_Source) XXX_Unmarshal(b []byte) error { return m.Unmarshal(b) @@ -119,7 +124,7 @@ func (m *Error_SafeDetail) Reset() { *m = Error_SafeDetail{} } func (m *Error_SafeDetail) String() string { return proto.CompactTextString(m) } func (*Error_SafeDetail) ProtoMessage() {} func (*Error_SafeDetail) Descriptor() ([]byte, []int) { - return fileDescriptor_errors_a71973a7ba445e50, []int{0, 1} + return fileDescriptor_errors_7e07537ae42edeaa, []int{0, 1} } func (m *Error_SafeDetail) XXX_Unmarshal(b []byte) error { return m.Unmarshal(b) @@ -216,6 +221,12 @@ func (m *Error) MarshalTo(dAtA []byte) (int, error) { i += n } } + if len(m.OriginMarker) > 0 { + dAtA[i] = 0x42 + i++ + i = encodeVarintErrors(dAtA, i, uint64(len(m.OriginMarker))) + i += copy(dAtA[i:], m.OriginMarker) + } return i, nil } @@ -329,6 +340,10 @@ func (m *Error) Size() (n int) { n += 1 + l + sovErrors(uint64(l)) } } + l = len(m.OriginMarker) + if l > 0 { + n += 1 + l + sovErrors(uint64(l)) + } return n } @@ -620,6 +635,35 @@ func (m *Error) Unmarshal(dAtA []byte) error { return err } iNdEx = postIndex + case 8: + if wireType != 2 { + return fmt.Errorf("proto: wrong wireType = %d for field OriginMarker", wireType) + } + var stringLen uint64 + for shift := uint(0); ; shift += 7 { + if shift >= 64 { + return ErrIntOverflowErrors + } + if iNdEx >= l { + return io.ErrUnexpectedEOF + } + b := dAtA[iNdEx] + iNdEx++ + stringLen |= (uint64(b) & 0x7F) << shift + if b < 0x80 { + break + } + } + intStringLen := int(stringLen) + if intStringLen < 0 { + return ErrInvalidLengthErrors + } + postIndex := iNdEx + intStringLen + if postIndex > l { + return io.ErrUnexpectedEOF + } + m.OriginMarker = string(dAtA[iNdEx:postIndex]) + iNdEx = postIndex default: iNdEx = preIndex skippy, err := skipErrors(dAtA[iNdEx:]) @@ -982,31 +1026,32 @@ var ( ) func init() { - proto.RegisterFile("sql/pgwire/pgerror/errors.proto", fileDescriptor_errors_a71973a7ba445e50) + proto.RegisterFile("sql/pgwire/pgerror/errors.proto", fileDescriptor_errors_7e07537ae42edeaa) } -var fileDescriptor_errors_a71973a7ba445e50 = []byte{ - // 346 bytes of a gzipped FileDescriptorProto - 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0x74, 0x51, 0xb1, 0x4e, 0x2a, 0x41, - 0x14, 0x65, 0x1f, 0xb0, 0xbc, 0x77, 0xe1, 0x15, 0x8c, 0x89, 0x99, 0x50, 0x2c, 0x20, 0x0d, 0xd5, - 0x92, 0x60, 0x61, 0x6f, 0xb0, 0x52, 0x9b, 0xc5, 0xca, 0x66, 0x33, 0x0e, 0x77, 0x61, 0xc3, 0xb2, - 0x83, 0x33, 0x43, 0x0c, 0x7f, 0xe1, 0xc7, 0xf8, 0x11, 0x94, 0x96, 0x96, 0xba, 0xfe, 0x88, 0x99, - 0xd9, 0x61, 0x2d, 0x8c, 0xcd, 0xe4, 0xdc, 0x7b, 0x6e, 0xce, 0x3d, 0xf7, 0x0c, 0xf4, 0xd5, 0x63, - 0x36, 0xd9, 0x2e, 0x9f, 0x52, 0x89, 0x93, 0xed, 0x12, 0xa5, 0x14, 0x72, 0x62, 0x5f, 0x15, 0x6e, - 0xa5, 0xd0, 0x82, 0x74, 0xb9, 0xe0, 0x6b, 0x29, 0x18, 0x5f, 0x85, 0x8e, 0x3f, 0x7b, 0xa9, 0x43, - 0xf3, 0xca, 0x20, 0x42, 0xa0, 0xc1, 0xc5, 0x02, 0xa9, 0x37, 0xf0, 0xc6, 0xff, 0x22, 0x8b, 0x09, - 0x85, 0xd6, 0x06, 0x95, 0x62, 0x4b, 0xa4, 0x7f, 0x6c, 0xfb, 0x58, 0x92, 0x53, 0xf0, 0x17, 0xa8, - 0x59, 0x9a, 0xd1, 0xba, 0x25, 0x5c, 0x65, 0x54, 0x56, 0x69, 0xae, 0x69, 0xa3, 0x54, 0x31, 0x98, - 0x5c, 0x80, 0xaf, 0xc4, 0x4e, 0x72, 0xa4, 0xcd, 0x81, 0x37, 0x6e, 0x4f, 0xfb, 0xe1, 0x0f, 0x1f, - 0xa1, 0xf5, 0x10, 0xce, 0xed, 0x58, 0xe4, 0xc6, 0xc9, 0x08, 0xfe, 0x6b, 0xcc, 0x70, 0x83, 0x5a, - 0xee, 0xe3, 0x35, 0xee, 0xa9, 0x6f, 0x55, 0x3b, 0x55, 0xf3, 0x1a, 0xf7, 0x64, 0x06, 0x6d, 0xc5, - 0x12, 0x8c, 0x9d, 0x9d, 0xd6, 0xa0, 0x3e, 0x6e, 0x4f, 0x47, 0xbf, 0xaf, 0x60, 0x09, 0xce, 0xec, - 0x68, 0x04, 0xaa, 0xc2, 0xbd, 0x1b, 0xf0, 0xcb, 0xe5, 0xe6, 0x82, 0x24, 0xcd, 0xaa, 0x1c, 0x0c, - 0x36, 0xbd, 0x2c, 0xcd, 0xcb, 0x10, 0x9a, 0x91, 0xc5, 0xa4, 0x07, 0x7f, 0x93, 0x5d, 0xce, 0x75, - 0x2a, 0x72, 0x97, 0x41, 0x55, 0xf7, 0x62, 0x80, 0xef, 0x3d, 0x64, 0x08, 0x1d, 0xeb, 0xf0, 0x18, - 0x65, 0xa9, 0x6c, 0x5d, 0xdf, 0xba, 0x38, 0x43, 0x38, 0xc1, 0xdc, 0x44, 0xbe, 0x88, 0x95, 0x66, - 0x7c, 0x1d, 0x6b, 0xc9, 0xf8, 0x31, 0xf4, 0xae, 0xa3, 0xe6, 0x86, 0xb9, 0x33, 0xc4, 0xe5, 0xf0, - 0xf0, 0x11, 0xd4, 0x0e, 0x45, 0xe0, 0xbd, 0x16, 0x81, 0xf7, 0x56, 0x04, 0xde, 0x7b, 0x11, 0x78, - 0xcf, 0x9f, 0x41, 0xed, 0xbe, 0xe5, 0xce, 0x7d, 0xf0, 0xed, 0x9f, 0x9f, 0x7f, 0x05, 0x00, 0x00, - 0xff, 0xff, 0xec, 0x81, 0x3a, 0x52, 0x16, 0x02, 0x00, 0x00, +var fileDescriptor_errors_7e07537ae42edeaa = []byte{ + // 365 bytes of a gzipped FileDescriptorProto + 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0x74, 0x52, 0x31, 0x6f, 0xe2, 0x30, + 0x14, 0x26, 0x07, 0x04, 0xce, 0x70, 0x03, 0x3e, 0xe9, 0x64, 0x31, 0x04, 0x38, 0x16, 0xa6, 0x20, + 0x71, 0xc3, 0xed, 0x15, 0x9d, 0x5a, 0x96, 0xd0, 0xa9, 0x4b, 0xe4, 0x9a, 0x97, 0x60, 0x25, 0xc4, + 0xd4, 0x36, 0xaa, 0xf8, 0x17, 0xfd, 0x59, 0x8c, 0x1d, 0xdb, 0xad, 0x4d, 0xff, 0x48, 0x65, 0xc7, + 0xa4, 0x43, 0xd5, 0x25, 0xfa, 0xde, 0xf7, 0x3d, 0xbd, 0xf7, 0xbd, 0xcf, 0x41, 0x23, 0x75, 0x9f, + 0xcf, 0xf7, 0xe9, 0x03, 0x97, 0x30, 0xdf, 0xa7, 0x20, 0xa5, 0x90, 0x73, 0xfb, 0x55, 0xe1, 0x5e, + 0x0a, 0x2d, 0xf0, 0x80, 0x09, 0x96, 0x49, 0x41, 0xd9, 0x36, 0x74, 0xfa, 0xdf, 0x97, 0x26, 0x6a, + 0x5f, 0x1a, 0x84, 0x31, 0x6a, 0x31, 0xb1, 0x01, 0xe2, 0x8d, 0xbd, 0xd9, 0xcf, 0xc8, 0x62, 0x4c, + 0x50, 0x67, 0x07, 0x4a, 0xd1, 0x14, 0xc8, 0x0f, 0x4b, 0x9f, 0x4b, 0xfc, 0x07, 0xf9, 0x1b, 0xd0, + 0x94, 0xe7, 0xa4, 0x69, 0x05, 0x57, 0x99, 0x29, 0x5b, 0x5e, 0x68, 0xd2, 0xaa, 0xa6, 0x18, 0x8c, + 0xff, 0x23, 0x5f, 0x89, 0x83, 0x64, 0x40, 0xda, 0x63, 0x6f, 0xd6, 0x5b, 0x8c, 0xc2, 0x2f, 0x3e, + 0x42, 0xeb, 0x21, 0x5c, 0xdb, 0xb6, 0xc8, 0xb5, 0xe3, 0x29, 0xfa, 0xa5, 0x21, 0x87, 0x1d, 0x68, + 0x79, 0x8c, 0x33, 0x38, 0x12, 0xdf, 0x4e, 0xed, 0xd7, 0xe4, 0x15, 0x1c, 0xf1, 0x12, 0xf5, 0x14, + 0x4d, 0x20, 0x76, 0x76, 0x3a, 0xe3, 0xe6, 0xac, 0xb7, 0x98, 0x7e, 0xbf, 0x82, 0x26, 0xb0, 0xb4, + 0xad, 0x11, 0x52, 0x35, 0x36, 0xab, 0x84, 0xe4, 0x29, 0x2f, 0xe2, 0x1d, 0x95, 0x19, 0x48, 0xd2, + 0xad, 0x56, 0x55, 0xe4, 0xca, 0x72, 0xc3, 0x6b, 0xe4, 0x57, 0x0e, 0xcd, 0x99, 0x09, 0xcf, 0xeb, + 0xb0, 0x0c, 0x36, 0x5c, 0xce, 0x8b, 0x2a, 0xa9, 0x76, 0x64, 0x31, 0x1e, 0xa2, 0x6e, 0x72, 0x28, + 0x98, 0xe6, 0xa2, 0x70, 0x41, 0xd5, 0xf5, 0x30, 0x46, 0xe8, 0xd3, 0x0c, 0x9e, 0xa0, 0xbe, 0x3d, + 0xe3, 0x9c, 0x77, 0x35, 0xd9, 0x9e, 0xb6, 0x72, 0x99, 0x87, 0xe8, 0x37, 0x14, 0xe6, 0x5d, 0x36, + 0xb1, 0xd2, 0x94, 0x65, 0xb1, 0x96, 0x94, 0x9d, 0x5f, 0x66, 0xe0, 0xa4, 0xb5, 0x51, 0x6e, 0x8c, + 0x70, 0x31, 0x39, 0xbd, 0x05, 0x8d, 0x53, 0x19, 0x78, 0x4f, 0x65, 0xe0, 0x3d, 0x97, 0x81, 0xf7, + 0x5a, 0x06, 0xde, 0xe3, 0x7b, 0xd0, 0xb8, 0xed, 0xb8, 0x4c, 0xee, 0x7c, 0xfb, 0x63, 0xfc, 0xfb, + 0x08, 0x00, 0x00, 0xff, 0xff, 0xa0, 0xad, 0x99, 0x81, 0x3b, 0x02, 0x00, 0x00, } diff --git a/pkg/sql/pgwire/pgerror/errors.proto b/pkg/sql/pgwire/pgerror/errors.proto index 95f18f73e700..d00194babd76 100644 --- a/pkg/sql/pgwire/pgerror/errors.proto +++ b/pkg/sql/pgwire/pgerror/errors.proto @@ -50,4 +50,10 @@ message Error { // complement to the detail field that can be reported // in sentry reports. This is scrubbed of PII. repeated SafeDetail safe_detail = 7; + + // the origin marker is guaranteed to propagate as-is + // through error wraps, and can be used to recognize + // the original cause of a pgerror. + // This may contain PII and should not be reported. + string origin_marker = 8; }; diff --git a/pkg/sql/pgwire/pgerror/markers.go b/pkg/sql/pgwire/pgerror/markers.go new file mode 100644 index 000000000000..748752847fa5 --- /dev/null +++ b/pkg/sql/pgwire/pgerror/markers.go @@ -0,0 +1,202 @@ +// Copyright 2019 The Cockroach Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +// implied. See the License for the specific language governing +// permissions and limitations under the License. + +package pgerror + +import ( + "context" + "fmt" + "io" + "reflect" + + "github.com/pkg/errors" +) + +// Marker errors are designed to solve a problem: recognize +// when a pgerror is derived from a particular error object. +// +// This can be used e.g. to recognized particular origins for errors. +// +// The rules are as follows: +// +// - undecorated errors are considered to have equivalent markers if +// they have the same type (including package path for the type), +// and for Error objects also if they have the same code. +// +// - use WithMarker(err) to decorate an error with a new marker that's +// derived from its original marker but includes the file path of +// the caller of WithMarker(). This can be used to produce separate +// markers from that of undecorated errors, for example to separate +// errors generated in different places with errors.New() using the +// same message. +// +// - if an error is already marked, WithMarker() will panic. +// +// - Wrap() in this package, and errors.Wrap(), preserve the marker. +// +// - use pgerror.IsMarkedError(err1, err2) to test whether two +// errors have equivalent markers. +// +// We use a string marker instead of the more common/traditional +// chaining through errors.Wrap() and the Cause() interface, +// because pgerror.Error objects must flow through the network +// via protobuf, and the errors' package Cause() mechanism +// is not protobuf-encodable. +// +// We use the file path specifically in WithMarker(): +// +// - instead of the package path, so that different error objects in +// the same package have different markers; +// +// - instead of including the line number or function name, so that +// the marker persists across minor code refactorings; this ensures +// comparability across mixed version clusters. + +// WithMarker decorates an error with its own string marker. +// The marker is derived from the original marker to also include +// the file path of the caller of WithMarker. +func WithMarker(err error) error { + // Generate a marker. + marker := getMarker(err) + + // Decorate the marker with the caller information. + src := makeSrcCtx(1) + decoratedMarker := fmt.Sprintf("%s\n%s", marker, src.File) + + // Get the decorated pg error or build a fresh one. + newErr := WrapWithDepthf(1, err, CodeUncategorizedError, "") + // We're guaranteed an *Error object when the input is not nil. + pgErr := newErr.(*Error) + + // If there's a mark already set and it's not the one we're adding, + // refuse to create a new one. + if pgErr.OriginMarker != "" && pgErr.OriginMarker != marker { + panic(fmt.Sprintf("error already marked as %q", pgErr.OriginMarker)) + } + + // Override the original marker with the new one. + pgErr.OriginMarker = decoratedMarker + return pgErr +} + +// WithSameMarker decorated an error with the same marker as another +// error. +func WithSameMarker(err error, refErr error) error { + if err == nil || refErr == nil { + panic("cannot mark nil error or using nil reference error") + } + + var marker string + pgRefErr, ok := GetPGCause(refErr) + if ok && pgRefErr.OriginMarker != "" { + marker = pgRefErr.OriginMarker + } else { + marker = getMarker(refErr) + } + + // Get the decorated pg error or build a fresh one. + newErr := WrapWithDepthf(1, err, CodeUncategorizedError, "") + // We're guaranteed an *Error object when the input is not nil. + pgErr := newErr.(*Error) + + // Assign the marker. + pgErr.OriginMarker = marker + return pgErr +} + +// IsMarkedError tests whether a provided error has the same mark as +// the error in the second argument. +// The boolean return is the yes/no answer. +// If the errors are the same object (same reference), the result is +// true. +// If the errors are both derived from an Error object +// and they have the same, non-empty OriginMarker, the +// function returns true. +// Otherwise, it returns false. +func IsMarkedError(errToTest error, markedErr error) bool { + if errToTest == markedErr { + return true + } + if errToTest == nil || markedErr == nil { + return false + } + + lMarker := getMarker(errToTest) + rMarker := getMarker(markedErr) + + return lMarker != "" && lMarker == rMarker +} + +// getMarker returns a portable identification for the error. We +// assume that an error is identified by the full type identification +// (including package path) of the error object and its full error +// message. +func getMarker(err error) string { + err = errors.Cause(err) + + pgErr, isPgError := err.(*Error) + if isPgError && pgErr.OriginMarker != "" { + return pgErr.OriginMarker + } + + prefix := err.Error() + if isPgError { + prefix = fmt.Sprintf("(%s) %s", pgErr.Code, prefix) + } + + // We'll need the type to get the type identification below. + t := reflect.TypeOf(err) + + // We want the path of the type "inside". + origT := t + for { + switch origT.Kind() { + case reflect.Array, reflect.Chan, reflect.Map, reflect.Ptr, reflect.Slice: + origT = origT.Elem() + continue + } + break + } + + // The "natural" representation of an error would put the package + // name first. However, because we're interested in comparing these + // markers and we want to reject different errors quickly, we want + // to put the fields with the highest variability earlier in the + // marker. Hence message first, then type, then package. + return fmt.Sprintf("%s\n%s\n%s", + prefix, + t.String(), + origT.PkgPath()) +} + +// ContextCanceledMarkerError can be used as parameter to +// IsMarkedError to recognize a pgerror that wrapped a +// context.Canceled error. +// This is provided for convenience only, to avoid recomputing +// the marker on every call to IsMarkerError(). +var ContextCanceledMarkerError = Wrap(context.Canceled, "", "") + +// ContextDeadlineExceededMarkerError can be used as parameter to +// IsMarkedError to recognize a pgerror that wrapped a +// context.DeadlineExceeded error. +// This is provided for convenience only, to avoid recomputing +// the marker on every call to IsMarkerError(). +var ContextDeadlineExceededMarkerError = Wrap(context.DeadlineExceeded, "", "") + +// IOEOFMarkerError can be used as parameter to +// IsMarkedError to recognize a pgerror that wrapped a +// io.EOF error. +// This is provided for convenience only, to avoid recomputing +// the marker on every call to IsMarkerError(). +var IOEOFMarkerError = Wrap(io.EOF, "", "") diff --git a/pkg/sql/pgwire/pgerror/markers_test.go b/pkg/sql/pgwire/pgerror/markers_test.go new file mode 100644 index 000000000000..66fcc3719580 --- /dev/null +++ b/pkg/sql/pgwire/pgerror/markers_test.go @@ -0,0 +1,70 @@ +// Copyright 2019 The Cockroach Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +// implied. See the License for the specific language governing +// permissions and limitations under the License. + +package pgerror + +import ( + "context" + e1 "errors" + "io" + "regexp" + "testing" + + e2 "github.com/pkg/errors" +) + +func TestMarkerEquivalence(t *testing.T) { + testData := []struct { + expEq bool + err1 error + err2 error + expectedRe *regexp.Regexp + }{ + {true, NewError(CodeSyntaxError, "woo"), NewError(CodeSyntaxError, "woo"), regexp.MustCompile(`woo\n.*pgerror.Error`)}, + {false, NewError(CodeSyntaxError, "woo"), NewError(CodeSystemError, "woo"), nil}, + {true, Wrapf(NewError(CodeSyntaxError, "woo"), CodeSystemError, "waa"), NewError(CodeSyntaxError, "woo"), nil}, + {true, e2.Wrap(NewError(CodeSyntaxError, "woo"), "waa"), NewError(CodeSyntaxError, "woo"), nil}, + {true, e2.WithMessage(e2.New("woo"), "waa"), e2.New("woo"), nil}, + {false, e1.New("woo"), e2.New("woo"), nil}, + + {true, io.EOF, io.EOF, regexp.MustCompile(`EOF\n.*errorString`)}, + {true, io.EOF, IOEOFMarkerError, nil}, + {false, io.EOF, WithMarker(io.EOF), nil}, + {true, WithMarker(io.EOF), WithMarker(io.EOF), regexp.MustCompile(`EOF\n.*\n.*\n.*/markers_test.go`)}, + + {true, context.Canceled, context.Canceled, regexp.MustCompile(`context canceled\n.*errorString`)}, + {true, context.Canceled, ContextCanceledMarkerError, nil}, + {false, context.Canceled, WithMarker(context.Canceled), nil}, + + {true, WithSameMarker(context.Canceled, ContextCanceledMarkerError), ContextCanceledMarkerError, nil}, + {true, WithSameMarker(context.Canceled, WithMarker(context.Canceled)), WithMarker(context.Canceled), nil}, + {true, WithSameMarker(e2.New("woo"), context.Canceled), context.Canceled, nil}, + {false, WithSameMarker(e2.New("woo"), context.Canceled), WithMarker(context.Canceled), nil}, + {false, WithSameMarker(e2.New("woo"), context.Canceled), e2.New("woo"), nil}, + } + + for i, test := range testData { + m1 := getMarker(test.err1) + m2 := getMarker(test.err2) + if test.expEq && m1 != m2 { + t.Errorf("%d: unexpected marker difference:\n%s\n-- vs. --\n%s", i, m1, m2) + } else if !test.expEq && m1 == m2 { + t.Errorf("%d: unexpected marker equivalence:\n%s", i, m1) + } + + if test.expectedRe != nil && !test.expectedRe.MatchString(m1) { + t.Errorf("%d: marker does not match %q:\n%s", i, test.expectedRe, m1) + } + } +} diff --git a/pkg/sql/pgwire/pgerror/wrap.go b/pkg/sql/pgwire/pgerror/wrap.go index cf1b47a84235..602cae3417e5 100644 --- a/pkg/sql/pgwire/pgerror/wrap.go +++ b/pkg/sql/pgwire/pgerror/wrap.go @@ -128,9 +128,11 @@ func collectErrForWrap(err error, fallbackCode string) *Error { pgErr = collectErrForWrap(cause.Cause(), fallbackCode) } else { // There's no discernible cause. Build a new pgerr from scratch. - pgErr = &Error{Code: fallbackCode} - // - // For certain types of errors, we'll override the fall back code, + pgErr = &Error{ + Code: fallbackCode, + OriginMarker: getMarker(err), + } + // to ensure the error flows back up properly. // We also need to re-define the interfaces here explicitly // because we can't use `roachpb` directly (import loop) diff --git a/pkg/sql/physical_schema_accessors.go b/pkg/sql/physical_schema_accessors.go index d95e1f3ba857..aea5b7030d43 100644 --- a/pkg/sql/physical_schema_accessors.go +++ b/pkg/sql/physical_schema_accessors.go @@ -19,6 +19,7 @@ import ( "context" "github.com/cockroachdb/cockroach/pkg/internal/client" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" "github.com/cockroachdb/cockroach/pkg/util/encoding" @@ -168,7 +169,7 @@ func (a UncachedPhysicalAccessor) GetObjectDesc( // We have a descriptor. Is it in the right state? We'll keep it if // it is in the ADD state. - if err := filterTableState(desc); err == nil || err == errTableAdding { + if err := filterTableState(desc); err == nil || pgerror.IsMarkedError(err, errTableAdding) { // Immediately after a RENAME an old name still points to the // descriptor during the drain phase for the name. Do not // return a descriptor during draining. diff --git a/pkg/sql/planner.go b/pkg/sql/planner.go index b67e994c8bb4..71a3d4153f21 100644 --- a/pkg/sql/planner.go +++ b/pkg/sql/planner.go @@ -25,6 +25,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/server/serverpb" "github.com/cockroachdb/cockroach/pkg/sql/coltypes" "github.com/cockroachdb/cockroach/pkg/sql/parser" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/querycache" "github.com/cockroachdb/cockroach/pkg/sql/row" "github.com/cockroachdb/cockroach/pkg/sql/rowcontainer" @@ -401,7 +402,7 @@ func (p *planner) LookupTableByID(ctx context.Context, tableID sqlbase.ID) (row. flags := ObjectLookupFlags{CommonLookupFlags: CommonLookupFlags{avoidCached: p.avoidCachedDescriptors}} table, err := p.Tables().getTableVersionByID(ctx, p.txn, tableID, flags) if err != nil { - if err == errTableAdding { + if pgerror.IsMarkedError(err, errTableAdding) { return row.TableEntry{IsAdding: true}, nil } return row.TableEntry{}, err diff --git a/pkg/sql/row/fk_existence_base.go b/pkg/sql/row/fk_existence_base.go index d66d076cbc30..27f6e9715b97 100644 --- a/pkg/sql/row/fk_existence_base.go +++ b/pkg/sql/row/fk_existence_base.go @@ -236,4 +236,4 @@ func computeFkCheckColumnIDs( } } -var errSkipUnusedFK = errors.New("no columns involved in FK included in writer") +var errSkipUnusedFK = pgerror.WithMarker(errors.New("no columns involved in FK included in writer")) diff --git a/pkg/sql/row/fk_existence_delete.go b/pkg/sql/row/fk_existence_delete.go index 9be6865926a0..18e48d80743e 100644 --- a/pkg/sql/row/fk_existence_delete.go +++ b/pkg/sql/row/fk_existence_delete.go @@ -19,6 +19,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/internal/client" "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" ) @@ -63,7 +64,7 @@ func makeFkExistenceCheckHelperForDelete( continue } fk, err := makeFkExistenceCheckBaseHelper(txn, otherTables, idx, ref, colMap, alloc, CheckDeletes) - if err == errSkipUnusedFK { + if pgerror.IsMarkedError(err, errSkipUnusedFK) { continue } if err != nil { diff --git a/pkg/sql/row/fk_existence_insert.go b/pkg/sql/row/fk_existence_insert.go index 84bbf5d5f40d..c621e405e8fe 100644 --- a/pkg/sql/row/fk_existence_insert.go +++ b/pkg/sql/row/fk_existence_insert.go @@ -19,6 +19,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/internal/client" "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" ) @@ -67,7 +68,7 @@ func makeFkExistenceCheckHelperForInsert( for _, idx := range table.AllNonDropIndexes() { if idx.ForeignKey.IsSet() { fk, err := makeFkExistenceCheckBaseHelper(txn, otherTables, idx, idx.ForeignKey, colMap, alloc, CheckInserts) - if err == errSkipUnusedFK { + if pgerror.IsMarkedError(err, errSkipUnusedFK) { continue } if err != nil { diff --git a/pkg/sql/schema_changer.go b/pkg/sql/schema_changer.go index a3ba6d8bc3d0..fc7fc2124709 100644 --- a/pkg/sql/schema_changer.go +++ b/pkg/sql/schema_changer.go @@ -172,29 +172,24 @@ func isPermanentSchemaChangeError(err error) bool { return false } - switch err { + switch { case - context.Canceled, - context.DeadlineExceeded, - errExistingSchemaChangeLease, - errExpiredSchemaChangeLease, - errNotHitGCTTLDeadline, - errSchemaChangeDuringDrain, - errSchemaChangeNotFirstInLine: + pgerror.IsMarkedError(err, pgerror.ContextCanceledMarkerError), + pgerror.IsMarkedError(err, pgerror.ContextDeadlineExceededMarkerError), + pgerror.IsMarkedError(err, errExistingSchemaChangeLease), + pgerror.IsMarkedError(err, errExpiredSchemaChangeLease), + pgerror.IsMarkedError(err, errNotHitGCTTLDeadline), + pgerror.IsMarkedError(err, errSchemaChangeDuringDrain), + pgerror.IsMarkedError(err, errSchemaChangeNotFirstInLine), + pgerror.IsMarkedError(err, exampleTableVersionMismatchError): return false } - switch err := err.(type) { - case errTableVersionMismatch: - return false - case *pgerror.Error: - switch err.Code { - case pgerror.CodeSerializationFailureError, pgerror.CodeConnectionFailureError: + if pgErr, ok := err.(*pgerror.Error); ok { + switch { + case + pgErr.Code == pgerror.CodeSerializationFailureError, + pgErr.Code == pgerror.CodeConnectionFailureError: return false - - case pgerror.CodeInternalError: - if strings.Contains(err.Message, context.DeadlineExceeded.Error()) { - return false - } } } @@ -202,33 +197,36 @@ func isPermanentSchemaChangeError(err error) bool { } var ( - errExistingSchemaChangeLease = pgerror.NewErrorf(pgerror.CodeDataExceptionError, "an outstanding schema change lease exists") - errExpiredSchemaChangeLease = pgerror.NewErrorf(pgerror.CodeDataExceptionError, "the schema change lease has expired") - errSchemaChangeNotFirstInLine = pgerror.NewErrorf(pgerror.CodeDataExceptionError, "schema change not first in line") - errNotHitGCTTLDeadline = pgerror.NewErrorf(pgerror.CodeDataExceptionError, "not hit gc ttl deadline") - errSchemaChangeDuringDrain = pgerror.NewErrorf(pgerror.CodeDataExceptionError, "a schema change ran during the drain phase, re-increment") + errExistingSchemaChangeLease = pgerror.WithMarker( + pgerror.NewErrorf(pgerror.CodeDataExceptionError, "an outstanding schema change lease exists")) + errExpiredSchemaChangeLease = pgerror.WithMarker( + pgerror.NewErrorf(pgerror.CodeDataExceptionError, "the schema change lease has expired")) + errSchemaChangeNotFirstInLine = pgerror.WithMarker( + pgerror.NewErrorf(pgerror.CodeDataExceptionError, "schema change not first in line")) + errNotHitGCTTLDeadline = pgerror.WithMarker( + pgerror.NewErrorf(pgerror.CodeDataExceptionError, "not hit gc ttl deadline")) + errSchemaChangeDuringDrain = pgerror.WithMarker( + pgerror.NewErrorf(pgerror.CodeDataExceptionError, "a schema change ran during the drain phase, re-increment")) ) func shouldLogSchemaChangeError(err error) bool { - return err != errExistingSchemaChangeLease && - err != errSchemaChangeNotFirstInLine && - err != errNotHitGCTTLDeadline + return !pgerror.IsMarkedError(err, errExistingSchemaChangeLease) && + !pgerror.IsMarkedError(err, errSchemaChangeNotFirstInLine) && + !pgerror.IsMarkedError(err, errNotHitGCTTLDeadline) } -type errTableVersionMismatch struct { - version sqlbase.DescriptorVersion - expected sqlbase.DescriptorVersion -} +var exampleTableVersionMismatchError = pgerror.WithMarker(&errTableVersionMismatch{}) -func makeErrTableVersionMismatch(version, expected sqlbase.DescriptorVersion) error { - return errors.WithStack(errTableVersionMismatch{ - version: version, - expected: expected, - }) -} +type errTableVersionMismatch struct{} -func (e errTableVersionMismatch) Error() string { - return fmt.Sprintf("table version mismatch: %d, expected: %d", e.version, e.expected) +func (*errTableVersionMismatch) Error() string { return "" } + +func makeErrTableVersionMismatch(version, expected sqlbase.DescriptorVersion) error { + return pgerror.WithSameMarker( + pgerror.NewErrorf(pgerror.CodeObjectNotInPrerequisiteStateError, + "table version mismatch: %d, expected: %d", + version, expected), + exampleTableVersionMismatchError) } // AcquireLease acquires a schema change lease on the table if @@ -1592,7 +1590,7 @@ func (s *SchemaChangeManager) Start(stopper *stop.Stopper) { if shouldLogSchemaChangeError(err) { log.Warningf(ctx, "Error executing schema change: %s", err) } - if err == sqlbase.ErrDescriptorNotFound { + if pgerror.IsMarkedError(err, sqlbase.ErrDescriptorNotFound) { // Someone deleted this table. Don't try to run the schema // changer again. Note that there's no gossip update for the // deletion which would remove this schemaChanger. diff --git a/pkg/sql/schema_changer_test.go b/pkg/sql/schema_changer_test.go index d66e63c9dac9..2d481f086188 100644 --- a/pkg/sql/schema_changer_test.go +++ b/pkg/sql/schema_changer_test.go @@ -37,6 +37,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/distsqlrun" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" "github.com/cockroachdb/cockroach/pkg/sql/stats" @@ -3133,7 +3134,7 @@ CREATE TABLE t.test (k INT PRIMARY KEY, v INT, pi DECIMAL REFERENCES t.pi (d) DE _, err = sqlbase.GetTableDescFromID(ctx, txn, tableDesc.ID) return err }); err != nil { - if err == sqlbase.ErrDescriptorNotFound { + if pgerror.IsMarkedError(err, sqlbase.ErrDescriptorNotFound) { return nil } return err diff --git a/pkg/sql/set_zone_config.go b/pkg/sql/set_zone_config.go index 5b602b8cf14e..ebf19516aa13 100644 --- a/pkg/sql/set_zone_config.go +++ b/pkg/sql/set_zone_config.go @@ -304,7 +304,7 @@ func (n *setZoneConfigNode) startExec(params runParams) error { _, completeZone, completeSubzone, err := GetZoneConfigInTxn(params.ctx, params.p.txn, uint32(targetID), index, partition, n.setDefault) - if err == errNoZoneConfigApplies { + if pgerror.IsMarkedError(err, errNoZoneConfigApplies) { // No zone config yet. // // GetZoneConfigInTxn will fail with errNoZoneConfigApplies when diff --git a/pkg/sql/show_zone_config.go b/pkg/sql/show_zone_config.go index 1754f5bfcd46..1425f38a5b6f 100644 --- a/pkg/sql/show_zone_config.go +++ b/pkg/sql/show_zone_config.go @@ -22,6 +22,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/config" "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/sql/lex" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sem/types" "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" @@ -107,7 +108,7 @@ func (n *showZoneConfigNode) startExec(params runParams) error { zoneID, zone, subzone, err := GetZoneConfigInTxn(params.ctx, params.p.txn, uint32(targetID), index, partition, false /* getInheritedDefault */) - if err == errNoZoneConfigApplies { + if pgerror.IsMarkedError(err, errNoZoneConfigApplies) { // TODO(benesch): This shouldn't be the caller's responsibility; // GetZoneConfigInTxn should just return the default zone config if no zone // config applies. diff --git a/pkg/sql/sqlbase/structured.go b/pkg/sql/sqlbase/structured.go index bd8ba01cb457..7f301bd151f8 100644 --- a/pkg/sql/sqlbase/structured.go +++ b/pkg/sql/sqlbase/structured.go @@ -176,12 +176,12 @@ func (dir IndexDescriptor_Direction) ToEncodingDirection() (encoding.Direction, // ErrDescriptorNotFound is returned by GetTableDescFromID to signal that a // descriptor could not be found with the given id. -var ErrDescriptorNotFound = errors.New("descriptor not found") +var ErrDescriptorNotFound = pgerror.WithMarker(errors.New("descriptor not found")) // ErrIndexGCMutationsList is returned by FindIndexByID to signal that the // index with the given ID does not have a descriptor and is in the garbage // collected mutations list. -var ErrIndexGCMutationsList = errors.New("index in GC mutations list") +var ErrIndexGCMutationsList = pgerror.WithMarker(errors.New("index in GC mutations list")) // NewMutableCreatedTableDescriptor returns a MutableTableDescriptor from the // given TableDescriptor with the cluster version being the zero table. This diff --git a/pkg/sql/table.go b/pkg/sql/table.go index 8e7a93b5a64a..1d53488ed85b 100644 --- a/pkg/sql/table.go +++ b/pkg/sql/table.go @@ -108,8 +108,8 @@ func (p *planner) getVirtualTabler() VirtualTabler { return p.extendedEvalCtx.VirtualSchemas } -var errTableDropped = errors.New("table is being dropped") -var errTableAdding = errors.New("table is being added") +var errTableDropped = pgerror.WithMarker(errors.New("table is being dropped")) +var errTableAdding = pgerror.WithMarker(errors.New("table is being added")) func filterTableState(tableDesc *sqlbase.TableDescriptor) error { switch { @@ -336,7 +336,7 @@ func (tc *TableCollection) getTableVersion( // Read the descriptor from the store in the face of some specific errors // because of a known limitation of AcquireByName. See the known // limitations of AcquireByName for details. - if err == errTableDropped || err == sqlbase.ErrDescriptorNotFound { + if pgerror.IsMarkedError(err, errTableDropped) || pgerror.IsMarkedError(err, sqlbase.ErrDescriptorNotFound) { return readTableFromStore() } // Lease acquisition failed with some other error. This we don't @@ -400,7 +400,7 @@ func (tc *TableCollection) getTableVersionByID( origTimestamp := txn.OrigTimestamp() table, expiration, err := tc.leaseMgr.Acquire(ctx, origTimestamp, tableID) if err != nil { - if err == sqlbase.ErrDescriptorNotFound { + if pgerror.IsMarkedError(err, sqlbase.ErrDescriptorNotFound) { // Transform the descriptor error into an error that references the // table's ID. return nil, sqlbase.NewUndefinedRelationError( @@ -607,7 +607,7 @@ func (tc *TableCollection) getUncommittedTable( if mutTbl.Name == string(tn.TableName) && mutTbl.ParentID == dbID { // Right state? - if err = filterTableState(mutTbl.TableDesc()); err != nil && err != errTableAdding { + if err = filterTableState(mutTbl.TableDesc()); err != nil && !pgerror.IsMarkedError(err, errTableAdding) { if !required { // If it's not required here, we simply say we don't have it. err = nil diff --git a/pkg/sql/zone_config.go b/pkg/sql/zone_config.go index 98d80678890e..71f521fd14a5 100644 --- a/pkg/sql/zone_config.go +++ b/pkg/sql/zone_config.go @@ -35,7 +35,7 @@ func init() { config.ZoneConfigHook = ZoneConfigHook } -var errNoZoneConfigApplies = errors.New("no zone config applies") +var errNoZoneConfigApplies = pgerror.WithMarker(errors.New("no zone config applies")) // getZoneConfig recursively looks up entries in system.zones until an // entry that applies to the object with the specified id is @@ -158,7 +158,7 @@ func ZoneConfigHook( } zoneID, zone, _, placeholder, err := getZoneConfig( id, getKey, false /* getInheritedDefault */) - if err == errNoZoneConfigApplies { + if pgerror.IsMarkedError(err, errNoZoneConfigApplies) { return nil, nil, true, nil } else if err != nil { return nil, nil, false, err @@ -272,12 +272,13 @@ func (p *planner) resolveTableForZone( return res, err } +var errMissingKey = pgerror.WithMarker(errors.New("missing key")) + // resolveZone resolves a zone specifier to a zone ID. If the zone // specifier points to a table, index or partition, the table part // must be properly normalized already. It is the caller's // responsibility to do this using e.g .resolveTableForZone(). func resolveZone(ctx context.Context, txn *client.Txn, zs *tree.ZoneSpecifier) (sqlbase.ID, error) { - errMissingKey := errors.New("missing key") id, err := config.ResolveZoneSpecifier(zs, func(parentID uint32, name string) (uint32, error) { kv, err := txn.Get(ctx, sqlbase.MakeNameMetadataKey(sqlbase.ID(parentID), name)) @@ -295,7 +296,7 @@ func resolveZone(ctx context.Context, txn *client.Txn, zs *tree.ZoneSpecifier) ( }, ) if err != nil { - if err == errMissingKey { + if pgerror.IsMarkedError(err, errMissingKey) { return 0, zoneSpecifierNotFoundError(*zs) } return 0, err