Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: don't test error identities using reference equality #35920

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/sql/crdb_internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/exec_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
24 changes: 13 additions & 11 deletions pkg/sql/lease.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/opt_catalog.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 4 additions & 2 deletions pkg/sql/pgwire/pgerror/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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) {
Expand Down
107 changes: 76 additions & 31 deletions pkg/sql/pgwire/pgerror/errors.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions pkg/sql/pgwire/pgerror/errors.proto
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Loading