From 0440cb1c0801ea0f455f69ae32f717fd843fdba9 Mon Sep 17 00:00:00 2001
From: Faizan Qazi <faizan@cockroachlabs.com>
Date: Thu, 23 Mar 2023 16:37:52 -0400
Subject: [PATCH] sql/schemachanger: block legacy schema changer operations
 earlier

Previously, it was possible for ADD/DROP CONSTRAINT and
other declarative schema changer operations to collide with
DROP TABLE in the legacy schema changer, since our gate
was not early enough. So, some work would be done before
the schema change was blocked with a retry error.
This patch prevents DROP TABLE/VIEW/SEQUENCE from
executing until a declarative schema change on such
objects is completed earlier.

Fixes: #99379

Release note: None
---
 pkg/sql/drop_sequence.go |  7 +++++++
 pkg/sql/drop_table.go    | 14 ++++++--------
 pkg/sql/drop_view.go     |  7 +++++++
 3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/pkg/sql/drop_sequence.go b/pkg/sql/drop_sequence.go
index 8f2caf3efab7..0d13f54316fc 100644
--- a/pkg/sql/drop_sequence.go
+++ b/pkg/sql/drop_sequence.go
@@ -21,6 +21,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
+	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scerrors"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
 	"github.com/cockroachdb/cockroach/pkg/util/iterutil"
@@ -83,6 +84,12 @@ func (n *dropSequenceNode) startExec(params runParams) error {
 	ctx := params.ctx
 	for _, toDel := range n.td {
 		droppedDesc := toDel.desc
+		// Exit early with an error if the table is undergoing a declarative schema
+		// change, before we try to get job IDs and update job statuses later. See
+		// createOrUpdateSchemaChangeJob.
+		if catalog.HasConcurrentDeclarativeSchemaChange(droppedDesc) {
+			return scerrors.ConcurrentSchemaChangeError(droppedDesc)
+		}
 		err := params.p.dropSequenceImpl(
 			ctx, droppedDesc, true /* queueJob */, tree.AsStringWithFQNames(n.n, params.Ann()), n.n.DropBehavior,
 		)
diff --git a/pkg/sql/drop_table.go b/pkg/sql/drop_table.go
index 59c1464134d8..6b64678edeca 100644
--- a/pkg/sql/drop_table.go
+++ b/pkg/sql/drop_table.go
@@ -229,7 +229,12 @@ func (p *planner) dropTableImpl(
 	behavior tree.DropBehavior,
 ) ([]string, error) {
 	var droppedViews []string
-
+	// Exit early with an error if the table is undergoing a declarative schema
+	// change, before we try to get job IDs and update job statuses later. See
+	// createOrUpdateSchemaChangeJob.
+	if catalog.HasConcurrentDeclarativeSchemaChange(tableDesc) {
+		return nil, scerrors.ConcurrentSchemaChangeError(tableDesc)
+	}
 	// Remove foreign key back references from tables that this table has foreign
 	// keys to.
 	// Copy out the set of outbound fks as it may be overwritten in the loop.
@@ -353,13 +358,6 @@ func (p *planner) initiateDropTable(
 		return errors.Errorf("table %q is already being dropped", tableDesc.Name)
 	}
 
-	// Exit early with an error if the table is undergoing a declarative schema
-	// change, before we try to get job IDs and update job statuses later. See
-	// createOrUpdateSchemaChangeJob.
-	if catalog.HasConcurrentDeclarativeSchemaChange(tableDesc) {
-		return scerrors.ConcurrentSchemaChangeError(tableDesc)
-	}
-
 	// Use the delayed GC mechanism to schedule usage of the more efficient
 	// ClearRange pathway.
 	if tableDesc.IsTable() {
diff --git a/pkg/sql/drop_view.go b/pkg/sql/drop_view.go
index dd0ea192ab4a..8146bd68aec6 100644
--- a/pkg/sql/drop_view.go
+++ b/pkg/sql/drop_view.go
@@ -20,6 +20,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/funcdesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
+	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scerrors"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -236,6 +237,12 @@ func (p *planner) dropViewImpl(
 ) ([]string, error) {
 	var cascadeDroppedViews []string
 
+	// Exit early with an error if the table is undergoing a declarative schema
+	// change, before we try to get job IDs and update job statuses later. See
+	// createOrUpdateSchemaChangeJob.
+	if catalog.HasConcurrentDeclarativeSchemaChange(viewDesc) {
+		return nil, scerrors.ConcurrentSchemaChangeError(viewDesc)
+	}
 	// Remove back-references from the tables/views this view depends on.
 	dependedOn := append([]descpb.ID(nil), viewDesc.DependsOn...)
 	for _, depID := range dependedOn {