sql/schemachanger: enable drop index by default #92971

fqazi · 2022-12-03T02:43:07Z

This patch will enable drop index by default in the declarative schema changer with the following fixes:

Return proper index names for errors
Some index notices should only be generated when work is done
Views and indexes should have to be cleaned together when the index becomes invisible.
Drop index should return correct errors in system tables
Enable drop index by default and update logic tests

Current limitations for falling back are if there are foreign key constraint dependencies or zone configs

cockroach-teamcity · 2022-12-03T02:43:19Z

This change is

ajwerner

Well that was satisfyingly small. What's the deal with dropping hash-sharded indexes? How about expression-based indexes? Do we have tests? Another thing that's harder to test but we'll want to test is dropping old-style hash sharded indexes with the stored column.

pkg/sql/opt/exec/execbuilder/testdata/show_trace

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

ajwerner · 2022-12-06T14:46:23Z

pkg/sql/schemachanger/scplan/testdata/drop_index

@@ -625,28 +625,17 @@ DROP INDEX idx3 CASCADE
 ops
 DROP INDEX idx4 CASCADE
 ----
-StatementPhase stage 1 of 1 with 3 MutationType ops
+StatementPhase stage 1 of 1 with 1 MutationType op


This one is interesting. I think ideal the index would go to validated and that would be "synthetic" so to speak. I think this is fine for now, but let's not forget about it.

fqazi · 2022-12-06T15:16:31Z

Well that was satisfyingly small. What's the deal with dropping hash-sharded indexes? How about expression-based indexes? Do we have tests? Another thing that's harder to test but we'll want to test is dropping old-style hash sharded indexes with the stored column.

Good point, let me add end-to-end coverage for this stuff. We have basic sanity tests for hash-sharded and expression-based indexes. But we need more coverage in those areas. Also, let me dig into legacy hash shared indexes, those might be a blind spot. Think another spot is zone config-related side effects, let me sort those out as well (fallback temporarily).

postamar

CI is not happy with this branch.

edit: I forgot to mention, do we have test cases in which an index in a materialized view is dropped? For example

CREATE TABLE kv (k INT PRIMARY KEY, v STRING); 
CREATE MATERIALIZED VIEW v1 AS SELECT k, v FROM kv;   
CREATE INDEX idx ON v1(v);
CREATE MATERIALIZED VIEW v2 AS SELECT v FROM v1@idx;
DROP INDEX v1@idx CASCADE;

postamar · 2022-12-07T14:30:41Z

pkg/sql/schemachanger/scbuild/builder_state.go

+			schemaDesc := b.descCache[tableDesc.GetParentSchemaID()].desc
+			panic(catalog.NewMutableAccessToVirtualSchemaError(schemaDesc.(catalog.SchemaDescriptor)))
+		}
+	}


This may be out of scope for this change but naively I'd expect this check to be covered by our existing privilege checks. What's the story here?

The builder never asks for mutable references to these descriptors so there isn't any way to trigger the errors via the normal code path. We could hit them on execution, but I think that's not the right approach for errors like these @postamar .

postamar · 2022-12-07T14:32:11Z

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

 //   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`.
 //   3. Check if requires CCL binary for eventual zone config removal.


Zone configs are not a CCL feature, though. I'm curious what this is about. @Xiang-Gu do you remember, by any chance? I realize that it's been a while!

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

postamar · 2022-12-07T14:33:01Z

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

+		dropped := dropAnIndex(b, index, n.IfExists, n.DropBehavior)
+		if dropped {
+			anyIndexesDropped = true
+		}


nit: replace this with anyIndexesDropped = dropAnIndex(b, index, n.IfExists, n.DropBehavior)

It's possible that if multiple indexes are specified one of them won't be dropped. Is the following cleaner:

anyIndexesDropped = anyIndexesDropped || dropAnIndex(b, index, n.IfExists, n.DropBehavior)

Actually that's a terrible idea it will get optimized out, we need an if statement.

it can be anyIndexesDropped = dropAnIndex(b, index, n.IfExists, n.DropBehavior) || anyIndexesDropped but that's probably too clever. I'm cool with this. If you did feel the need to do anything more clever:

didDrop := dropAnIndex(/* ... *./) anyIndexesDropped = anyIndexesDropped || didDrop

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

postamar · 2022-12-07T14:41:43Z

pkg/sql/schemachanger/scplan/internal/rules/dep_drop_index.go

+
+func init() {
+	registerDepRuleForDrop(
+		"view can only go to absent when the index becomes hidden",


Can you reformulate this rule name in a similar style as the others? Otherwise when we have to look at the corpus of rules it can become very confusing very quickly.

The from element is usually referred to before to, as in "'from' is in whatever state (before/right before) 'to' is in whatever state".

Good point, I reworked this into 3 different rules:

index no longer public before index name => This ensures the name is only cleaned up after visibility is not there

dependent view no longer public before secondary index => This ensures that views enter dropped before the index disappears

dependent view absent before secondary index=> This guarantees that views are completely dropped before the dependent secondary index

secondary index should be validated before dependent view can be absent => Make sure that the view can't be dropped before the index reaches validated

pkg/sql/schemachanger/scplan/internal/rules/dep_drop_index.go

postamar · 2022-12-07T14:43:52Z

pkg/sql/schemachanger/scplan/internal/rules/dep_drop_index.go

+		"view can only go to absent when the index becomes hidden",
+		scgraph.SameStagePrecedence,
+		"index", "dependent",
+		scpb.Status_VALIDATED, scpb.Status_ABSENT,


Curious: why ABSENT and not DROPPED? I wonder whether, in fact, there shouldn't be two rules, perhaps:

One which forces the view to reach DROPPED before the index is no longer public? Otherwise, the view will be broken.

Another which constrains the index's ABSENT state relative to the view? The index can't be ABSENT before the view, right? Otherwise the view's back-references will be invalid?

This should be dropped, I reworked the rules to have saner ordering as suggested.

These rules might be very inefficient. Can you file an issue to model the references in the rules engine so that it can be accelerated? In particular, I think this will make drop database cascade for cases where there are many indexes and views O(views * indexes).

Previously, some of the errors generate by the declarative schema changer errors included both the table and index names. This was incorrect, since the schema changer errors should only include the index name. To address this, this patch updates these error paths to only have the index name. Release note: None

…tables Previously, the DROP INDEX command in the declarative schema changer did not generate the correct errors for system tables. To address this, this patch modifies the index resolution to generate the correct errors. Release note: None

ajwerner

Something about the rules related to index columns moving to absent smells to me but that's not changed by this. LGTM

ajwerner · 2022-12-15T18:33:33Z

pkg/sql/schemachanger/scplan/internal/rules/dep_drop_index.go

+		"view can only go to absent when the index becomes hidden",
+		scgraph.SameStagePrecedence,
+		"index", "dependent",
+		scpb.Status_VALIDATED, scpb.Status_ABSENT,


These rules might be very inefficient. Can you file an issue to model the references in the rules engine so that it can be accelerated? In particular, I think this will make drop database cascade for cases where there are many indexes and views O(views * indexes).

ajwerner · 2022-12-15T18:35:27Z

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

 //   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


nit: this can be 2. now

ajwerner · 2022-12-15T18:37:54Z

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

+		dropped := dropAnIndex(b, index, n.IfExists, n.DropBehavior)
+		if dropped {
+			anyIndexesDropped = true
+		}


it can be anyIndexesDropped = dropAnIndex(b, index, n.IfExists, n.DropBehavior) || anyIndexesDropped but that's probably too clever. I'm cool with this. If you did feel the need to do anything more clever:

didDrop := dropAnIndex(/* ... *./) anyIndexesDropped = anyIndexesDropped || didDrop

Previously, the declarative schema changer notices were always generated even if no indexes were dropped. This was inadequate because some of these should have been reported if any work was done. To address this, this patch will detect if any indexes were actually dropped. Release note: None

Previously, during dropped indexes and views that depended on them did not have any proper dependency order enforced. This could lead to improper event log entries / wrong transactional behaviour. This patch, ensures that views are cleaned once the index becomes invisible. Release note: None

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

fqazi

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @postamar)

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_index.go line 197 at r5 (raw file):

Previously, ajwerner wrote…

Debugging code

Done.

pkg/sql/schemachanger/scplan/internal/rules/dep_drop_index.go line 114 at r8 (raw file):

Previously, ajwerner wrote…

These rules might be very inefficient. Can you file an issue to model the references in the rules engine so that it can be accelerated? In particular, I think this will make drop database cascade for cases where there are many indexes and views O(views * indexes).

Opened: #93730 and added a TODO

fqazi · 2022-12-15T19:27:34Z

@ajwerner @postamar TFTR!

fqazi · 2022-12-15T20:19:22Z

bors r+

craig · 2022-12-15T23:59:33Z

Build succeeded:

Bazel Essential CI (Cockroach)

fqazi force-pushed the enableDropIdx branch 6 times, most recently from 1d3593a to d121a43 Compare December 6, 2022 01:43

fqazi marked this pull request as ready for review December 6, 2022 01:43

fqazi requested review from a team December 6, 2022 01:43

fqazi requested a review from a team as a code owner December 6, 2022 01:43

ajwerner reviewed Dec 6, 2022

View reviewed changes

fqazi force-pushed the enableDropIdx branch from d121a43 to 679620f Compare December 6, 2022 15:39

postamar reviewed Dec 7, 2022

View reviewed changes

fqazi force-pushed the enableDropIdx branch 9 times, most recently from 9b8b878 to da60f72 Compare December 9, 2022 04:57

fqazi requested review from ajwerner and postamar December 9, 2022 14:19

fqazi force-pushed the enableDropIdx branch from da60f72 to b93cc2d Compare December 9, 2022 14:57

fqazi added 2 commits December 14, 2022 09:51

fqazi force-pushed the enableDropIdx branch from b93cc2d to 5d3bd45 Compare December 14, 2022 15:43

ajwerner approved these changes Dec 15, 2022

View reviewed changes

fqazi added 3 commits December 15, 2022 14:22

fqazi force-pushed the enableDropIdx branch from 5d3bd45 to ffc9c57 Compare December 15, 2022 19:23

fqazi commented Dec 15, 2022

View reviewed changes

craig bot merged commit 2c2822c into cockroachdb:master Dec 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql/schemachanger: enable drop index by default #92971

sql/schemachanger: enable drop index by default #92971

fqazi commented Dec 3, 2022 •

edited

Loading

cockroach-teamcity commented Dec 3, 2022

ajwerner left a comment

ajwerner Dec 6, 2022

fqazi commented Dec 6, 2022

postamar left a comment •

edited

Loading

postamar Dec 7, 2022

fqazi Dec 8, 2022

postamar Dec 7, 2022

postamar Dec 7, 2022

fqazi Dec 8, 2022

fqazi Dec 9, 2022

ajwerner Dec 15, 2022

postamar Dec 7, 2022

fqazi Dec 9, 2022 •

edited

Loading

fqazi Dec 9, 2022

postamar Dec 7, 2022

fqazi Dec 9, 2022

ajwerner Dec 15, 2022

ajwerner left a comment

ajwerner Dec 15, 2022

ajwerner Dec 15, 2022

ajwerner Dec 15, 2022

fqazi left a comment

fqazi commented Dec 15, 2022

fqazi commented Dec 15, 2022

craig bot commented Dec 15, 2022

sql/schemachanger: enable drop index by default #92971

sql/schemachanger: enable drop index by default #92971

Conversation

fqazi commented Dec 3, 2022 • edited Loading

cockroach-teamcity commented Dec 3, 2022

ajwerner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fqazi commented Dec 6, 2022

postamar left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fqazi Dec 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fqazi left a comment

Choose a reason for hiding this comment

fqazi commented Dec 15, 2022

fqazi commented Dec 15, 2022

craig bot commented Dec 15, 2022

fqazi commented Dec 3, 2022 •

edited

Loading

postamar left a comment •

edited

Loading

fqazi Dec 9, 2022 •

edited

Loading