sql: add support for create index inside new schema changer #67266

fqazi · 2021-07-06T13:50:39Z

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-07-06T13:50:50Z

This change is

nihalpednekar · 2021-07-06T15:28:39Z

LGTM from Bulk perspective

nihalpednekar · 2021-07-06T18:01:51Z

@adityamaru brought to my attention that Bulk also owns some parts of */backfill.go so please wait until someone else from Bulk takes a look at this as well.

ajwerner

I'm anxious about the ways that this is running into the pain of descriptors vs. elements.

Reviewed 19 of 33 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @fqazi)

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

//    mod(fnv32(colNames[0]::STRING)+fnv32(colNames[1])+...,buckets)
//
func makeHashShardComputeExpr(colNames []string, buckets int) *string {

I wonder if we can share this code?

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

func (b *buildContext) createIndex(ctx context.Context, n *tree.CreateIndex) {
	// Look up the table first

nit: period

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

	_, table, err := resolver.ResolveExistingTableObject(ctx, b.Res, &n.Table,
		tree.ObjectLookupFlagsWithRequired())

we ought to be able to plumb through a mutable flag here to not need to resolve it again. I'd like to know if that does not work.

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

		panic(err)
	}
	// Detect if the index already exists

nit: period

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

		if foundIndex.Dropped() {
			panic(pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
				"index %q being dropped, try again later", string(n.Name)))

nit: use the format/string method on the AST node for an index name.

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

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

&n.Table?

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

	}
	colNames := make([]string, 0, len(n.Columns))
	// Setup the column ID's

nit: period and no '.

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

Quoted 10 lines of code…

			_, typ, _, err := schemaexpr.DequalifyAndValidateExpr(
				ctx,
				table,
				columnNode.Expr,
				types.Any,
				"index expression",
				b.SemaCtx,
				tree.VolatilityImmutable,
				&n.Table,
			)

This is not going to work if the column was added in this transaction. At least we should detect that case by searching the elements and then returning a not supported error.

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

Quoted 4 lines of code…

			mutableDesc, err := b.Descs.GetMutableTableByID(ctx, b.EvalCtx.Txn, table.GetID(), tree.ObjectLookupFlagsWithRequired())
			if err != nil {
				panic(err)
			}

let's avoid this lookup.

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

			// Otherwise, create a new virtual column and add it to the table
			// descriptor.

Otherwise relative to what?

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

			// descriptor.
			colName := tabledesc.GenerateUniqueName("crdb_idx_expr", func(name string) bool {
				_, err := mutableDesc.FindColumnWithName(tree.Name(name))

Doesn't this need to think about newly added columns? Imagine creating more than one index or creating an index with more than one expression.

pkg/sql/schemachanger/scbuild/table.go, line 253 at r1 (raw file):

		FamilyName: familyName,
	})
	/*newPrimaryIdxID := */ b.addOrUpdatePrimaryIndexTargetsForAddColumn(table, colID, col.Name)

What is this about?

pkg/sql/lex/BUILD.bazel, line 7 at r1 (raw file):

    srcs = [
        "encode.go",
        "keywords.go",

I don't think you meant to add these.

fqazi

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

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

Previously, ajwerner wrote…

	_, table, err := resolver.ResolveExistingTableObject(ctx, b.Res, &n.Table,
		tree.ObjectLookupFlagsWithRequired())
we ought to be able to plumb through a mutable flag here to not need to resolve it again. I'd like to know if that does not work.

Done.

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

Previously, ajwerner wrote…

&n.Table?

Done.

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

Previously, ajwerner wrote…

			mutableDesc, err := b.Descs.GetMutableTableByID(ctx, b.EvalCtx.Txn, table.GetID(), tree.ObjectLookupFlagsWithRequired())
			if err != nil {
				panic(err)
			}

let's avoid this lookup.

Done.

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

create
Your right no otherwise, this will always be done

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

Previously, ajwerner wrote…

Doesn't this need to think about newly added columns? Imagine creating more than one index or creating an index with more than one expression.

Yes, we need to sort out those scenarios, as a stop-gap, I'm returning unsupported errors. The query stuff I think will allow us better interfaces for this stuff too.

pkg/sql/schemachanger/scbuild/table.go, line 253 at r1 (raw file):

Previously, ajwerner wrote…

What is this about?

We were incorrectly adding a secondaryIndex with the same ID in the unique case if I remember correctly. So, the newPrimaryIdxId parameter has no real use.

pkg/sql/lex/BUILD.bazel, line 7 at r1 (raw file):

Previously, ajwerner wrote…

I don't think you meant to add these.

Done.

ajwerner

My comments are mostly superficial and point towards tightening up the commentary.

ajwerner · 2021-11-08T13:47:24Z

pkg/sql/schemachanger/scbuild/table.go

@@ -132,7 +194,7 @@ func (b *buildContext) alterTableAddColumn(
 		familyID = b.findOrAddColumnFamily(
 			table, familyName, d.Family.Create, d.Family.IfNotExists,
 		)
-	} else {
+	} else if !d.IsVirtual() { // FIXME: Compute columns should not have families?


Stored ones definitely need families. Virtual ones I would think can live without them but I don't know whether or not they do.

ajwerner · 2021-11-08T13:48:53Z

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

+	allowedNewColumnNames []tree.Name,
+	allowImplicitPartitioning bool,
+) error {
+	return nil


ideally we'd track this and verify it in the tests

ajwerner · 2021-11-08T13:51:18Z

pkg/sql/schemachanger/scgraph/graph.go

@@ -249,3 +265,50 @@ func (de *DepEdge) To() *scpb.Node { return de.to }

 // Name returns the name of the rule which generated this edge.
 func (de *DepEdge) Name() string { return de.rule }
+
+// GetNodeRanks fetches ranks of nodes in topological order


nit: period

ajwerner · 2021-11-08T13:52:46Z

pkg/sql/schemachanger/scjob/job.go

@@ -66,6 +70,10 @@ func (n *newSchemaChangeResumer) Resume(ctx context.Context, execCtxI interface{
 		n.job,
 		execCfg.Codec,
 		execCfg.Settings,
+		sql.MakeIndexValidator(execCfg.DB, execCfg.Codec, execCfg.InternalExecutor),
+		func(ctx context.Context, tableDesc *tabledesc.Mutable, indexDesc descpb.IndexDescriptor, partBy *tree.PartitionBy, allowedNewColumnNames []tree.Name, allowImplicitPartitioning bool) (newImplicitCols []catalog.Column, newPartitioning descpb.PartitioningDescriptor, err error) {


wrap this, or, perhaps better, write a function that has this signature.

How would you feel about creating an interface as opposed to this function type? IDEs make it easy to implement the interface.

ajwerner · 2021-11-08T13:53:04Z

pkg/sql/schemachanger/scop/mutation.go

@@ -119,6 +142,8 @@ type RemoveTypeBackRef struct {
 	TypeID descpb.ID
 }

+// FIXME: Check create index first


Can you add more to this?

ajwerner · 2021-11-08T13:53:16Z

pkg/sql/schemachanger/scop/mutation.go

+	RangePartitions []*scpb.RangePartitions
+}
+
+// NoOpInfo a no-op mutation operation.


Can you explain what this is for?

ajwerner · 2021-11-08T13:53:42Z

pkg/sql/schemachanger/scpb/scpb.proto

@@ -189,3 +218,26 @@ message Database {
  uint32 database_id = 1 [(gogoproto.customname) = "DatabaseID", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb.ID"];
  repeated uint32 dependentObjects = 2  [(gogoproto.customname) = "DependentObjects", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb.ID"];
 }
+
+// FIXME: Dead code


ajwerner · 2021-11-08T13:54:36Z

pkg/sql/schemachanger/scplan/plan.go

 		next := append(cur[:0:0], cur...)
 		isStageRevertible := true
 		var ops []scop.Op
 		for revertible := 1; revertible >= 0; revertible-- {
 			isStageRevertible = revertible == 1
-			for i, ts := range cur {
-				for _, e := range edges {
+			// FIXME: Flip the loop


can you say more or do this?

ajwerner · 2021-11-08T13:54:46Z

pkg/sql/schemachanger/scplan/plan.go

@@ -223,132 +235,10 @@ func buildStages(init scpb.State, g *scgraph.Graph, params Params) []Stage {
 			break
 		}
 		// Sort ops based on graph dependencies.
-		sortOps(g, s.Ops.Slice())
+		//SortOps(g, s.Ops.Slice())


ajwerner · 2021-11-08T13:55:41Z

pkg/sql/schemachanger/scplan/plan_test.go

@@ -191,3 +193,84 @@ func marshalOps(t *testing.T, plan *scplan.Plan) string {
 	}
 	return stages.String()
 }
+
+// TestPlanGraphSort sanity checks sorting of the graph.


it seems like this test can go in scgraph. I wonder if we can rework it to be easier to read and extend.

fqazi

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

pkg/sql/schemachanger/scjob/job.go, line 74 at r24 (raw file):

How would you feel about creating an interface as opposed to this function type? IDEs make it easy to implement the interface.

pkg/sql/schemachanger/scop/mutation.go, line 145 at r24 (raw file):

Previously, ajwerner wrote…

Can you add more to this?

Stale comment left over from when I started off.

pkg/sql/schemachanger/scop/mutation.go, line 267 at r24 (raw file):

Previously, ajwerner wrote…

Can you explain what this is for?

// NoOpInfo a no-op mutation operation, which is allows
// an emit function to conditionally generate operations.

pkg/sql/schemachanger/scpb/scpb.proto, line 222 at r24 (raw file):

Previously, ajwerner wrote…

?

Stale comment from development, I should have checked my diffs for these.

Done.

pkg/sql/schemachanger/scplan/plan.go, line 170 at r24 (raw file):

Previously, ajwerner wrote…

can you say more or do this?

The edge and cur loops were flipped, which was a reminder for me.

Done.

pkg/sql/schemachanger/scplan/plan.go, line 238 at r24 (raw file):

Previously, ajwerner wrote…

detritus

Done

pkg/sql/schemachanger/scplan/plan_test.go, line 197 at r24 (raw file):

Previously, ajwerner wrote…

it seems like this test can go in scgraph. I wonder if we can rework it to be easier to read and extend.

Reworked this to have more syntatic sugar and more readability.

Done.

ajwerner

when it builds

Reviewed 1 of 58 files at r25, 1 of 33 files at r26, 4 of 46 files at r27.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @fqazi)

pkg/sql/schema_changer_ccl.go, line 50 at r27 (raw file):

		allowImplicitPartitioning)
}
func MakeCCLCallbacks(

linter is going to want a comment here

pkg/sql/schemachanger/scexec/dependencies.go, line 99 at r27 (raw file):

// CCLCallbacks provides an interface that implements CCL exclusive
// callbacks.
type CCLCallbacks interface {

nit: call this Partitioner? The fact that it's CCL isn't that important or meaningful.

pkg/sql/schemachanger/scplan/plan.go, line 236 at r27 (raw file):

			break
		}
		// Sort ops based on graph dependencies.

This comment is now stale.

fqazi · 2021-11-08T17:58:53Z

@ajwerner TFTR, thanks for the lightning faster review :)

fqazi · 2021-11-09T01:03:40Z

bors r+

craig · 2021-11-09T01:33:40Z

Build failed (retrying...):

GitHub CI (Cockroach)

ajwerner · 2021-11-09T01:55:16Z

Merge skew with the interleaved removal.

bors r-

craig · 2021-11-09T01:55:18Z

Canceled.

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#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-09T05:57:34Z

bors r+

craig · 2021-11-09T06:39:09Z

Build succeeded:

GitHub CI (Cockroach)

72436: server: fix some logging output related to configuration r=rauchenstein a=knz 72535: admission: fix race conditions involving WorkQueue GC r=sumeerbhola a=sumeerbhola The periodic reset of tenantInfo.used=0 and the early releasing of the mutex in Admit can cause two race conditions (when the WorkQueue is using tokens): - A tenantInfo referenced in Admit may have been GC'd. - The decrement of used, when work is cancelled, can cause the uint64 to overflow. Both are fixed, and a test is added that fails if the original code is instrumented with additional invariant checking. The new code includes the extra invariant checking. Fixes #72485 Release note: None 72553: pkg/roachprod: allow multiple-stores to be created on GCP r=nicktrav a=nicktrav Port an existing flag from the AWS roachprod flags that allows multiple stores to be created. When this flag is enabled, multiple data directories are created and mounted as `/mnt/data{1..N}`. Standardize the existing ext4 disk creation logic in the GCE setup script to match the AWS functionality. Interleave the existing ZFS setup commands based on the `--filesystem` flag. Fix a bug introduced in #54986 that will always create multiple data disks, ignoring the value of the flag. This has the effect of never creating a RAID 0 array, which is the intended default behavior. The ability to create a RAID 0 array on GCE VMs is required for the Pebble write-throughput benchmarks. Release note: None 72567: sql: simplify the role option formatting logic r=rafiss,RichardJCai a=knz Release note: None 72584: Revert "sql: add support for create index inside new schema changer" r=fqazi a=fqazi Reverts #67266 Co-authored-by: Raphael 'kena' Poss <[email protected]> Co-authored-by: sumeerbhola <[email protected]> Co-authored-by: Nick Travers <[email protected]> Co-authored-by: Faizan Qazi <[email protected]>

fqazi requested review from a team and nihalpednekar and removed request for a team July 6, 2021 13:50

fqazi force-pushed the createTableNew branch 3 times, most recently from 9590bae to 0bfb2d5 Compare July 6, 2021 14:41

nihalpednekar approved these changes Jul 6, 2021

View reviewed changes

fqazi force-pushed the createTableNew branch from 0bfb2d5 to 525f96b Compare July 6, 2021 17:37

nihalpednekar requested a review from a team July 6, 2021 18:02

fqazi force-pushed the createTableNew branch 6 times, most recently from c363764 to f1d4499 Compare July 29, 2021 23:51

fqazi requested a review from a team as a code owner July 29, 2021 23:51

fqazi force-pushed the createTableNew branch 2 times, most recently from 6caad0e to 85cd376 Compare July 30, 2021 13:51

ajwerner reviewed Aug 2, 2021

View reviewed changes

fqazi force-pushed the createTableNew branch from 85cd376 to d241fa8 Compare August 3, 2021 17:30

fqazi requested a review from a team as a code owner August 3, 2021 17:30

fqazi force-pushed the createTableNew branch from d241fa8 to b4fc302 Compare August 3, 2021 19:04

fqazi commented Aug 3, 2021

View reviewed changes

fqazi force-pushed the createTableNew branch from b4fc302 to 55e354b Compare August 3, 2021 19:21

fqazi requested a review from ajwerner August 3, 2021 19:22

fqazi force-pushed the createTableNew branch from 55e354b to a8c31c1 Compare August 4, 2021 03:03

ajwerner reviewed Nov 8, 2021

View reviewed changes

fqazi force-pushed the createTableNew branch from 4264e81 to ce4dbe1 Compare November 8, 2021 17:48

fqazi commented Nov 8, 2021

View reviewed changes

ajwerner approved these changes Nov 8, 2021

View reviewed changes

fqazi force-pushed the createTableNew branch from ce4dbe1 to 8ef5b5c Compare November 8, 2021 17:57

fqazi force-pushed the createTableNew branch 7 times, most recently from 12a69d0 to 56966da Compare November 8, 2021 21:50

fqazi force-pushed the createTableNew branch from 56966da to bba020a Compare November 9, 2021 05:30

fqazi added 2 commits November 9, 2021 00:36

fqazi force-pushed the createTableNew branch from bba020a to b9a65b1 Compare November 9, 2021 05:42

fqazi force-pushed the createTableNew branch from b9a65b1 to a90bd95 Compare November 9, 2021 05:55

craig bot merged commit 5d68dff into cockroachdb:master Nov 9, 2021

This was referenced Nov 9, 2021

sql/schemachanger/scplan: TestPlanAlterTable failed #72562

Closed

Revert "sql: add support for create index inside new schema changer" #72584

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add support for create index inside new schema changer #67266

sql: add support for create index inside new schema changer #67266

fqazi commented Jul 6, 2021

cockroach-teamcity commented Jul 6, 2021

nihalpednekar commented Jul 6, 2021

nihalpednekar commented Jul 6, 2021

ajwerner left a comment

fqazi left a comment

ajwerner left a comment

ajwerner Nov 8, 2021

ajwerner Nov 8, 2021

ajwerner Nov 8, 2021

ajwerner Nov 8, 2021

ajwerner Nov 8, 2021

ajwerner Nov 8, 2021

ajwerner Nov 8, 2021

ajwerner Nov 8, 2021

ajwerner Nov 8, 2021

ajwerner Nov 8, 2021

fqazi left a comment

ajwerner left a comment

fqazi commented Nov 8, 2021 •

edited

Loading

fqazi commented Nov 9, 2021

craig bot commented Nov 9, 2021

ajwerner commented Nov 9, 2021

craig bot commented Nov 9, 2021

fqazi commented Nov 9, 2021

craig bot commented Nov 9, 2021

sql: add support for create index inside new schema changer #67266

sql: add support for create index inside new schema changer #67266

Conversation

fqazi commented Jul 6, 2021

cockroach-teamcity commented Jul 6, 2021

nihalpednekar commented Jul 6, 2021

nihalpednekar commented Jul 6, 2021

ajwerner left a comment

Choose a reason for hiding this comment

fqazi left a 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

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 left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

fqazi commented Nov 8, 2021 • edited Loading

fqazi commented Nov 9, 2021

craig bot commented Nov 9, 2021

ajwerner commented Nov 9, 2021

craig bot commented Nov 9, 2021

fqazi commented Nov 9, 2021

craig bot commented Nov 9, 2021

fqazi commented Nov 8, 2021 •

edited

Loading