Merge pull request cockroachdb#10216 from a-robinson/errors

sql: Improve dependent object errors

pkg/sql/alter_table.go

@@ -192,12 +192,13 @@ func (n *alterTableNode) Start() error {
 				if !found {
 					continue
 				}
-				err := n.p.canRemoveDependentViewGeneric("column", string(t.Column), ref, t.DropBehavior)
+				err := n.p.canRemoveDependentViewGeneric(
+					"column", string(t.Column), n.tableDesc.ParentID, ref, t.DropBehavior)
 				if err != nil {
 					return err
 				}
-				viewDesc, err :=
-					n.p.getViewDescForCascade("column", string(t.Column), ref.ID, t.DropBehavior)
+				viewDesc, err := n.p.getViewDescForCascade(
+					"column", string(t.Column), n.tableDesc.ParentID, ref.ID, t.DropBehavior)
 				if err != nil {
 					return err
 				}
@@ -244,13 +245,13 @@ func (n *alterTableNode) Start() error {
 				if t.IfExists {
 					continue
 				}
-				return errors.Errorf("constraint %q does not exist", t.Constraint)
+				return fmt.Errorf("constraint %q does not exist", t.Constraint)
 			}
 			switch details.Kind {
 			case sqlbase.ConstraintTypePK:
-				return errors.Errorf("cannot drop primary key")
+				return fmt.Errorf("cannot drop primary key")
 			case sqlbase.ConstraintTypeUnique:
-				return errors.Errorf("UNIQUE constraint depends on index %q, use DROP INDEX if you really want to drop it", t.Constraint)
+				return fmt.Errorf("UNIQUE constraint depends on index %q, use DROP INDEX if you really want to drop it", t.Constraint)
 			case sqlbase.ConstraintTypeCheck:
 				for i := range n.tableDesc.Checks {
 					if n.tableDesc.Checks[i].Name == name {
@@ -282,7 +283,7 @@ func (n *alterTableNode) Start() error {
 			name := string(t.Constraint)
 			constraint, ok := info[name]
 			if !ok {
-				return errors.Errorf("constraint %q does not exist", t.Constraint)
+				return fmt.Errorf("constraint %q does not exist", t.Constraint)
 			}
 			if !constraint.Unvalidated {
 				continue

pkg/sql/create.go

@@ -59,7 +59,7 @@ func (p *planner) CreateDatabase(n *parser.CreateDatabase) (planNode, error) {
 	}
 
 	if p.session.User != security.RootUser {
-		return nil, errors.Errorf("only %s is allowed to create databases", security.RootUser)
+		return nil, fmt.Errorf("only %s is allowed to create databases", security.RootUser)
 	}
 
 	return &createDatabaseNode{p: p, n: n}, nil
@@ -663,7 +663,7 @@ func (p *planner) resolveFK(
 	}
 
 	if len(targetCols) != len(srcCols) {
-		return errors.Errorf("%d columns must reference exactly %d columns in referenced table (found %d)",
+		return fmt.Errorf("%d columns must reference exactly %d columns in referenced table (found %d)",
 			len(srcCols), len(srcCols), len(targetCols))
 	}
 
@@ -925,7 +925,7 @@ func (n *createViewNode) makeViewTableDesc(
 
 	// TODO(a-robinson): Support star expressions as soon as we can (#10028).
 	if planContainsStar(n.sourcePlan) {
-		return desc, errors.New("views do not currently support * expressions")
+		return desc, fmt.Errorf("views do not currently support * expressions")
 	}
 
 	n.resolveViewDependencies(&desc, affected)
@@ -1148,7 +1148,8 @@ func (p *planner) makeTableDesc(
 		if idx.ForeignKey.IsSet() {
 			for i := range idx.ColumnIDs {
 				if _, ok := colsInFKs[idx.ColumnIDs[i]]; ok {
-					return desc, errors.Errorf("column %q cannot be used by multiple foreign key constraints", idx.ColumnNames[i])
+					return desc, fmt.Errorf(
+						"column %q cannot be used by multiple foreign key constraints", idx.ColumnNames[i])
 				}
 				colsInFKs[idx.ColumnIDs[i]] = struct{}{}
 			}

pkg/sql/drop.go

@@ -325,12 +325,13 @@ func (n *dropIndexNode) Start() error {
 			for _, tableRef := range tableDesc.DependedOnBy {
 				if tableRef.IndexID == idx.ID {
 					// Ensure that we have DROP privilege on all dependent views
-					err := n.p.canRemoveDependentViewGeneric("index", idx.Name, tableRef, n.n.DropBehavior)
+					err := n.p.canRemoveDependentViewGeneric(
+						"index", idx.Name, tableDesc.ParentID, tableRef, n.n.DropBehavior)
 					if err != nil {
 						return err
 					}
-					viewDesc, err :=
-						n.p.getViewDescForCascade("index", idx.Name, tableRef.ID, n.n.DropBehavior)
+					viewDesc, err := n.p.getViewDescForCascade(
+						"index", idx.Name, tableDesc.ParentID, tableRef.ID, n.n.DropBehavior)
 					if err != nil {
 						return err
 					}
@@ -628,16 +629,17 @@ func (p *planner) canRemoveDependentView(
 	ref sqlbase.TableDescriptor_Reference,
 	behavior parser.DropBehavior,
 ) error {
-	return p.canRemoveDependentViewGeneric(from.TypeName(), from.Name, ref, behavior)
+	return p.canRemoveDependentViewGeneric(from.TypeName(), from.Name, from.ParentID, ref, behavior)
 }
 
 func (p *planner) canRemoveDependentViewGeneric(
 	typeName string,
 	objName string,
+	parentID sqlbase.ID,
 	ref sqlbase.TableDescriptor_Reference,
 	behavior parser.DropBehavior,
 ) error {
-	viewDesc, err := p.getViewDescForCascade(typeName, objName, ref.ID, behavior)
+	viewDesc, err := p.getViewDescForCascade(typeName, objName, parentID, ref.ID, behavior)
 	if err != nil {
 		return err
 	}
@@ -798,8 +800,8 @@ func (p *planner) dropTableImpl(tableDesc *sqlbase.TableDescriptor) ([]string, e
 	// Drop all views that depend on this table, assuming that we wouldn't have
 	// made it to this point if `cascade` wasn't enabled.
 	for _, ref := range tableDesc.DependedOnBy {
-		viewDesc, err :=
-			p.getViewDescForCascade(tableDesc.TypeName(), tableDesc.Name, ref.ID, parser.DropCascade)
+		viewDesc, err := p.getViewDescForCascade(
+			tableDesc.TypeName(), tableDesc.Name, tableDesc.ParentID, ref.ID, parser.DropCascade)
 		if err != nil {
 			return droppedViews, err
 		}
@@ -920,8 +922,8 @@ func (p *planner) dropViewImpl(
 
 	if behavior == parser.DropCascade {
 		for _, ref := range viewDesc.DependedOnBy {
-			dependentDesc, err :=
-				p.getViewDescForCascade(viewDesc.TypeName(), viewDesc.Name, ref.ID, behavior)
+			dependentDesc, err := p.getViewDescForCascade(
+				viewDesc.TypeName(), viewDesc.Name, viewDesc.ParentID, ref.ID, behavior)
 			if err != nil {
 				return cascadeDroppedViews, err
 			}
@@ -988,20 +990,25 @@ func removeMatchingReferences(
 }
 
 func (p *planner) getViewDescForCascade(
-	typeName string, objName string, viewID sqlbase.ID, behavior parser.DropBehavior,
+	typeName string, objName string, parentID, viewID sqlbase.ID, behavior parser.DropBehavior,
 ) (*sqlbase.TableDescriptor, error) {
 	viewDesc, err := sqlbase.GetTableDescFromID(p.txn, viewID)
 	if err != nil {
 		log.Warningf(p.ctx(), "unable to retrieve descriptor for view %d: %v", viewID, err)
 		return nil, errors.Wrapf(err, "error resolving dependent view ID %d", viewID)
 	}
 	if behavior != parser.DropCascade {
-		viewName, err := p.getQualifiedTableName(viewDesc)
-		if err != nil {
-			log.Warningf(p.ctx(), "unable to retrieve qualified name of view %d: %v", viewID, err)
-			return nil, errors.Wrapf(err, "cannot drop %s %q because it is depended on by a view")
+		viewName := viewDesc.Name
+		if viewDesc.ParentID != parentID {
+			var err error
+			viewName, err = p.getQualifiedTableName(viewDesc)
+			if err != nil {
+				log.Warningf(p.ctx(), "unable to retrieve qualified name of view %d: %v", viewID, err)
+				return nil, sqlbase.NewDependentObjectError(
+					"cannot drop %s %q because a view depends on it", typeName, objName)
+			}
 		}
-		return nil, errors.Errorf("cannot drop %s %q because it is depended on by view %q",
+		return nil, sqlbase.NewDependentObjectError("cannot drop %s %q because view %q depends on it",
 			typeName, objName, viewName)
 	}
 	return viewDesc, nil

pkg/sql/rename.go

@@ -79,15 +79,23 @@ func (p *planner) RenameDatabase(n *parser.RenameDatabase) (planNode, error) {
 			continue
 		}
 		if len(tbDesc.DependedOnBy) > 0 {
-			viewName, err := p.getQualifiedTableNameFromID(tbDesc.DependedOnBy[0].ID)
+			viewDesc, err := sqlbase.GetTableDescFromID(p.txn, tbDesc.DependedOnBy[0].ID)
 			if err != nil {
-				log.Warningf(p.ctx(), "Unable to retrieve name of view %d: %v",
-					tbDesc.DependedOnBy[0].ID, err)
-				return nil, errors.Errorf(
-					"cannot rename database because table %q is depended on by a view", tbDesc.Name)
+				return nil, err
 			}
-			return nil, errors.Errorf("cannot rename database because table %q is depended on by view %q",
-				tbDesc.Name, viewName)
+			viewName := viewDesc.Name
+			if dbDesc.ID != viewDesc.ParentID {
+				var err error
+				viewName, err = p.getQualifiedTableName(viewDesc)
+				if err != nil {
+					log.Warningf(p.ctx(), "Unable to retrieve fully-qualified name of view %d: %v",
+						viewDesc.ID, err)
+					return nil, sqlbase.NewDependentObjectError(
+						"cannot rename database because a view depends on table %q", tbDesc.Name)
+				}
+			}
+			return nil, sqlbase.NewDependentObjectError(
+				"cannot rename database because view %q depends on table %q", viewName, tbDesc.Name)
 		}
 	}
 
@@ -167,7 +175,7 @@ func (p *planner) RenameTable(n *parser.RenameTable) (planNode, error) {
 	// query with the new name, we simply disallow such renames for now.
 	if len(tableDesc.DependedOnBy) > 0 {
 		return nil, p.dependentViewRenameError(
-			tableDesc.TypeName(), oldTn.String(), tableDesc.DependedOnBy[0].ID)
+			tableDesc.TypeName(), oldTn.String(), tableDesc.ParentID, tableDesc.DependedOnBy[0].ID)
 	}
 
 	// Check if target database exists.
@@ -272,7 +280,8 @@ func (p *planner) RenameIndex(n *parser.RenameIndex) (planNode, error) {
 		if tableRef.IndexID != tableDesc.Indexes[i].ID {
 			continue
 		}
-		return nil, p.dependentViewRenameError("index", n.Index.Index.String(), tableRef.ID)
+		return nil, p.dependentViewRenameError(
+			"index", n.Index.Index.String(), tableDesc.ParentID, tableRef.ID)
 	}
 
 	if n.NewName == "" {
@@ -362,7 +371,8 @@ func (p *planner) RenameColumn(n *parser.RenameColumn) (planNode, error) {
 			}
 		}
 		if found {
-			return nil, p.dependentViewRenameError("column", n.Name.String(), tableRef.ID)
+			return nil, p.dependentViewRenameError(
+				"column", n.Name.String(), tableDesc.ParentID, tableRef.ID)
 		}
 	}
 
@@ -429,13 +439,23 @@ func (p *planner) RenameColumn(n *parser.RenameColumn) (planNode, error) {
 
 // TODO(a-robinson): Support renaming objects depended on by views once we have
 // a better encoding for view queries (#10083).
-func (p *planner) dependentViewRenameError(typeName, objName string, viewID sqlbase.ID) error {
-	viewName, err := p.getQualifiedTableNameFromID(viewID)
+func (p *planner) dependentViewRenameError(
+	typeName, objName string, parentID, viewID sqlbase.ID,
+) error {
+	viewDesc, err := sqlbase.GetTableDescFromID(p.txn, viewID)
 	if err != nil {
-		log.Warningf(p.ctx(), "unable to retrieve name of view %d: %v", viewID, err)
-		return errors.Errorf("cannot rename %s %q because it is depended on by a view",
-			typeName, objName)
+		return err
+	}
+	viewName := viewDesc.Name
+	if viewDesc.ParentID != parentID {
+		var err error
+		viewName, err = p.getQualifiedTableName(viewDesc)
+		if err != nil {
+			log.Warningf(p.ctx(), "unable to retrieve name of view %d: %v", viewID, err)
+			return sqlbase.NewDependentObjectError("cannot rename %s %q because a view depends on it",
+				typeName, objName)
+		}
 	}
-	return errors.Errorf("cannot rename %s %q because it is depended on by view %q",
+	return sqlbase.NewDependentObjectError("cannot rename %s %q because view %q depends on it",
 		typeName, objName, viewName)
 }

pkg/sql/sqlbase/errors.go

@@ -60,6 +60,7 @@ var _ ErrorWithPGCode = &ErrDatabaseAlreadyExists{}
 var _ ErrorWithPGCode = &ErrRelationAlreadyExists{}
 var _ ErrorWithPGCode = &ErrWrongObjectType{}
 var _ ErrorWithPGCode = &ErrSyntax{}
+var _ ErrorWithPGCode = &ErrDependentObject{}
 
 const (
 	txnAbortedMsg = "current transaction is aborted, commands ignored " +
@@ -378,6 +379,31 @@ func (e *ErrSyntax) SrcContext() SrcCtx {
 	return e.ctx
 }
 
+// NewDependentObjectError creates a new ErrDependentObject.
+func NewDependentObjectError(format string, args ...interface{}) error {
+	return &ErrDependentObject{ctx: MakeSrcCtx(1), msg: fmt.Sprintf(format, args...)}
+}
+
+// ErrDependentObject represents a dependent object error.
+type ErrDependentObject struct {
+	ctx SrcCtx
+	msg string
+}
+
+func (e *ErrDependentObject) Error() string {
+	return e.msg
+}
+
+// Code implements the ErrorWithPGCode interface.
+func (*ErrDependentObject) Code() string {
+	return pgerror.CodeDependentObjectsStillExistError
+}
+
+// SrcContext implements the ErrorWithPGCode interface.
+func (e *ErrDependentObject) SrcContext() SrcCtx {
+	return e.ctx
+}
+
 // IsIntegrityConstraintError returns true if the error is some kind of SQL
 // constraint violation.
 func IsIntegrityConstraintError(err error) bool {

pkg/sql/table.go

@@ -518,16 +518,6 @@ func (p *planner) databaseFromSearchPath(tn *parser.TableName) (string, error) {
 	return p.session.Database, nil
 }
 
-// getQualifiedTableNameFromID returns the database-qualified name of the table
-// or view with the provided ID.
-func (p *planner) getQualifiedTableNameFromID(id sqlbase.ID) (string, error) {
-	desc, err := sqlbase.GetTableDescFromID(p.txn, id)
-	if err != nil {
-		return "", err
-	}
-	return p.getQualifiedTableName(desc)
-}
-
 // getQualifiedTableName returns the database-qualified name of the table
 // or view represented by the provided descriptor.
 func (p *planner) getQualifiedTableName(desc *sqlbase.TableDescriptor) (string, error) {

pkg/sql/testdata/alter_table

@@ -283,13 +283,13 @@ ALTER TABLE t ADD COLUMN e INT
 statement ok
 CREATE VIEW v AS SELECT x, y FROM t WHERE e > 5
 
-statement error cannot drop column "x" because it is depended on by view "test.v"
+statement error cannot drop column "x" because view "v" depends on it
 ALTER TABLE t DROP COLUMN x
 
-statement error cannot drop column "y" because it is depended on by view "test.v"
+statement error cannot drop column "y" because view "v" depends on it
 ALTER TABLE t DROP COLUMN y
 
-statement error cannot drop column "e" because it is depended on by view "test.v"
+statement error cannot drop column "e" because view "v" depends on it
 ALTER TABLE t DROP COLUMN e
 
 statement ok
@@ -307,7 +307,7 @@ CREATE VIEW v AS SELECT x, y FROM t WHERE e > 5
 statement ok
 ALTER TABLE t DROP COLUMN IF EXISTS q
 
-statement error cannot drop column "e" because it is depended on by view "test.v"
+statement error cannot drop column "e" because view "v" depends on it
 ALTER TABLE t DROP COLUMN IF EXISTS e
 
 statement ok

pkg/sql/testdata/drop_index

@@ -113,7 +113,7 @@ users bar     false  1   title  ASC       false
 statement ok
 CREATE VIEW v AS SELECT name FROM users@{FORCE_INDEX=foo}
 
-statement error cannot drop index "foo" because it is depended on by view "test.v"
+statement error cannot drop index "foo" because view "v" depends on it
 DROP INDEX users@foo
 
 statement ok