sql/schemachanger: enable declarative drop index by default

Previously, DROP INDEX was only supported in the legacy
schema changer. This patch will enable it by default for
the declarative schema changer and update logic tests.

This support is still disabled if we encounter:
1) Zone configs
2) Foreign key constraint dependencies

Release note: None

pkg/sql/logictest/testdata/logic_test/alter_table

@@ -217,6 +217,7 @@ ALTER TABLE t DROP CONSTRAINT foo
 statement ok
 DROP INDEX foo CASCADE
 
+onlyif config local-legacy-schema-changer
 query TTTTRT retry
 SELECT job_type, description, user_name, status, fraction_completed, error
 FROM crdb_internal.jobs
@@ -227,6 +228,21 @@ LIMIT 2
 SCHEMA CHANGE GC  GC for DROP INDEX test.public.t@foo CASCADE  root  running    0  ·
 SCHEMA CHANGE     DROP INDEX test.public.t@foo CASCADE         root  succeeded  1  ·
 
+
+skipif config local-legacy-schema-changer
+skipif config local-mixed-22.2-23.1
+query TTTTRT retry
+SELECT job_type, description, user_name, status, fraction_completed, error
+FROM crdb_internal.jobs
+WHERE job_type = 'NEW SCHEMA CHANGE' OR job_type = 'SCHEMA CHANGE GC'
+ORDER BY created DESC
+LIMIT 2
+----
+SCHEMA CHANGE GC   GC for DROP INDEX test.public.t@foo CASCADE  node  running    0  ·
+NEW SCHEMA CHANGE  DROP INDEX test.public.t@foo CASCADE         root  succeeded  1  ·
+
+
+
 query TTBITTBBB colnames
 SHOW INDEXES FROM t
 ----
@@ -302,6 +318,7 @@ INSERT INTO t (a, d, x, y, z) VALUES (33, 34, DECIMAL '2.0', DECIMAL '2.1', DECI
 statement ok
 DROP INDEX t@t_f_idx
 
+onlyif config local-legacy-schema-changer
 query TTTTRT retry
 SELECT job_type, description, user_name, status, fraction_completed, error
 FROM crdb_internal.jobs
@@ -312,6 +329,20 @@ LIMIT 2
 SCHEMA CHANGE GC  GC for DROP INDEX test.public.t@t_f_idx  root  running    0  ·
 SCHEMA CHANGE     DROP INDEX test.public.t@t_f_idx         root  succeeded  1  ·
 
+skipif config local-legacy-schema-changer
+skipif config local-mixed-22.2-23.1
+query TTTTRT retry
+SELECT job_type, description, user_name, status, fraction_completed, error
+FROM crdb_internal.jobs
+WHERE job_type = 'NEW SCHEMA CHANGE' OR job_type = 'SCHEMA CHANGE GC'
+ORDER BY created DESC
+LIMIT 2
+----
+SCHEMA CHANGE GC   GC for DROP INDEX test.public.t@t_f_idx  node  running    0  ·
+NEW SCHEMA CHANGE  DROP INDEX test.public.t@t_f_idx         root  succeeded  1  ·
+
+
+
 statement ok
 ALTER TABLE t DROP COLUMN f
 
@@ -2073,6 +2104,7 @@ statement ok
 ALTER TABLE t ADD CONSTRAINT t_pkey PRIMARY KEY (id)
 
 skipif config local-legacy-schema-changer
+skipif config local-mixed-22.2-23.1
 query TT
 SHOW CREATE TABLE t
 ----

pkg/sql/logictest/testdata/logic_test/create_index

@@ -193,8 +193,6 @@ query T noticetrace
 DROP INDEX CONCURRENTLY IF EXISTS create_index_concurrently_idx
 ----
 NOTICE: CONCURRENTLY is not required as all indexes are dropped concurrently
-NOTICE: the data for dropped indexes is reclaimed asynchronously
-HINT: The reclamation delay can be customized in the zone configuration for the table.
 
 query TT
 SHOW CREATE TABLE create_index_concurrently_tbl

pkg/sql/logictest/testdata/logic_test/drop_index

@@ -101,6 +101,12 @@ users       users_pkey  false       3             title        N/A        true
 
 user testuser
 
+skipif config local-legacy-schema-changer
+skipif config local-mixed-22.2-23.1
+statement error must be owner of table users or have CREATE privilege on table users
+DROP INDEX users@bar
+
+onlyif config local-legacy-schema-changer
 statement error user testuser does not have CREATE privilege on relation users
 DROP INDEX users@bar
 

pkg/sql/logictest/testdata/logic_test/new_schema_changer

@@ -1393,7 +1393,7 @@ CREATE VIEW child AS SELECT count(*) FROM parent@idx
 statement ok
 SET use_declarative_schema_changer = 'unsafe'
 
-statement error pgcode 2BP01 cannot drop index "parent@idx" because view "child" depends on it
+statement error pgcode 2BP01 cannot drop index "idx" because view "child" depends on it
 DROP INDEX parent@idx
 
 statement ok

pkg/sql/logictest/testdata/logic_test/privileges_comments

@@ -39,9 +39,14 @@ skipif config local-legacy-schema-changer
 statement error must be owner of table t45707 or have CREATE privilege on table t45707
 COMMENT ON COLUMN d45707.t45707.x IS 'x45707'
 
+onlyif config local-legacy-schema-changer
 statement error user testuser does not have CREATE privilege on relation t45707
 COMMENT ON INDEX d45707.t45707@t45707_pkey IS 'p45707'
 
+skipif config local-legacy-schema-changer
+statement error must be owner of table t45707 or have CREATE privilege on table t45707
+COMMENT ON INDEX d45707.t45707@t45707_pkey IS 'p45707'
+
 # Verify that the user can view the comments
 
 query T

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_index.go

@@ -25,26 +25,13 @@ import (
 	"github.com/cockroachdb/errors"
 )
 
-// TODO (xiang): To enable full support for DROP INDEX, we still need to
+// TODO (xiang): To remove fallbacks for DROP INDEX we still need to:
 //   1. Model adding and dropping FK constraints correctly
 //      for dropping an index with dependent FK constraint;
-//   2. Fix the "revertibility bug" in the planner as it currently
-//      seems to ignore non-revertible operations and plan
-//      them to happen in a much early phase (say, stmt phase);
-//   2.a. Once the above two are done, add a test where
-//        we drop an index with a dependent FK constraint and a
-//        dependent view to end-to-end test, scbuild test, scplan test.
-//   2.b. Answer the question "Are there any CCL required `DROP INDEX` usage?"
-//        If yes, add a end-to-end test in `pkg/ccl/schemachangerccl`.
+//   2. Once the above two are done, add a test where
+//      we drop an index with a dependent FK constraint and a
+//      dependent view to end-to-end test, scbuild test, scplan test.
 //   3. Check if requires CCL binary for eventual zone config removal.
-//   4. Make all existing unit tests and logic tests pass using this
-//      new `DROP INDEX` implementation. This will likely involve
-//      rewriting expected output since there are the legacy and declarative
-//      schema changer exhibits minor behavior difference. Ideally, we
-//      should modify the tests in a way such that if we turn off the
-//      declarative schema changer for `DROP INDEX`, the legacy schema
-//      changer's `DROP INDEX` will be able to pass all the tests as well.
-//   5. Set `DROP INDEX` to be fully-supported in `process.go`.
 
 // DropIndex implements DROP INDEX.
 // It resolves the to-be-dropped index into elements and inform `BuildCtx`

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/process.go

@@ -102,8 +102,7 @@ var supportedStatements = map[reflect.Type]supportedStatement{
 	reflect.TypeOf((*tree.CommentOnColumn)(nil)):     {fn: CommentOnColumn, on: true, minSupportedClusterVersion: clusterversion.V22_2Start},
 	reflect.TypeOf((*tree.CommentOnIndex)(nil)):      {fn: CommentOnIndex, on: true, minSupportedClusterVersion: clusterversion.V22_2Start},
 	reflect.TypeOf((*tree.CommentOnConstraint)(nil)): {fn: CommentOnConstraint, on: true, minSupportedClusterVersion: clusterversion.V22_2Start},
-	// TODO (Xiang): turn on `DROP INDEX` as fully supported.
-	reflect.TypeOf((*tree.DropIndex)(nil)): {fn: DropIndex, on: false, minSupportedClusterVersion: clusterversion.V22_2Start},
+	reflect.TypeOf((*tree.DropIndex)(nil)):           {fn: DropIndex, on: true, minSupportedClusterVersion: clusterversion.V23_1Start},
 }
 
 func init() {

pkg/sql/schemachanger/scbuild/testdata/drop_index

@@ -8,6 +8,10 @@ CREATE TABLE t1(i INT, j STRING);
 CREATE INDEX idx1 ON t1(i);
 CREATE INDEX idx2 ON t1(lower(j)) WHERE i > 0;
 CREATE INDEX idx3 ON t1(i) USING HASH;
+CREATE VIEW v AS SELECT count(j) FROM t1@idx3;
+CREATE MATERIALIZED VIEW v2 AS SELECT i, j FROM t1;
+CREATE INDEX idx ON v2(j);
+CREATE MATERIALIZED VIEW v3 AS SELECT j FROM v2@idx
 ----
 
 build
@@ -71,3 +75,99 @@ DROP INDEX idx3 CASCADE
   {columnIds: [5], constraintId: 2, expr: 'crdb_internal_i_shard_16 IN (0:::INT8, 1:::INT8, 2:::INT8, 3:::INT8, 4:::INT8, 5:::INT8, 6:::INT8, 7:::INT8, 8:::INT8, 9:::INT8, 10:::INT8, 11:::INT8, 12:::INT8, 13:::INT8, 14:::INT8, 15:::INT8)', fromHashShardedColumn: true, referencedColumnIds: [5], tableId: 104}
 - [[ConstraintWithoutIndexName:{DescID: 104, Name: check_crdb_internal_i_shard_16, ConstraintID: 2}, ABSENT], PUBLIC]
   {constraintId: 2, name: check_crdb_internal_i_shard_16, tableId: 104}
+- [[Namespace:{DescID: 105, Name: v, ReferencedDescID: 100}, ABSENT], PUBLIC]
+  {databaseId: 100, descriptorId: 105, name: v, schemaId: 101}
+- [[Owner:{DescID: 105}, ABSENT], PUBLIC]
+  {descriptorId: 105, owner: root}
+- [[UserPrivileges:{DescID: 105, Name: admin}, ABSENT], PUBLIC]
+  {descriptorId: 105, privileges: 2, userName: admin}
+- [[UserPrivileges:{DescID: 105, Name: root}, ABSENT], PUBLIC]
+  {descriptorId: 105, privileges: 2, userName: root}
+- [[View:{DescID: 105}, ABSENT], PUBLIC]
+  {forwardReferences: [{columnIds: [2], indexId: 6, toId: 104}], usesRelationIds: [104], viewId: 105}
+- [[ObjectParent:{DescID: 105, ReferencedDescID: 101}, ABSENT], PUBLIC]
+  {objectId: 105, parentSchemaId: 101}
+- [[Column:{DescID: 105, ColumnID: 1}, ABSENT], PUBLIC]
+  {columnId: 1, pgAttributeNum: 1, tableId: 105}
+- [[ColumnName:{DescID: 105, Name: count, ColumnID: 1}, ABSENT], PUBLIC]
+  {columnId: 1, name: count, tableId: 105}
+- [[ColumnType:{DescID: 105, ColumnFamilyID: 0, ColumnID: 1}, ABSENT], PUBLIC]
+  {columnId: 1, isNullable: true, tableId: 105, type: {family: IntFamily, oid: 20, width: 64}}
+- [[Column:{DescID: 105, ColumnID: 4294967295}, ABSENT], PUBLIC]
+  {columnId: 4.294967295e+09, isHidden: true, isSystemColumn: true, pgAttributeNum: 4.294967295e+09, tableId: 105}
+- [[ColumnName:{DescID: 105, Name: crdb_internal_mvcc_timestamp, ColumnID: 4294967295}, ABSENT], PUBLIC]
+  {columnId: 4.294967295e+09, name: crdb_internal_mvcc_timestamp, tableId: 105}
+- [[ColumnType:{DescID: 105, ColumnFamilyID: 0, ColumnID: 4294967295}, ABSENT], PUBLIC]
+  {columnId: 4.294967295e+09, isNullable: true, tableId: 105, type: {family: DecimalFamily, oid: 1700}}
+- [[Column:{DescID: 105, ColumnID: 4294967294}, ABSENT], PUBLIC]
+  {columnId: 4.294967294e+09, isHidden: true, isSystemColumn: true, pgAttributeNum: 4.294967294e+09, tableId: 105}
+- [[ColumnName:{DescID: 105, Name: tableoid, ColumnID: 4294967294}, ABSENT], PUBLIC]
+  {columnId: 4.294967294e+09, name: tableoid, tableId: 105}
+- [[ColumnType:{DescID: 105, ColumnFamilyID: 0, ColumnID: 4294967294}, ABSENT], PUBLIC]
+  {columnId: 4.294967294e+09, isNullable: true, tableId: 105, type: {family: OidFamily, oid: 26}}
+
+build
+DROP INDEX v2@idx CASCADE
+----
+- [[IndexColumn:{DescID: 106, ColumnID: 2, IndexID: 2}, ABSENT], PUBLIC]
+  {columnId: 2, indexId: 2, tableId: 106}
+- [[IndexColumn:{DescID: 106, ColumnID: 3, IndexID: 2}, ABSENT], PUBLIC]
+  {columnId: 3, indexId: 2, kind: KEY_SUFFIX, tableId: 106}
+- [[SecondaryIndex:{DescID: 106, IndexID: 2, ConstraintID: 0}, ABSENT], PUBLIC]
+  {indexId: 2, isCreatedExplicitly: true, tableId: 106}
+- [[IndexName:{DescID: 106, Name: idx, IndexID: 2}, ABSENT], PUBLIC]
+  {indexId: 2, name: idx, tableId: 106}
+- [[IndexData:{DescID: 106, IndexID: 2}, ABSENT], PUBLIC]
+  {indexId: 2, tableId: 106}
+- [[Namespace:{DescID: 107, Name: v3, ReferencedDescID: 100}, ABSENT], PUBLIC]
+  {databaseId: 100, descriptorId: 107, name: v3, schemaId: 101}
+- [[Owner:{DescID: 107}, ABSENT], PUBLIC]
+  {descriptorId: 107, owner: root}
+- [[UserPrivileges:{DescID: 107, Name: admin}, ABSENT], PUBLIC]
+  {descriptorId: 107, privileges: 2, userName: admin}
+- [[UserPrivileges:{DescID: 107, Name: root}, ABSENT], PUBLIC]
+  {descriptorId: 107, privileges: 2, userName: root}
+- [[View:{DescID: 107}, ABSENT], PUBLIC]
+  {forwardReferences: [{columnIds: [2], indexId: 2, toId: 106}], isMaterialized: true, usesRelationIds: [106], viewId: 107}
+- [[ObjectParent:{DescID: 107, ReferencedDescID: 101}, ABSENT], PUBLIC]
+  {objectId: 107, parentSchemaId: 101}
+- [[ColumnFamily:{DescID: 107, Name: primary, ColumnFamilyID: 0}, ABSENT], PUBLIC]
+  {name: primary, tableId: 107}
+- [[Column:{DescID: 107, ColumnID: 1}, ABSENT], PUBLIC]
+  {columnId: 1, pgAttributeNum: 1, tableId: 107}
+- [[ColumnName:{DescID: 107, Name: j, ColumnID: 1}, ABSENT], PUBLIC]
+  {columnId: 1, name: j, tableId: 107}
+- [[ColumnType:{DescID: 107, ColumnFamilyID: 0, ColumnID: 1}, ABSENT], PUBLIC]
+  {columnId: 1, isNullable: true, tableId: 107, type: {family: StringFamily, oid: 25}}
+- [[Column:{DescID: 107, ColumnID: 2}, ABSENT], PUBLIC]
+  {columnId: 2, isHidden: true, pgAttributeNum: 2, tableId: 107}
+- [[ColumnName:{DescID: 107, Name: rowid, ColumnID: 2}, ABSENT], PUBLIC]
+  {columnId: 2, name: rowid, tableId: 107}
+- [[ColumnType:{DescID: 107, ColumnFamilyID: 0, ColumnID: 2}, ABSENT], PUBLIC]
+  {columnId: 2, tableId: 107, type: {family: IntFamily, oid: 20, width: 64}}
+- [[ColumnDefaultExpression:{DescID: 107, ColumnID: 2}, ABSENT], PUBLIC]
+  {columnId: 2, expr: unique_rowid(), tableId: 107}
+- [[Column:{DescID: 107, ColumnID: 4294967295}, ABSENT], PUBLIC]
+  {columnId: 4.294967295e+09, isHidden: true, isSystemColumn: true, pgAttributeNum: 4.294967295e+09, tableId: 107}
+- [[ColumnName:{DescID: 107, Name: crdb_internal_mvcc_timestamp, ColumnID: 4294967295}, ABSENT], PUBLIC]
+  {columnId: 4.294967295e+09, name: crdb_internal_mvcc_timestamp, tableId: 107}
+- [[ColumnType:{DescID: 107, ColumnFamilyID: 0, ColumnID: 4294967295}, ABSENT], PUBLIC]
+  {columnId: 4.294967295e+09, isNullable: true, tableId: 107, type: {family: DecimalFamily, oid: 1700}}
+- [[Column:{DescID: 107, ColumnID: 4294967294}, ABSENT], PUBLIC]
+  {columnId: 4.294967294e+09, isHidden: true, isSystemColumn: true, pgAttributeNum: 4.294967294e+09, tableId: 107}
+- [[ColumnName:{DescID: 107, Name: tableoid, ColumnID: 4294967294}, ABSENT], PUBLIC]
+  {columnId: 4.294967294e+09, name: tableoid, tableId: 107}
+- [[ColumnType:{DescID: 107, ColumnFamilyID: 0, ColumnID: 4294967294}, ABSENT], PUBLIC]
+  {columnId: 4.294967294e+09, isNullable: true, tableId: 107, type: {family: OidFamily, oid: 26}}
+- [[IndexColumn:{DescID: 107, ColumnID: 2, IndexID: 1}, ABSENT], PUBLIC]
+  {columnId: 2, indexId: 1, tableId: 107}
+- [[IndexColumn:{DescID: 107, ColumnID: 1, IndexID: 1}, ABSENT], PUBLIC]
+  {columnId: 1, indexId: 1, kind: STORED, tableId: 107}
+- [[PrimaryIndex:{DescID: 107, IndexID: 1, ConstraintID: 1}, ABSENT], PUBLIC]
+  {constraintId: 1, indexId: 1, isUnique: true, tableId: 107}
+- [[IndexName:{DescID: 107, Name: v3_pkey, IndexID: 1}, ABSENT], PUBLIC]
+  {indexId: 1, name: v3_pkey, tableId: 107}
+- [[IndexData:{DescID: 107, IndexID: 1}, ABSENT], PUBLIC]
+  {indexId: 1, tableId: 107}
+- [[TableData:{DescID: 107, ReferencedDescID: 100}, ABSENT], PUBLIC]
+  {databaseId: 100, tableId: 107}