sql/schemachanger: implement DROP COLUMN

[ajwerner]

See individual commits.

Fixes #84072

[cockroach-teamcity]

This change is 

[ajwerner]

The last commit should be broken up into a few. I'll do that later after we see how CI is doing.

[ajwerner]

Sent out the changefeed changes in #84571 and the testing fix in #84575.

[postamar]

I did a review pass on just the last commit. Nice work. This is certainly coming along! Very exciting.

Reviewed 74 of 74 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @postamar)

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go line 68 at r4 (raw file):

) {
	var rowLevelTTL *scpb.RowLevelTTL
	// TODO(ajwerner): Does this need to look at status or target status?

I would look at status if only because target status may be changed by the builder. I have no idea how consistent we are about this. Looking ahead, we need a better way to express queries on current state.

---

pkg/sql/schemachanger/scplan/internal/opgen/opgen_primary_index.go line 68 at r4 (raw file):

					return &scop.MakeMergedIndexWriteOnly{
						TableID: this.TableID,
						IndexID: this.IndexID,

I assume this extra op-edge was necessary for the rollbacks to work? What of the writes occurring between MERGED and WRITE_ONLY, won't they be lost?

---

pkg/sql/schemachanger/scplan/internal/rules/dep_index_and_column.go line 830 at r4 (raw file):

			}
		},
	)

I don't understand this rule. Shouldn't this be: the column can only reach WRITE_ONLY if the indexes can all be written to, in other words when the temp indexes are all WRITE_ONLY and the other indexes are also WRITE_ONLY?

[ajwerner]

I've pushed again and addressed the concrete feedback. After the changefeed changes make their way through bors and CI is happy (which I think it should be), I'll take a stab at breaking this out into a few more commits. I do think it's quite close.

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

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go line 68 at r4 (raw file):

Previously, postamar (Marius Posta) wrote…

I would look at status if only because target status may be changed by the builder. I have no idea how consistent we are about this. Looking ahead, we need a better way to express queries on current state.

Done.

---

pkg/sql/schemachanger/scplan/internal/opgen/opgen_primary_index.go line 68 at r4 (raw file):

Previously, postamar (Marius Posta) wrote…

I assume this extra op-edge was necessary for the rollbacks to work? What of the writes occurring between MERGED and WRITE_ONLY, won't they be lost?

This is necessary for very subtle reasons I remembered about only recently. We've had bugs we've been carrying around. I need to split this off into more commits to give some more motivation. This comment is revelatory:

cockroach/pkg/sql/backfill.go

Lines 1962 to 2039 in 116c5aa

	// backfillIndexes fills the missing columns in the indexes of the
	// leased tables.
	//
	//
	// If temporaryIndexes is non-empty, we assume that we are using the
	// MVCC-compatible backfilling process. This mutation has already been
	// checked to ensure all newly added indexes are using one type of
	// index backfill.
	//
	// The MVCC-compatible index backfilling process has a goal of not
	// having to issue AddSStable requests with backdated timestamps.
	//
	// To do this, we backfill new indexes while they are in a BACKFILLING
	// state in which they do not see writes or deletes. While the
	// backfill is running a temporary index captures all inflight rights.
	//
	// When the backfill is completed, the backfilling index is stepped up
	// to MERGING and then writes and deletes missed during
	// the backfill are merged from the temporary index.
	//
	// Finally, the new index is brought into the DELETE_AND_WRITE_ONLY
	// state for validation.
	//
	// ┌─────────────────┐ ┌─────────────────┐ ┌─────────────────┐
	// │ │ │ │ │ │
	// │ PrimaryIndex │ │ NewIndex │ │ TempIndex │
	// t0 │ (PUBLIC) │ │ (BACKFILLING) │ │ (DELETE_ONLY) │
	// │ │ │ │ │ │
	// └─────────────────┘ └─────────────────┘ └────────┬────────┘
	// │
	// ┌────────▼────────┐
	// │ │
	// │ TempIndex │
	// t1 │(DELETE_AND_WRITE) │
	// │ │ │
	// └────────┬────────┘ │
	// │ │
	// ┌─────────────────┐ ┌─────────────────┐ ┌────────▼────────┐ │ TempIndex receiving writes
	// │ │ │ │ │ │ │
	// │ PrimaryIndex ├────────►│ NewIndex │ │ TempIndex │ │
	// t2 │ (PUBLIC) │ Backfill│ (BACKFILLING) │ │(DELETE_AND_WRITE│ │
	// │ │ │ │ │ │ │
	// └─────────────────┘ └────────┬────────┘ └─────────────────┘ │
	// │ │
	// ┌────────▼────────┐ │
	// │ │ │
	// │ NewIndex │ │
	// t3 │ (DELETE_ONLY) │ │
	// │ │ │
	// └────────┬────────┘ │
	// │ │
	// ┌────────▼────────┐ │
	// │ │ │
	// │ NewIndex │ │ │
	// │ (MERGING) │ │ │
	// t4 │ │ │ │ NewIndex receiving writes
	// └─────────────────┘ │ │
	// │ │
	// ┌─────────────────┐ ┌─────────────────┐ │ │
	// │ │ │ │ │ │
	// │ NewIndex │◄────────────┤ TempIndex │ │ │
	// t5 │ (MERGING) │ BatchMerge │(DELETE_AND_WRITE│ │ │
	// │ │ │ │ │ │
	// └────────┬────────┘ └───────┬─────────┘ │ │
	// │ │ │ │
	// ┌────────▼────────┐ ┌───────▼─────────┐ │ │
	// │ │ │ │ │ │
	// │ NewIndex │ │ TempIndex │ │
	// t6 │(DELETE_AND_WRITE) │ (DELETE_ONLY) │ │
	// │ │ │ │ │
	// └───────┬─────────┘ └───────┬─────────┘ │
	// │ │
	// │ │
	// ▼ ▼
	// [validate and make public] [ dropped ]
	//
	// This operates over multiple goroutines concurrently and is thus not
	// able to reuse the original kv.Txn safely.

The issue is that in Merging we do not enforce uniqueness constraints. We need to be enforcing uniqueness constraints before we go on to validating the uniqueness constraint. This enforcement happens through the use of a CPut, which we do not use in Merging here:

cockroach/pkg/sql/catalog/table_elements.go

Lines 204 to 221 in 116c5aa

	// ForcePut, if true, forces all writes to use Put rather than CPut or InitPut.
	//
	// Users of this options should take great care as it
	// effectively mean unique constraints are not respected.
	//
	// Currently (2022-01-19) this two users: delete preserving
	// indexes and merging indexes.
	//
	// Delete preserving encoding indexes are used only as a log of
	// index writes during backfill, thus we can blindly put values into
	// them.
	//
	// New indexes may miss updates during the backfilling process
	// that would lead to CPut failures until the missed updates
	// are merged into the index. Uniqueness for such indexes is
	// checked by the schema changer before they are brought back
	// online.
	ForcePut() bool

---

pkg/sql/schemachanger/scplan/internal/rules/dep_index_and_column.go line 830 at r4 (raw file):

Previously, postamar (Marius Posta) wrote…

I don't understand this rule. Shouldn't this be: the column can only reach WRITE_ONLY if the indexes can all be written to, in other words when the temp indexes are all WRITE_ONLY and the other indexes are also WRITE_ONLY?

I think the temp indexes condition would be sufficient. This one is also sufficient. The deal is that we need to make sure that we are writing values of the column somewhere before we start doing the backfill. I have rewritten the rule to be about the temporary index and added commentary.

[ajwerner]

This is RFAL. I broke this apart into individual commits. I think that was a good exercise. Only really the fourth commit (
sql/rowenc: compute primary index encoding columns correctly) is of real interest to outside teams. I was disappointed to discover the lack of testing in that area, but haven't done anything to make it better.

[mgartner]

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

---

pkg/sql/rowenc/index_encoding.go line 1115 at r8 (raw file):

// getStoredColumnsForPrimaryIndex computes the set of columns stored in this
// primary index's value for encoding which are not composite columns. The

So what do we do with composite columns then?

[ajwerner]

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

---

pkg/sql/rowenc/index_encoding.go line 1115 at r8 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

So what do we do with composite columns then?

Have a look at where the output of this thing is used. The composite logic remains intact.

[ajwerner]

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

---

pkg/sql/rowenc/index_encoding.go line 1115 at r8 (raw file):

Previously, ajwerner wrote…

Have a look at where the output of this thing is used. The composite logic remains intact.

Oh, I see, the comment is bad. I'll update it.

[ajwerner]

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

---

pkg/sql/rowenc/index_encoding.go line 1115 at r8 (raw file):

Previously, ajwerner wrote…

Oh, I see, the comment is bad. I'll update it.

Alright, I've reworded the comment. Hopefully it's clearer now.

[mgartner]

Fourth and fifth commit LGTM.

Reviewed 1 of 1 files at r9, 2 of 2 files at r10.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @livlobo and @postamar)

---

pkg/sql/rowenc/index_encoding.go line 1115 at r8 (raw file):

Previously, ajwerner wrote…

Alright, I've reworded the comment. Hopefully it's clearer now.

Brilliant, thank you.

[fqazi]

For end-to-end testing are we comfortable adding multistatement transactions? i.e. Two drop columns, I know we can't do a mix of ADD/DROP yet?

Looks good, but didn't give enough time to the builder and some test stuff. I'll do that tomorrow

Reviewed 2 of 74 files at r4, 82 of 82 files at r5, 28 of 28 files at r6, 5 of 5 files at r7, 1 of 1 files at r9, 2 of 2 files at r10, 35 of 35 files at r11, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @livlobo, and @postamar)

---

pkg/sql/rowenc/index_encoding.go line 1131 at r9 (raw file):

	// columns set is not populated and instead we defer to the colMap to compute
	// the complete set before subtracting the key columns.
	if index.GetVersion() < descpb.PrimaryIndexWithStoredColumnsVersion {

I guess we will infinitely support older encoding types? Until indexes get recreated.

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go line 164 at r11 (raw file):

		b.Drop(cc)
	}
	handleDropColumnDefaultExpression(b, col, colElts, behavior)

This pattern is much cleaner than what we have in add column, we should do the same there in the future.

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go line 307 at r11 (raw file):

}

func toPublicNotCurrentlyPublicFilter(

Would it make sense to just have a routine that returns a filter closure?

statusAndTargetFilter( scpb.Status_PUBLIC, scpb.ToPublic)
statusFilter(...)

Just a thought more cosmetic I think.

---

pkg/sql/schemachanger/scexec/scmutationexec/column.go line 123 at r6 (raw file):

		*(protoutil.Clone(mut.GetColumn())).(*descpb.ColumnDescriptor))

	// Ensure that the column is added in the right location. This is important

Is there a reason we couldn't push this ordering into the dep rules? I guess removing or adding stuff in the middle?

---

pkg/sql/schemachanger/scplan/internal/rules/dep_index_and_column.go line 503 at r6 (raw file):

	// a column which does not exist. This would lead to panics inside the
	// optimizer and an invalid table descriptor.
	registerDepRule("indexes containing columns reach absent before column",

I guess validation would also catch this? Or it might be something should check for?

[ajwerner]

For end-to-end testing are we comfortable adding multistatement transactions? i.e. Two drop columns, I know we can't do a mix of ADD/DROP yet?

I added a few of these types of tests. They were good. Caught a bug with improperly sorting the statements. Also figured out I had not handled dropping the OnUpdate expression. It was good to flex the new data-driven explains as well.

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

---

pkg/sql/rowenc/index_encoding.go line 1131 at r9 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

I guess we will infinitely support older encoding types? Until indexes get recreated.

Yep. At least we know they aren't created by the declarative schema changer.

---

pkg/sql/schemachanger/scexec/scmutationexec/column.go line 123 at r6 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Is there a reason we couldn't push this ordering into the dep rules? I guess removing or adding stuff in the middle?

We won't necessarily have elements in the plan for the other columns which aren't being dropped. Consider the case of rolling back the drop column of the first column in the table.

---

pkg/sql/schemachanger/scplan/internal/rules/dep_index_and_column.go line 503 at r6 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

I guess validation would also catch this? Or it might be something should check for?

I'll add the validation in a separate PR.

[ajwerner]

This is RFAL

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

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go line 307 at r11 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Would it make sense to just have a routine that returns a filter closure?

statusAndTargetFilter( scpb.Status_PUBLIC, scpb.ToPublic)
statusFilter(...)

Just a thought more cosmetic I think.

I didn't do anything in this commit but I'm interested in improvements here in future commits.

[Xiang-Gu]

This is a big change! I previously don't know that an index that is in MERGE_ONLY or MERGED can still receive writes. What is the concern of checking uniqness of writes during merging previously (when we transition the primary index to WRITE_ONLY before merging with the temporary index)?

[ajwerner]

While the index is in MERGING in the table descriptor, write to it use Put instead of CPut. See ForcePut on catalog.Index.

Consider the following timeline:

create table t (i int primary key, j int);
insert into t values (1, 1);
create unique index idx on t(j);

t1: index merging concludes
at this point the unique index contains:
1: 1

t2: validation begins by reading a snapshot at t2 to count that the number of rows in the unique index is equal to the number of rows in the primary index
t3: insert into t values (2, 1) -- this will lead to the unique index now only containing 1: 2 instead of 1: 1 and the force from MERGING means the write will succeed.
t4: Validation completes successfully and the index is moved to public

At this point we have a corrupt unique secondary index.

If, however, we moved from MERGING to WRITE_ONLY before proceeding with validation, the insert operation would fail and the index will remain consistent.

[Xiang-Gu]

This is a learning question: everytime I think of merging (with a temporary index), it's always been on a (new) primary index which is intended to be swapped with the old primary index, and that happens quite often (ADD COLUMN, DROP COLUMN, ALTER PRIMARY KEY, etc. As long as the primary index needs to be modified, we use this primary index swap technique). But here, I'm surprised to learn that secondary indexes also go through the same status, so I start to wonder in what case will we ever do this backfill-then-merge on a secondary index?

[ajwerner]

We always need to merge concurrent writes into an index when backfilling as of 22.1. The reasons for this are somewhat surprising and relate to history rewriting. See this rfc: https://github.com/cockroachdb/cockroach/blob/master/docs/RFCS/20211004_incremental_index_backfiller.md

[Xiang-Gu]

1. I don't quite understand the purpose of ct here, which is a ColumnType element?
2. Why do we need this clause that uses the isRelationBeingDropped field, which I thought was deemed "hacky"?

[postamar]

I think this is needed to gracefully handle DROP TABLE. IIRC we wanted to replace this filter (and thus remove this field) by using better rule constructs, something along the lines of "this rule only applies on this column if the parent table isn't present in the rel-database with an ABSENT target".

[Xiang-Gu]

This rule seems to be relevant when adding a column (so we insist this newly added column reaches WRITE_ONLY before the temporary reaches WRITE_ONLY). I'm curious how you discover the need to add such a dep rule when working on DROP COLUMN

[ajwerner]

You're right that this is unrelated to drop column. I don't remember where this came up, but there was a case I ran into where this mattered. I think it was that I removed the WRITE_ONLY state from "column depends on primary index" because it broke rollbacks of drop column, at which point in the add column case now it was valid to defer moving the column to WRITE_ONLY until later which caused some other problem.

[fqazi]

Looks good just a few minor questions from me. I think most of those are non-issues.

Reviewed 71 of 71 files at r12, 28 of 28 files at r13, 5 of 5 files at r14, 1 of 1 files at r15, 2 of 2 files at r16, 49 of 49 files at r17, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @fqazi, @livlobo, @postamar, and @Xiang-Gu)

---

pkg/sql/schemachanger/BUILD.bazel line 12 at r17 (raw file):

go_test(
    name = "schemachanger_test",
    size = "large",

I guess the drop stuff ballooned the length of this test, hopefully we can get this down with parallelism.

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go line 282 at r17 (raw file):

		}
		if ic.Kind != scpb.IndexColumn_STORED {
			panic(errors.AssertionFailedf("can only drop columns which are stored in the primary index, this one is %v ",

Are we surfacing the right error here? Or should this be "column %q is referenced by the primary key". Just sanity checking

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go line 353 at r17 (raw file):

	for i := n + 1; i < len(storedColumns); i++ {
		storedColumns[i].OrdinalInKind--
		b.Add(storedColumns[i])

I think this is fine because it will just replace the existing element based on the remaining keys. I'm guessing the ops will do the heavy lifting here since the target state will stay the same

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go line 374 at r17 (raw file):

		return
	}
	// TODO(ajwerner): Support dropping UNIQUE WITHOUT INDEX constraints.

We might just want a checklist-type Github issue for these. As we are doing this type of stuff over time some of these cases will take longer to address.

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go line 458 at r17 (raw file):

func handleDropColumnExpressions(b BuildCtx, colElts ElementResultSet, behavior tree.DropBehavior) {
	publicTargets := colElts.Filter(publicTargetFilter)

Does it make sense for the caller to give a filtered set of elements?

---

pkg/sql/schemachanger/scexec/scmutationexec/column.go line 123 at r6 (raw file):

Previously, ajwerner wrote…

We won't necessarily have elements in the plan for the other columns which aren't being dropped. Consider the case of rolling back the drop column of the first column in the table.

Ooh right

[ajwerner]

@fqazi other than the passing of the public elements are there any changes requested? Can I defer that?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @fqazi, @livlobo, @postamar, and @Xiang-Gu)

---

pkg/sql/schemachanger/BUILD.bazel line 12 at r17 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

I guess the drop stuff ballooned the length of this test, hopefully we can get this down with parallelism.

Indeed. I'm waiting on Ricky's logictest change before doing something.

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go line 282 at r17 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Are we surfacing the right error here? Or should this be "column %q is referenced by the primary key". Just sanity checking

This is an assertion failure because we already checked whether it was a primary key column earlier in the dropColumn function in checkColumnNotInPrimaryKey. I don't expect to see this error.

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go line 353 at r17 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

I think this is fine because it will just replace the existing element based on the remaining keys. I'm guessing the ops will do the heavy lifting here since the target state will stay the same

Yes, we'll overwrite the existing element being added.

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go line 374 at r17 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

We might just want a checklist-type Github issue for these. As we are doing this type of stuff over time some of these cases will take longer to address.

Yes, I'll create one.

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go line 458 at r17 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Does it make sense for the caller to give a filtered set of elements?

I suppose, but I'm not eager to wait for another CI cycle if you're amenable.

[fqazi]

Yes you can defer that

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @fqazi, @livlobo, @postamar, and @Xiang-Gu)

[ajwerner]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)