sql: add support for create index inside new schema by fqazi · Pull Request #72610 · cockroachdb/cockroach

fqazi · 2021-11-10T14:41:15Z

Previously, create index was not supported inside the
new schema changer. This was inadequate because to
help support transaction schema changes in the future
we need all schema changes inside the new schema changer.
To address this, this patch adds CREATE INDEX inside
the new schema changer.

Release note: None

cockroach-teamcity · 2021-11-10T14:41:27Z

This change is

ajwerner · 2021-11-10T16:49:30Z

pkg/sql/schemachanger/scgraph/graph.go

+		nodeDepEdgesFrom: map[*scpb.Node]*btree.BTree{},
+		nodeDepEdgesTo:   map[*scpb.Node]*btree.BTree{},


my suggestion was that you get rid of the map altogether. You can iterate the prefix of the tree corresponding to this node.

postamar

I should have reviewed this the first time around, but better late than never. Thanks for doing this, I definitely appreciate the effort this must have involved.

I didn't look deeply into the scplan stuff because I mostly don't understand it deeply.

I did find a few things which seemed questionable to me and worth changing before merging. Fixing them should mostly be about moving stuff around so hopefully no big deal. Nits are things I just couldn't help noticing, feel free to disregard them if addressing them is a waste of your time.

postamar · 2021-11-10T17:24:41Z

pkg/ccl/logictestccl/testdata/logic_test/new_schema_changer

+statement ok
+CREATE INDEX id1 on  defaultdb.t1(id, name) storing (money) PARTITION BY LIST (id) (
+                                                              PARTITION p1 VALUES IN (1)
+                                                            );


postamar · 2021-11-10T17:37:58Z

pkg/sql/backfill.go

+		settings:  settings,
+		ieFactory: ieFactory,
+	}
+}


Shouldn't you be defining indexValidator in scdeps instead? That way you don't need to add the index validator to the exec cfg. It just seems simpler? Perhaps I'm missing something.

postamar · 2021-11-10T17:38:38Z

pkg/sql/schema_changer_ccl.go

+		settings:    settings,
+		evalContext: evalContext,
+	}
+}


Same comment as indexValidator really, I'm not sure why this all isn't in scdeps.

postamar · 2021-11-10T17:40:55Z

pkg/sql/schemachanger/scbuild/index.go

+	}
+	res := tree.Serialize(modBuckets(hashedColumnsExpr()))
+	return &res
+}


Why did this need to be moved here? Is it because of a dependency circularity issue? If not I'd be inclined to leave it where it was.

postamar · 2021-11-10T17:48:48Z

pkg/sql/schemachanger/scbuild/index.go

+	}
+
+	if table.IsView() && !table.MaterializedView() {
+		panic(pgerror.Newf(pgcode.WrongObjectType, "%q is not a table or materialized view", &n.Table))


This seems wrong, I believe you should be testing for !table.IsPhysicalTable() || table.IsSequence() and modify the error message accordingly.

postamar · 2021-11-10T17:51:01Z

pkg/sql/schemachanger/scbuild/table.go

-			Index:        *idx,
-			PrimaryIndex: newPrimaryIdxID,
-		})
+	// Computed columns do not exist inside the primary index,


nit: I believe you mean Virtual computed columns?

postamar · 2021-11-10T17:53:40Z

pkg/sql/schemachanger/scdeps/exec_deps.go

+	}
+	tabledesc.UpdateIndexPartitioning(indexDesc, false, newImplicitCols, newPartitioning)
+	return nil
+}


Why is this defined as a method on txnDeps? Isn't the whole point of having d.partitioner.CreatePartitioning no dependencies would be required?

postamar · 2021-11-10T17:54:14Z

pkg/sql/schemachanger/scdeps/sctestdeps/test_deps.go

 }

+// AddPartitioning implements the scmutationexec.CatalogReader interface.
+func (s *TestState) AddPartitioning(


Same comment as for txnDeps.

postamar · 2021-11-10T17:56:18Z

pkg/sql/schemachanger/scexec/exec_validation.go

+			log.Errorf(ctx, "not implemented")
+		default:
+			panic("unimplemented")
+		}


nit: the logic in this switch case deserves to be refactored into functions, one per case.

postamar · 2021-11-10T17:59:28Z

pkg/sql/schemachanger/scexec/scmutationexec/scmutationexec.go

+		if err != nil {
+			return err
+		}
+		storeColNames = append(storeColNames, column.GetName())


nit: define a private function to map a column ID to a name perhaps? As we grow the schema changer, patterns like these are going to keep emerging, and we should take care that this particular source file doesn't turn into an unwieldy mess.

fqazi

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

pkg/ccl/logictestccl/testdata/logic_test/new_schema_changer, line 12 at r4 (raw file):

Previously, postamar (Marius Posta) wrote…

formatting

Done.

pkg/sql/backfill.go, line 1658 at r4 (raw file):

Previously, postamar (Marius Posta) wrote…

Shouldn't you be defining indexValidator in scdeps instead? That way you don't need to add the index validator to the exec cfg. It just seems simpler? Perhaps I'm missing something.

Done.

pkg/sql/schemachanger/scbuild/index.go, line 87 at r4 (raw file):

Previously, postamar (Marius Posta) wrote…

Why did this need to be moved here? Is it because of a dependency circularity issue? If not I'd be inclined to leave it where it was.

Yes, this was a circular dependency issue.

pkg/sql/schemachanger/scbuild/index.go, line 172 at r4 (raw file):

Previously, postamar (Marius Posta) wrote…

This seems wrong, I believe you should be testing for !table.IsPhysicalTable() || table.IsSequence() and modify the error message accordingly.

The error was taken from regular CREATE INDEX should we do the same there?

pkg/sql/schemachanger/scbuild/index.go, line 176 at r4 (raw file):

Previously, postamar (Marius Posta) wrote…

nit: combine the two ifs?

The second condition below also relies on it being materialized.

pkg/sql/schemachanger/scdeps/exec_deps.go, line 153 at r4 (raw file):

Previously, postamar (Marius Posta) wrote…

Why is this defined as a method on txnDeps? Isn't the whole point of having d.partitioner.CreatePartitioning no dependencies would be required?

Done.

pkg/sql/schemachanger/scdeps/sctestdeps/test_deps.go, line 453 at r4 (raw file):

Previously, postamar (Marius Posta) wrote…

Same comment as for txnDeps.

Done.

pkg/sql/schemachanger/scgraph/graph.go, line 84 at r2 (raw file):

Previously, ajwerner wrote…

my suggestion was that you get rid of the map altogether. You can iterate the prefix of the tree corresponding to this node.

Done.

pkg/sql/schema_changer_ccl.go, line 60 at r4 (raw file):

Previously, postamar (Marius Posta) wrote…

Same comment as indexValidator really, I'm not sure why this all isn't in scdeps.

Done.

ajwerner · 2021-11-10T22:44:20Z

pkg/sql/schemachanger/scgraph/graph.go

+	visit = func(n *scpb.Node) {
+		permanent, marked := marks[n]
+		if marked && !permanent {
+			panic("not a dag")


Thoughts on catching this panic and converting it to an error and then having this method return an error?

ajwerner · 2021-11-10T22:45:00Z

pkg/sql/schemachanger/scop/mutation.go

@@ -28,7 +31,19 @@ type MakeAddedIndexDeleteOnly struct {
 	TableID descpb.ID

 	// Index represents the index as it should appear in the mutation.


This comment is now stale

ajwerner · 2021-11-10T22:45:15Z

pkg/sql/schemachanger/scop/mutation.go

 	TableID descpb.ID

-	// Index is the descriptor as it should be added as part of the mutation. The
+	// IndexID is the descriptor as it should be added as part of the mutation. The


Comment is now stale

ajwerner · 2021-11-10T22:47:25Z

pkg/sql/schemachanger/scplan/opgen/opgen_secondary_index.go

+				if this.Unique {
+					return &scop.ValidateUniqueIndex{
+						TableID: this.TableID,
+						IndexID: this.IndexId,
+					}
+				}
+				return &scop.NoOpInfo{}


My preference is that we remove this optimization. The main reason is that today we validate all indexes, unique or otherwise. My secondary reason is that this NoOp smells a bit. Lastly, I think we'll want to represent not doing it elsewhere.

ajwerner

, let's do it

Reviewed 1 of 60 files at r5, 3 of 33 files at r6, 3 of 52 files at r7, 1 of 8 files at r10, 6 of 33 files at r11, 25 of 54 files at r12, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @fqazi, @postamar, and @rhu713)

Previously, the new schema changer attempted to use a bubble sort which compared the nodes linked to ops to generate an approximate order. Unfortunately, this approach had its limitations when it came to elements that had no edge linking them, which could lead to incorrect order relative to dependency edges. To address this, this patch moves the sorting to a simpler topological sort which guarantees dependency order. Release note: None

Fixes: cockroachdb#72562, cockroachdb#72561 Previously, when we updated the sorting inside the new schema changer to a rank based approach we incorrectly assumed that DepEdges will always be generated in a deterministic fashion. This does not happen because the database and rule matching is not fully determentistic. Unfortunately, this turned ouut to be false, which can lead to ranks for neighbour edges to be unstable. To address this, this patch iterates over DepEdges in a ordered fashion based on the To nodes, fixing intermittent failures inside the new schema changer tests. Release note: None

Fixes: cockroachdb#67264 Previously, create index was not supported inside the new shcema changer. This was inadequate because, to help support transaction schema changes in the future we need all schema changes inside the new schema changer. To address this, this patch adds CREATE INDEX inside the new schema changer. Release note: None

Previously, the unique index validation inside the new schema changer was stubbed out. This was inadequate because during index addition operations we were not properly confirming the unique constraints were properly implemented. To address this, this patch adds support in the new schema changer for the valdiation logic. Release note: None

fqazi · 2021-11-11T22:14:31Z

@ajwerner TFTR!

fqazi · 2021-11-11T22:14:35Z

bors r+

craig · 2021-11-12T00:42:21Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2021-11-12T03:55:18Z

Build succeeded:

GitHub CI (Cockroach)

fqazi requested review from a team and ajwerner November 10, 2021 14:41

fqazi requested a review from a team as a code owner November 10, 2021 14:41

fqazi requested review from msbutler and removed request for a team November 10, 2021 14:41

fqazi force-pushed the createTableNew branch 3 times, most recently from 8ab4ddc to 931a93c Compare November 10, 2021 15:48

ajwerner reviewed Nov 10, 2021

View reviewed changes

postamar suggested changes Nov 10, 2021

View reviewed changes

fqazi force-pushed the createTableNew branch from 931a93c to 19ff70c Compare November 10, 2021 18:07

msbutler requested review from rhu713 and removed request for msbutler November 10, 2021 18:10

fqazi force-pushed the createTableNew branch from 19ff70c to 9df97f0 Compare November 10, 2021 20:49

fqazi requested a review from a team as a code owner November 10, 2021 20:49

fqazi commented Nov 10, 2021

View reviewed changes

fqazi requested a review from ajwerner November 10, 2021 20:58

fqazi force-pushed the createTableNew branch from 9df97f0 to 8d73102 Compare November 10, 2021 20:58

fqazi requested a review from postamar November 10, 2021 20:58

ajwerner reviewed Nov 10, 2021

View reviewed changes

fqazi force-pushed the createTableNew branch 3 times, most recently from 31079ec to 7317f74 Compare November 11, 2021 04:27

fqazi requested a review from ajwerner November 11, 2021 05:13

fqazi force-pushed the createTableNew branch 4 times, most recently from 7e9ff10 to c28b27a Compare November 11, 2021 18:16

fqazi force-pushed the createTableNew branch from c28b27a to b086829 Compare November 11, 2021 20:35

ajwerner approved these changes Nov 11, 2021

View reviewed changes

fqazi added 4 commits November 11, 2021 17:06

fqazi force-pushed the createTableNew branch from b086829 to c98800d Compare November 11, 2021 22:14

craig bot merged commit 960e61e into cockroachdb:master Nov 12, 2021

This was referenced Nov 23, 2021

sql/schemachanger: TestInsertDuringAddColumnNotWritingToCurrentPrimaryIndex failed #72560

Closed

sql/schemachanger/scplan: TestPlanAlterTable failed #72561

Closed

sql/schemachanger: TestSchemaChangeWaitsForOtherSchemaChanges failed #72559

Closed

ajwerner mentioned this pull request Dec 14, 2021

sql/schemachanger/scexec: implement validation #66978

Closed

		nodeDepEdgesFrom: map[scpb.Node]btree.BTree{},
		nodeDepEdgesTo: map[scpb.Node]btree.BTree{},

		@@ -28,7 +31,19 @@ type MakeAddedIndexDeleteOnly struct {
		TableID descpb.ID

		// Index represents the index as it should appear in the mutation.

Conversation

fqazi commented Nov 10, 2021

Uh oh!

cockroach-teamcity commented Nov 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

postamar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fqazi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

fqazi commented Nov 11, 2021

Uh oh!

fqazi commented Nov 11, 2021

Uh oh!

craig bot commented Nov 12, 2021

Uh oh!

craig bot commented Nov 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants