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

pkg/sql/delete_preserving_index_test.go, line 30 at r1 (raw file):

	"github.com/cockroachdb/cockroach/pkg/sql/tests"
	"github.com/cockroachdb/cockroach/pkg/startupmigrations"
	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/fmtsafe/testdata/src/github.com/cockroachdb/errors"

Looks like goimports picked up the wrong errors package. you'll want github.com/cockroachdb/errors

pkg/sql/delete_preserving_index_test.go, line 132 at r1 (raw file):

		prefixEnd := append(prefix, []byte("\xff")...)

		revisions, err := storageccl.GetAllRevisions(context.Background(), kvDB, prefix, prefixEnd, now, end)

I believe we typically want to avoid importing ccl code in non-ccl packages. It's test code, but we might still want to check on this and perhaps just move where this function lives.

pkg/sql/catalog/descpb/structured.proto, line 566 at r1 (raw file):

  optional string predicate = 23 [(gogoproto.nullable) = false];

  optional bool use_delete_preserving_encoding = 24 [(gogoproto.nullable) = false];

Perhaps add a comment on why we have a separate bool for this instead of using the encode_type field.

pkg/sql/row/helper.go, line 94 at r1 (raw file):

	// Secondary indexes.
	Indexes      []catalog.Index
	indexEntries map[catalog.Index][]rowenc.IndexEntry

It looks like before this was stored on the struct to save on allocations in encodeSecondaryIndexes and encodeIndexes. Now, it looks like we are always allocating a new map. If we don't think that allocation savings is important anymore, we can probably remove this from the struct.

If we do still care about the allocation savings, perhaps we could so something where we store all of the entries in one slice and then have the maps that we return contain subslices of that single backing slice or something.

pkg/sql/row/helper.go, line 314 at r1 (raw file):

		if err != nil {
			return err
		}

I wonder if it would be worth it to cache deleteEncoding somewhere, since we are always writing the same value. If for some reason we do want to encode it every time, I believe you can pass a protoutil.Message directly to (*batch).Put which has some code to avoid unnecessary allocation and copying in this case.

pkg/sql/row/helper.go, line 317 at r1 (raw file):

		if traceKV {
			log.VEventf(ctx, 2, "Put %s -> %s", entry.Key, entry.Value.PrettyPrint())

Wonder if it would be useful to add something like (delete) to this entry to remind us this is a delete.

pkg/sql/rowenc/index_encoding.go, line 1199 at r1 (raw file):

			indexEntries[i].Value.SetBytes(encodedEntry)
		}
	}

We could avoid iterating over indexEntries again by doing this when we are constructing entry. Although, I suppose it is nice that this is parallel with how we are doing it in EncodeSecondaryIndex.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @stevendanna, and @tbg)

pkg/sql/delete_preserving_index_test.go, line 30 at r1 (raw file):

Done.

pkg/sql/delete_preserving_index_test.go, line 132 at r1 (raw file):

Done. I moved the helper function to the sql/backfill package.

pkg/sql/catalog/descpb/structured.proto, line 566 at r1 (raw file):

Done. Added to the message-level comment.

pkg/sql/row/helper.go, line 94 at r1 (raw file):

Done. Removed this stored field for this PR.

pkg/sql/row/helper.go, line 314 at r1 (raw file):

Done. Cached the deleteEncoding as a var in the file.

pkg/sql/row/helper.go, line 317 at r1 (raw file):

Done.

pkg/sql/rowenc/index_encoding.go, line 1199 at r1 (raw file):

How expensive is it to iterate through the loop this way? I want to make sure to catch all of the places that an entry might be built and this seemed like a reasonable way to do it.

pkg/roachpb/data.proto, line 757 at r1 (raw file):

Done. I moved this to catalog/descpb/structured.proto

Reviewed 9 of 18 files at r1, 5 of 9 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @rhu713, @stevendanna, and @tbg)

pkg/sql/delete_preserving_index_test.go, line 132 at r1 (raw file):

+1 to not depending on ccl, but I'm not sure backfill is the right place for it either. maybe kv/kvclient (since it is a client utility helper for using the KV API) or kv/bulk ?

pkg/sql/row/helper.go, line 94 at r1 (raw file):

hm, we'll want to work with sql folks to make sure we benchmark and profile some write-heavy workloads after this merges since allocating on every row might show up. commented below with a potential other idea.

pkg/sql/row/helper.go, line 198 at r2 (raw file):

) (secondaryIndexEntries map[catalog.Index][]rowenc.IndexEntry, err error) {

	indexEntries := make(map[catalog.Index][]rowenc.IndexEntry, len(rh.Indexes))

I wonder if we should keep rh.indexEntries around and just change its type to map instead to avoid re-allocating every time? we could do for i := range rh.indexEntries { rh.indexEntries[i] = rh.indexEntries[i][:0]} or something to reset it?

pkg/sql/backfill/revision_reader.go, line 3 at r2 (raw file):

// Copyright 2016 The Cockroach Authors.
//
// Licensed as a CockroachDB Enterprise file under the Cockroach Community

I think we also need to switch this to the BSL header instead of the CCL header.

pkg/sql/backfill/revision_reader.go, line 9 at r2 (raw file):

//     https://github.com/cockroachdb/cockroach/blob/master/licenses/CCL.txt

package backfill

mentioned below, think this should probably live elsewhere, maybe pkg/kv/kvclient

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @stevendanna, and @tbg)

pkg/sql/delete_preserving_index_test.go, line 132 at r1 (raw file):

Done. Moved to kv/client.

pkg/sql/row/helper.go, line 94 at r1 (raw file):

Okay, sounds good. That'll be after this PR is merged right? or before?

pkg/sql/row/helper.go, line 198 at r2 (raw file):

Okay. I changed this logic to make() the map when its len() is too short as an initialization check. And then I'm resetting it as you've suggested afterwards. Let me know what you think about it.

pkg/sql/backfill/revision_reader.go, line 3 at r2 (raw file):

Done.

pkg/sql/backfill/revision_reader.go, line 9 at r2 (raw file):

Done. Moved to kv/client.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @rhu713, @stevendanna, and @tbg)

pkg/sql/delete_preserving_index_test.go, line 132 at r1 (raw file):

I think they call the package kvclient vs just client unless you meant to make a new package?

pkg/sql/row/helper.go, line 94 at r1 (raw file):

I think after is probably OK -- we can get this working to unblock the schema change work, then check the perf for any stay allocs or whatever in parallel after

pkg/sql/row/helper.go, line 198 at r2 (raw file):

I doubt this matters, since I think it'll be the same len for all rows, but I think you could just compare to nil for the initialization.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @stevendanna, and @tbg)

pkg/sql/delete_preserving_index_test.go, line 132 at r1 (raw file):

Oops typoed the refactor, fixed.

pkg/sql/row/helper.go, line 198 at r2 (raw file):

Okay, used nil for the initialization.

Very cool!

I think we should add some kvtrace logic tests in pkg/sql/exec/execbuilder/testdata/delete_nonmetamorphic (a new file) and .../update_nonmetamorphic and .../upsert_nonmetamorphic that show these index deletes working on complicated indexes like partial indexes, expression indexes, inverted indexes, multi-column inverted indexes. See pkg/sql/exec/execbuilder/testdata/partial_index_nonmetamorphic for some examples. I think you'll have to do some "descriptor surgery" on the index descriptor to get it into a state where UseDeletePreservingEncoding is true. For this you should be able to use crdb_internal.pb_to_json, crdb_internal.json_to_pb, and crdb_internal.unsafe_upsert_descriptor. See pkg/sql/logictest/testdata/logic_test/schema_repair for some examples. Let me know if you run into issues.

Reviewed 11 of 18 files at r1, 3 of 9 files at r2, 3 of 4 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @mgartner, @rhu713, @stevendanna, and @tbg)

-- commits, line 8 at r5:
nit: give a short explanation of why delete-preserving index entries are needed, or at least link to the RFC.

pkg/sql/catalog/descpb/structured.proto, line 566 at r5 (raw file):

  optional string predicate = 23 [(gogoproto.nullable) = false];

  optional bool use_delete_preserving_encoding = 24 [(gogoproto.nullable) = false];

A comment describing what it means when this field is true might be helpful. Consider referencing the RFC

pkg/sql/catalog/descpb/structured.proto, line 1488 at r5 (raw file):

  optional bytes value = 1;

  optional bool deleted = 2 [(gogoproto.nullable) = false];

nit: explain that when deleted is true, value will be nil.

pkg/sql/catalog/tabledesc/index.go, line 337 at r5 (raw file):

}

func (w index) UseDeletePreservingEncoding() bool {

nit: add a comment to explain what this means, maybe referencing the RFC

pkg/sql/row/deleter.go, line 187 at r5 (raw file):

	for _, entry := range secondaryIndexEntry {
		if err := rd.Helper.deleteIndexEntry(ctx, b, idx, nil, &entry, traceKV); err != nil {

No need to change this, but I'll note that DeleteIndexRow is only called when truncating an entire index, so I don't think you need to use the helper here. More specifically, this code path is only exercised for truncating interleaved indexes, so we can likely delete it all now anyway, given that we no longer support interleaved indexes. I'll make a note to myself to delete this all soon, so don't worry about it.

pkg/sql/row/helper.go, line 94 at r1 (raw file):

I don't see the need to make this a map. The i-th element of secondaryIndexEntries should correspond to the i-th secondary index in rh.Indexes. So when you need to call rh.deleteIndexEntry for index entry i, you can pass rh.Indexes[i] as the index.

Perhaps an even better option is to simplify the interface of rh.deleteIndexEntry to:

func (rh *rowHelper) deleteIndexEntry(
	ctx context.Context,
	batch *kv.Batch,
        indexOrd int,
	valDirs []encoding.Direction,
	traceKV bool,
) error

deleteIndexEntry can get the indexOrd-th index from rh.Indexes and the indexOrd-th entry from rh.indexEntries. But I do see that the update code must pass in entries not in rh.indexEntries, so you'd need a separate version of deleteIndexEntry for that. I'm not sure if that's worth it.

Regardless of the approach, I think we can remove this map, unless I'm missing something.

pkg/sql/row/helper.go, line 318 at r5 (raw file):

	if index.UseDeletePreservingEncoding() {
		if traceKV {
			log.VEventf(ctx, 2, "Put (delete) %s -> %s", entry.Key, entry.Value.PrettyPrint())

We aren't actually putting entry.Value, so should we remove that from the log?

pkg/sql/row/inserter.go, line 185 at r5 (raw file):

	// For determinism, add the entries for the secondary indexes in the same
	// order as they appear in the helper.

If we remove the map, I think we can revert this change.

pkg/sql/row/updater.go, line 508 at r5 (raw file):

		// For determinism, add the entries for the secondary indexes in the same
		// order as they appear in the helper.

ditto

pkg/sql/rowenc/index_encoding.go, line 1191 at r5 (raw file):

	if index.UseDeletePreservingEncoding() {
		for i := range indexEntries {

nit: This pattern is the same as for secondary indexes. You could make a helper function like wrapIndexEntries to de-duplicate some code.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @mgartner, @rhu713, @stevendanna, and @tbg)

pkg/sql/row/helper.go, line 94 at r1 (raw file):

Is the map a problem? we're still reusing it across rows, like we did the slice before, and zero-slicing and resting the slices in it, so do we think it is a substantial increase in allocs?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @mgartner, @rhu713, @stevendanna, and @tbg)

pkg/sql/row/helper.go, line 94 at r1 (raw file):

• s/resting/reusing/

Okay, I'll work on these tests now.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @mgartner, @rhu713, @stevendanna, and @tbg)

-- commits, line 8 at r5:

Done.

pkg/sql/catalog/descpb/structured.proto, line 566 at r5 (raw file):

Done.

pkg/sql/catalog/descpb/structured.proto, line 1488 at r5 (raw file):

Done.

pkg/sql/catalog/tabledesc/index.go, line 337 at r5 (raw file):

Done.

pkg/sql/row/deleter.go, line 187 at r5 (raw file):

Gotcha.

pkg/sql/row/helper.go, line 94 at r1 (raw file):

Here's a comparison between the new changes and master with 10 runs, at first glance there doesn't seem to be too much difference?

name                                         old time/op  new time/op  delta
SQL/Cockroach/Update/count=1-16               488µs ± 3%   493µs ± 2%    ~     (p=0.094 n=9+9)
SQL/Cockroach/Update/count=10-16              667µs ± 3%   673µs ± 5%    ~     (p=0.720 n=9+10)
SQL/Cockroach/Update/count=100-16            2.20ms ± 7%  2.13ms ± 4%  -3.07%  (p=0.019 n=10+10)
SQL/Cockroach/Update/count=1000-16           19.1ms ± 5%  19.1ms ± 4%    ~     (p=0.965 n=10+8)
SQL/MultinodeCockroach/Update/count=1-16      942µs ± 3%   958µs ± 3%    ~     (p=0.063 n=9+9)
SQL/MultinodeCockroach/Update/count=10-16    1.16ms ± 4%  1.20ms ± 3%  +3.87%  (p=0.011 n=9+9)
SQL/MultinodeCockroach/Update/count=100-16   2.84ms ± 4%  2.84ms ± 4%    ~     (p=0.959 n=8+8)
SQL/MultinodeCockroach/Update/count=1000-16  19.6ms ±11%  19.0ms ±11%    ~     (p=0.222 n=9+9) ```

pkg/sql/row/helper.go, line 318 at r5 (raw file):

Done. Removed.

pkg/sql/rowenc/index_encoding.go, line 1191 at r5 (raw file):

Done.

Looks great! I'll take another look once you add those tests.

Reviewed 1 of 4 files at r5, 1 of 1 files at r6, 4 of 4 files at r7, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @mgartner, @rhu713, and @stevendanna)

pkg/sql/delete_preserving_index_test.go, line 102 at r7 (raw file):

		if deletePreservingEncoding {
			// Mutate index descriptor
			//to use the delete-preserving encoding.

nit: space after // and wrap at the 80-character mark (this probably fits on one line)

pkg/sql/catalog/descpb/structured.proto, line 570 at r7 (raw file):

  // This is necessary to preserve the delete history for the MVCC-compatible
  // index backfiller
  // `docs/RFCS/20211004_incremental_index_backfiller.md#new-index-encoding-for-deletions-vs-mvcc`

nit: make it clear that if true, entries are not deleted from the index, but remain with the deleted bit set to true.

nit: remove the back ticks `

pkg/sql/catalog/tabledesc/index.go, line 341 at r7 (raw file):

// This is necessary to preserve the delete history for the MVCC-compatible
// index backfiller
// `docs/RFCS/20211004_incremental_index_backfiller.md#new-index-encoding-for-deletions-vs-mvcc`

ditto: explain delete behavior and remove backticks

pkg/sql/row/helper.go, line 94 at r1 (raw file):

I agree, this looks fine. Thanks for running the benchmarks!

I have mostly comments and questions, the only change I'm requesting pertains to the added field in descpb.IndexDescriptor.

@mgartner Just added upsert, update, and delete tests for partial, expression, inverted, and multicolumn inverted indices. Lmk if I've missed anything else.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @mgartner, @stevendanna, and @tbg)

pkg/sql/catalog/descpb/structured.proto, line 570 at r7 (raw file):

Done.

pkg/sql/catalog/tabledesc/index.go, line 341 at r7 (raw file):

Done.

pkg/sql/row/inserter.go, line 185 at r5 (raw file):

Resolved after running benchmarks.

pkg/sql/row/updater.go, line 508 at r5 (raw file):

Resolved after running benchmarks.

Nice work! The tests look great. I think it's worth adding a test for delete, update, and upsert with a "regular" index too - see my inline comments.

Reviewed 6 of 7 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @mgartner, @rhu713, @stevendanna, and @tbg)

pkg/sql/opt/exec/execbuilder/testdata/delete_nonmetamorphic, line 1 at r9 (raw file):

# LogicTest: local !metamorphic

I think it's worth adding a similar test for just a regular index like CREATE TABLE t (a INT PRIMARY KEY, b INT, c INT, INDEX (b, c)).

pkg/sql/opt/exec/execbuilder/testdata/delete_nonmetamorphic, line 12 at r9 (raw file):

    c STRING,
    FAMILY (a, b, c),
    CHECK (b > 0),

nit: remove this check -- I don't think it's necessary.

pkg/sql/opt/exec/execbuilder/testdata/delete_nonmetamorphic, line 17 at r9 (raw file):

let $t_id
select id from system.namespace where name='tpi'

nit: capitalize keywords SELECT FROM and WHERE

pkg/sql/opt/exec/execbuilder/testdata/delete_nonmetamorphic, line 71 at r9 (raw file):

statement ok
CREATE INDEX t_a_plus_b_idx ON tei ((a + b))

nit: you should be able to include this index in CREATE TABLE.

pkg/sql/opt/exec/execbuilder/testdata/delete_nonmetamorphic, line 74 at r9 (raw file):

let $t_id
select id from system.namespace where name='tei'

nit: capitalize keywords SELECT FROM and WHERE

pkg/sql/opt/exec/execbuilder/testdata/delete_nonmetamorphic, line 127 at r9 (raw file):

let $t_id
select id from system.namespace where name='tii'

nit: capitalize keywords SELECT FROM and WHERE

pkg/sql/opt/exec/execbuilder/testdata/delete_nonmetamorphic, line 183 at r9 (raw file):

let $t_id
select id from system.namespace where name='tmi'

nit: capitalize keywords SELECT FROM and WHERE

pkg/sql/opt/exec/execbuilder/testdata/update_nonmetamorphic, line 100 at r9 (raw file):

statement ok
SET enable_implicit_select_for_update = true

Some tests with a regular index would be good here too.

pkg/sql/opt/exec/execbuilder/testdata/update_nonmetamorphic, line 110 at r9 (raw file):

    c STRING,
    FAMILY (a, b, c),
    CHECK (b > 0),

nit: remove check

pkg/sql/opt/exec/execbuilder/testdata/update_nonmetamorphic, line 115 at r9 (raw file):

let $t_id
select id from system.namespace where name='tpi'

nit: capitalize keywords SELECT FROM and WHERE

pkg/sql/opt/exec/execbuilder/testdata/update_nonmetamorphic, line 195 at r9 (raw file):

statement ok
CREATE INDEX t_a_plus_b_idx ON tei ((a + b))

nit: you should be able to move into CREATE TABLE

pkg/sql/opt/exec/execbuilder/testdata/update_nonmetamorphic, line 198 at r9 (raw file):

let $t_id
select id from system.namespace where name='tei'

nit: capitalize keywords SELECT FROM and WHERE

pkg/sql/opt/exec/execbuilder/testdata/update_nonmetamorphic, line 259 at r9 (raw file):

let $t_id
select id from system.namespace where name='tii'

nit: capitalize keywords SELECT FROM and WHERE

pkg/sql/opt/exec/execbuilder/testdata/update_nonmetamorphic, line 314 at r9 (raw file):

let $t_id
select id from system.namespace where name='tmi'

nit: capitalize keywords SELECT FROM and WHERE

pkg/sql/opt/exec/execbuilder/testdata/upsert_nonmetamorphic, line 74 at r9 (raw file):

batch flow coordinator  CPut /Table/55/2/2/0 -> /BYTES/0x8a (expecting does not exist)
exec stmt               execution failed after 0 rows: duplicate key value violates unique constraint "woo"

ditto: add regular index case

pkg/sql/opt/exec/execbuilder/testdata/upsert_nonmetamorphic, line 85 at r9 (raw file):

    c STRING,
    FAMILY (a, b, c),
    CHECK (b > 0),

nit: remove check

pkg/sql/opt/exec/execbuilder/testdata/upsert_nonmetamorphic, line 90 at r9 (raw file):

let $t_id
select id from system.namespace where name='tpi'

nit: capitalize keywords SELECT FROM and WHERE

pkg/sql/opt/exec/execbuilder/testdata/upsert_nonmetamorphic, line 173 at r9 (raw file):

let $t_id
select id from system.namespace where name='tei'

nit: capitalize keywords SELECT FROM and WHERE

pkg/sql/opt/exec/execbuilder/testdata/upsert_nonmetamorphic, line 243 at r9 (raw file):

let $t_id
select id from system.namespace where name='tii'

nit: capitalize keywords SELECT FROM and WHERE

pkg/sql/opt/exec/execbuilder/testdata/upsert_nonmetamorphic, line 298 at r9 (raw file):

let $t_id
select id from system.namespace where name='tmi'

nit: capitalize keywords SELECT FROM and WHERE

Done. Added the regular index tests.

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

pkg/sql/catalog/descpb/structured.proto, line 571 at r7 (raw file):

Done. Added decision reasoning in the comment.

pkg/sql/catalog/descpb/structured.proto, line 1490 at r7 (raw file):

@dt @postamar I added a new pb directory called rowencpb to the rowenc package, and moved this proto to that. I tried moving it to execinfrapb but this resulted in an import cycle. Please take a look and let me know if this is the right way to resolve this.

pkg/sql/opt/exec/execbuilder/testdata/delete_nonmetamorphic, line 1 at r9 (raw file):

Done.

pkg/sql/opt/exec/execbuilder/testdata/delete_nonmetamorphic, line 12 at r9 (raw file):

Done.

pkg/sql/opt/exec/execbuilder/testdata/delete_nonmetamorphic, line 17 at r9 (raw file):

Done.

pkg/sql/opt/exec/execbuilder/testdata/delete_nonmetamorphic, line 71 at r9 (raw file):

Done.

pkg/sql/opt/exec/execbuilder/testdata/delete_nonmetamorphic, line 74 at r9 (raw file):

Done.

pkg/sql/opt/exec/execbuilder/testdata/delete_nonmetamorphic, line 127 at r9 (raw file):

Done.

pkg/sql/opt/exec/execbuilder/testdata/delete_nonmetamorphic, line 183 at r9 (raw file):

Done.

pkg/sql/opt/exec/execbuilder/testdata/update_nonmetamorphic, line 100 at r9 (raw file):

Done. Added regular index test.

pkg/sql/opt/exec/execbuilder/testdata/update_nonmetamorphic, line 110 at r9 (raw file):

Done.

pkg/sql/opt/exec/execbuilder/testdata/update_nonmetamorphic, line 115 at r9 (raw file):

Done.

pkg/sql/opt/exec/execbuilder/testdata/update_nonmetamorphic, line 195 at r9 (raw file):

Done.

pkg/sql/opt/exec/execbuilder/testdata/update_nonmetamorphic, line 198 at r9 (raw file):

Done.

pkg/sql/opt/exec/execbuilder/testdata/update_nonmetamorphic, line 259 at r9 (raw file):

Done.

pkg/sql/opt/exec/execbuilder/testdata/update_nonmetamorphic, line 314 at r9 (raw file):

Done.

pkg/sql/opt/exec/execbuilder/testdata/upsert_nonmetamorphic, line 74 at r9 (raw file):

Done.

pkg/sql/opt/exec/execbuilder/testdata/upsert_nonmetamorphic, line 85 at r9 (raw file):

Done.

pkg/sql/opt/exec/execbuilder/testdata/upsert_nonmetamorphic, line 90 at r9 (raw file):

Done.

pkg/sql/opt/exec/execbuilder/testdata/upsert_nonmetamorphic, line 173 at r9 (raw file):

Done.

pkg/sql/opt/exec/execbuilder/testdata/upsert_nonmetamorphic, line 243 at r9 (raw file):

Done.

pkg/sql/opt/exec/execbuilder/testdata/upsert_nonmetamorphic, line 298 at r9 (raw file):

Done.

Sorry the the delay!!!

Reviewed 10 of 10 files at r10, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dt, @mgartner, @postamar, @stevendanna, and @tbg)