*: Use CPut when writing to `system.descriptor` table #88844

Xiang-Gu · 2022-09-27T18:40:02Z

Each individual commit has a rather thorough message, so I encourage
reviewers to read them as well.

Commit 1: code pattern, non-functional changes
Commit 2: Added a []byte field in descriptor builder
and immutable struct. We also modified existing functions
to carry over this field between descriptor and Mutable/immutable
descriptor, and vice versa.

Commit 3: Upgrade WriteDescToBatch to use CPut rather than
Put to prevent clobbering of the system.descriptor table.
We get the expected value for the CPut when first reading the
descriptor into collection from storage, as well as keep booking
the descriptor's latest change inside the uncommitted descriptor
set, in case more we write to the same descriptor more than once
in the same transaction.

I didn't add any tests in this PR since this changed function
WriteDescToBatch is heavily invoked for many sql stmts,
so I think we should be pretty confident about its correctness
if it passes all existing tests (unit, logic, and roachtest).

Release note: None

cockroach-teamcity · 2022-09-27T18:40:12Z

This change is

postamar · 2022-09-27T18:53:02Z

pkg/sql/catalog/tabledesc/structured.go

@@ -49,7 +49,7 @@ import (
 // Mutable is a custom type for TableDescriptors
 // going through schema mutations.
 type Mutable struct {
-	wrapper
+	immutable


This change in this whole commit should, in my opinion. go completely in the other way. We shouldn't name things as immutable when, in fact, they're not immutable.

I agree! For now, I've reverted this commit.

While I'm on this, I think I can have a PR that clears this part of code a bit, maybe after your #88786 is merged where we use the following pattern for all types of descriptors

type wrapper struct { descpb.(Table|Schema|Database|Type|Function)Descriptor other_fields } type immutable struct { wrapper // no other fields should be here! } type Mutable struct { wrapper original *immutable }

And, wrapper will be the base implementation for catalog.Descriptor, which means we should only have functions whose receivers are *wrapper and *Mutable but not *immutable.

postamar · 2022-09-27T19:01:40Z

pkg/sql/catalog/descriptor.go

@@ -88,6 +88,8 @@ type DescriptorBuilder interface {
 	// upgrade migrations
 	RunRestoreChanges(descLookupFn func(id descpb.ID) Descriptor) error

+	SetRawBytesInDescriptor(rawBytes []byte)


Can we pass the raw bytes to the DescriptorBuilder constructors instead? Either you create the builder from another descriptor and that byte slice is available there, or you create it from a protobuf and the bytes are usually not far. In any case, we have to pass them along.

I think this PR might benefit from being rebased on #88786 which tightens how these builders can be created.

The difficulty would be when we upsert a descriptor to the uncommitted descriptor set, we will need to update this rawBytesInStorage field, instead of passing it along. This is the main reason I choose to have a setter on this field.

pkg/sql/catalog/descs/uncommitted_descriptors.go

This will be needed when I later added a field to the function builder which we wish to be able to set it.

Xiang-Gu · 2022-10-03T02:27:10Z

@postamar I rebased and polished a bit more on your recent merge. I fixed a few test failures discovered from CI -- they're because I forgot to properly initialize this field for unsafe_upsert_descriptor.

I think this is RFAL :)

postamar

LGTM conditional on addressing the minor comments. Nicely done.

postamar · 2022-10-03T15:06:24Z

pkg/sql/catalog/dbdesc/database_desc.go

+		return nil
+	}
+	return desc.rawBytesInStorage
+}


Consider deep-copying the byte slice, either here or in the builder. Notice that the builder already deep-copies the protobuf in almost all cases except for an edge case on the hot path. The memory overhead is not a concern outside of this edge case.

postamar · 2022-10-03T15:08:29Z

pkg/sql/catalog/descs/collection.go

+	// which means it has existed in `tc.uncommitted`, we will retrieve the expected
+	// bytes from there.
+	var expected []byte
+	if exist := tc.uncommitted.uncommitted.GetByID(desc.GetID()); exist == nil {


nit: use getUncommittedByID here instead

uber nit: perhaps it's just me but evaluating the nil case first in this if-block feels weird to me.

postamar · 2022-10-03T15:10:03Z

pkg/sql/catalog/descs/collection_test.go

@@ -284,7 +284,7 @@ func TestAddUncommittedDescriptorAndMutableResolution(t *testing.T) {
 			immByNameAfter, err := descriptors.GetImmutableDatabaseByName(ctx, txn, "new_name", flags)
 			require.NoError(t, err)
 			require.Equal(t, db.GetVersion(), immByNameAfter.GetVersion())
-			require.Equal(t, mut.ImmutableCopy(), immByNameAfter)
+			require.Equal(t, mut.ImmutableCopy().(catalog.DatabaseDescriptor).DatabaseDesc(), immByNameAfter.DatabaseDesc())


nit: if you want to avoid the cast here, you can also compare the results of .DescriptorProto() to the same effect.

postamar · 2022-10-03T15:16:59Z

pkg/sql/catalog/descs/uncommitted_descriptors.go

+	if err = val.SetProto(mut.DescriptorProto()); err != nil {
+		return err
+	}
+	rawBytesInStorage := val.TagAndDataBytes()


nit: rename this to rawBytesInStorageAfterPendingWrite perhaps?

postamar · 2022-10-03T15:17:46Z

pkg/sql/catalog/descs/uncommitted_descriptors.go

+	// compute what the raw bytes of this descriptor in storage is going to
+	// look like when that write happens, so that any further update to this
+	// descriptor, which will be retrieved from the "uncommitted" set, will
+	// carry the correct raw bytes in storage with it.


I'm fine with this, it's rather elegant.

postamar · 2022-10-03T15:20:16Z

pkg/sql/catalog/tabledesc/structured.go

+func (desc *Mutable) GetRawBytesInStorage() []byte {
+	if desc == nil {
+		return nil
+	}


desc can never be nil here, or in other Descriptor implementationgs

This PR added a `[]byte` filed (called `rawBytesInStorage`) in the [table|schema|database|type|function] immutable descriptor struct and the [table|schema|database|type|function] descriptor builder struct. The purpose is to faciliate the next commit where we want to do book keeping of what's actually in storage of each descriptor, so that we can use a CPut when updating a descriptor in storage. See next commit for more details. Accompanying this added field is two methods added: - `GetRawBytesInStorage() []byte` in `catalog.Descriptor`, and - `SetRawBytesInStorage([]byte)` in `catalog.DescriptorBuilder` They serve as the getter and setter of this field. We also modified method that gives a builder from a descriptor to carry over this field. The way this byte slice field will be initialized and carried over in a builder and a descriptor will be something like the following graph, where `XXX` denotes an entity, `xxx` denotes an action, and `==>` denotes the direction of action. DESCPB.DESCRIPTOR | | | | construct a builder | | | | V CATALOG.BUILDER | | | | set byte slice field | | | | V CATALOG.BUILDER (with the byte slice field set) | | | | build a desc | | | | V MUTABLEDESCRIPTOR or DESCRIPTOR (with the byte slice field carried over from the builder) | | | | new builder | | | | V CATALOG.BUILDER (with the byte slice field carried over from the descriptor) | | | | ...

Previously, `WriteDescToBatch`, which is called to update a descriptor in storage by a `desc.Collection`, uses `Put` primitive to directly modify the storage layer. We would like to tighten it to use a `CPut` to prevent (in)advertent clobbering of that table. This PR does that by book-keeping the raw bytes of the to-be-updated descriptor in the descriptor, acquired when we first read it into the `desc.Collection` from the storage layer. Then, the infrastructural work done in the previous commit allows us to carry over these raw bytes as we are preparing the `MutableDescriptor` that we pass into this `WriteDescToBatch` method. One additional difficulty is that what if we will need to call `WriteDescToBatch` more than once in one transaction. For example, in the new schema changer, statement phase and precommit phase will both be in one transaction, but we call `WriteDescToBatch` at the end of each stage. Hence, for some DDL stmts (e.g. `DROP COLUMN`), we will call `WriteDescToBatch` twice in one transaction. The first call will modify the descriptor in storage and also added this descriptor to `desc.Collection.uncommitted` set, so, the second call will get it from there. To make `CPut` work correctly for the second call, we will need to get the expected byte slice from the `uncommitted` descriptor set. This motivates the change to update a descriptor's byte slice field when it's added to the `uncommitted` descriptor sett.

Xiang-Gu

Thanks for the review! I've addressed the feedback.

bors r+

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

pkg/sql/catalog/dbdesc/database_desc.go line 352 at r6 (raw file):