catalog, catalogkv: add and use descriptor builder #61429

postamar · 2021-03-03T21:53:15Z

catalog,catalogkv: add and use descriptor builder

Release justification: bug fixes and low-risk updates to new
functionality.

This commit is, at heart, a refactor of how catalog.Descriptor objects
are created; either when read from storage via the catalogkv API
or when constructed from an in-memory descpb.*Descriptor struct.

The original motivation for this change was the fact that previously,
calling the descriptor validation methods could sometimes modify the
descriptor itself. Specifically, ValidateSelf would perform "privilege
surgery" and modify the privileges descriptor. This appears to have been
around since version 2 and is, in fact, responsible for a few bugs,
which this commit also fixes.

Taking a broader perspective it appears necessary that all such
descriptor surgeries should be performed together when read from
storage regardless of descriptor subtype (table, db, type, schema).
These surgeries are typically no-ops: table format upgrade, foreign key
representation upgrade, etc. Considering that catalog.Descriptor objects
can be created either from storage or from memory, this involves:
  1. unifying the constructors in the (table|db|type|schema)desc
     packages,
  2. unifying the descriptor unwrapping logic in catalogkv.

To address (1) this commit introduces catalog.DescriptorBuilder, a
builder interface which replaces the NewImmutable, NewExistingMutable,
NewCreatedMutable, etc. constructors. The builder does the same thing as
these except it uses a deep copy of the proto descriptor instead of
a shallow copy. This has, in itself, uncovered a few minor bugs.
Still, the main reason for the existence of this builder object is that
we can now perform all post-deserialization descriptor changes in a
method to be called before building the catalog.Descriptor object
proper.

This commit also changes how we update the descriptor modification time
using the MVCC timestamp. This is done at builder creation time, or, in
cases where we stick to descpb structs, using a new
descp.FromDescriptorWithMVCCTimestamp function which replaces
descpb.TableFromDescriptor and the descpb.Descriptor.GetTable,
GetDatabase, GetType and GetSchema methods. These methods are now all
forbidden from casual use by a new linter check (it used to be just
GetTable).

To address (2) this commit uses the new builder to unify all descriptor
wrapping logic in catalogkv into a new descriptorFromKeyValue function.
This function is what backs the catalogkv API now. This commit also
changes the API somewhat, the old GetDescriptorFromID function with its
many flag arguments is gone in favour of many lightweight public getter
functions (complementing existing ones) which all delegate to a common
private getDescriptorByID function. The most important side benefit to
doing all this is that we now no longer skip cross-reference validation
when getting mutable descriptors through the catalogkv API.
Cross-reference validation is now the default, and the exceptions to
this are few:
  - The catalogkv DescGetter implemementation, aka the
    oneLevelUncachedDescGetter, does not perform these checks, because
    they, in turn, require a DescGetter. In theory we could get around
    this infinite recursion with caching but right now there's no point.
  - GetAllDescriptorsUnvalidated explicitly performs no validation at
    all.

Doing all of this has uncovered a few bugs, the most serious of which is
that the protected_ts_meta system table didn't have the correct
permissions applied.

Furthermore this commit also unifies namespace collision detection logic
in catalogkv. A side-effect of this is that error messages for PG codes
42710 and 42P07 contain qualified names more often than before.

Release note (bug fix): Fixed privileges for system.protected_ts_meta.

cockroach-teamcity · 2021-03-03T21:53:22Z

This change is

postamar · 2021-03-05T22:38:18Z

pkg/ccl/backupccl/backup_test.go

@@ -2364,9 +2364,9 @@ CREATE TABLE d.t1 (x d.farewell);
 RESTORE DATABASE d FROM 'nodelocal://0/rev-history-backup'
  AS OF SYSTEM TIME %s
 `, ts1))
-			sqlDB.ExpectErr(t, `pq: type "d.public.farewell" already exists`,
+			sqlDB.ExpectErr(t, `pq: type "farewell" already exists`,


This message change and others like it are caused by having unified the descriptor collision logic in CheckObjectCollision in catalogkv.

postamar · 2021-03-05T22:42:20Z

pkg/ccl/backupccl/backupbase/targets_test.go

-			return desc
+		mkTable := func(descriptor tbDesc) catalog.Descriptor {
+			descProto := tabledesc.NewBuilder(&descriptor).BuildImmutable().DescriptorProto()
+			return catalogkv.NewBuilderWithMVCCTimestamp(descProto, ts1).BuildImmutable()


It's not necessary to use NewBuilderWithMVCCTimestamp here, strictly speaking, but it's the nice way of maybe-setting a modification time.

postamar · 2021-03-05T22:45:11Z

pkg/ccl/backupccl/restore_job.go

-			descs = append(descs, db)
-		}
-		return catalog.ValidateSelfAndCrossReferences(ctx, bdg, descs...)
+		return nil


I looked into this in detail while chasing an unrelated bug and realized that there's no point to these checks here because the validation-on-write checks kick in very soon after.

postamar · 2021-03-05T22:50:01Z

pkg/ccl/backupccl/restore_job.go

-
-	var updatedPrivileges *descpb.PrivilegeDescriptor
-
+) (updatedPrivileges *descpb.PrivilegeDescriptor, err error) {


I believe this is a bug, we still have to update privileges on system tables when doing a cluster restore.

postamar · 2021-03-05T22:51:40Z

pkg/ccl/backupccl/show.go

-	var privDesc *descpb.PrivilegeDescriptor
+	b := catalogkv.NewBuilder(descriptor)
+	if b == nil {
+		return ""


This happens when descriptor.Union == nil.

postamar · 2021-03-05T22:53:19Z

pkg/ccl/backupccl/targets.go

-	}
-	return nil
-}
-


I replaced this with CheckObjectCollision because it's the same thing.

postamar · 2021-03-05T22:55:24Z

pkg/ccl/importccl/import_processor_test.go

@@ -914,15 +914,15 @@ func newTestSpec(
 	switch format.Format {
 	case roachpb.IOFileFormat_CSV:
 		descr = descForTable(ctx, t,
-			"CREATE TABLE simple (i INT PRIMARY KEY, s text )", 10, 20, NoFKs)
+			"CREATE TABLE simple (i INT PRIMARY KEY, s text )", 100, 200, NoFKs)


These descriptor IDs were in the system range and this caused privilege validation failures.

postamar · 2021-03-05T22:57:11Z

pkg/ccl/streamingccl/streamclient/random_stream_client.go

@@ -223,7 +224,7 @@ func (m *randomStreamClient) getDescriptorAndNamespaceKVForTableID(
 		IngestionDatabaseID,
 		tableID,
 		fmt.Sprintf(RandomStreamSchemaPlaceholder, tableName),
-		&descpb.PrivilegeDescriptor{},
+		descpb.NewDefaultPrivilegeDescriptor(security.RootUserName()),


Now that we don't insta-upgrade our privilege descriptors when validating, we have to be more careful when crafting test cases.

postamar · 2021-03-05T22:59:30Z

pkg/sql/alter_database.go

-	if err != nil {
-		return err
-	}
-


This needed to be moved a tiny bit further up or else there would be cross-reference validation failures!

postamar · 2021-03-05T23:00:26Z

pkg/sql/alter_table_set_schema.go

@@ -80,7 +80,7 @@ func (p *planner) AlterTableSetSchema(
 	for _, dependent := range tableDesc.DependedOnBy {
 		if !dependent.ByID {
 			return nil, p.dependentViewError(
-				ctx, tableDesc.TypeName(), tableDesc.Name,
+				ctx, string(tableDesc.TypeName()), tableDesc.Name,


TypeName() now returns a catalog.DescriptorType which aliases string, for extra type safety.

postamar · 2021-03-05T23:02:17Z

pkg/sql/backfill.go

@@ -802,7 +802,11 @@ func TruncateInterleavedIndexes(
 		// All the data chunks have been removed. Now also removed the
 		// zone configs for the dropped indexes, if any.
 		if err := db.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
-			return RemoveIndexZoneConfigs(ctx, txn, execCfg, table.GetParentID(), indexes)


I'm certain that table.GetParentID() instead of table.GetID() is a bug here but found no way to test it. It only involves zone configs on interleaved indexes so I'm not super motivated to do more.

postamar · 2021-03-05T23:04:43Z

pkg/sql/backfill.go

+			if err != nil {
+				return err
+			}
+			return RemoveIndexZoneConfigs(ctx, txn, execCfg, freshTableDesc, indexes)


I changed the signature of RemoveIndexZoneConfigs to take a catalog.TableDescriptor because otherwise it loads it from storage using a catalogkv api call which causes cross-reference validation check failures in indexTruncateInTxn.
This is another of these new failures which cropped up after I raised the validation level for mutable descs.

postamar · 2021-03-05T23:05:18Z

pkg/sql/set_zone_config.go

 	indexDescs []descpb.IndexDescriptor,
 ) error {
 	if !execCfg.Codec.ForSystemTenant() {
 		// Tenants are agnostic to zone configs.
 		return nil
 	}
-	desc, err := catalogkv.GetDescriptorByID(ctx, txn, execCfg.Codec, tableID,
-		catalogkv.Mutable, catalogkv.TableDescriptorKind, true)


see https://github.com/cockroachdb/cockroach/pull/61429/files#r588774574

postamar · 2021-03-05T23:07:57Z

pkg/sql/catalog/descpb/privilege.go

-			return errors.AssertionFailedf("found no owner for system %s with ID=%d",
-				objectType, id)
+			return errors.AssertionFailedf("found no owner for %s%s with ID=%d",
+				maybeSystem, objectType, id)


For some reason the error messages here describe everything as "system" even when they're not system tables or anything.

postamar · 2021-03-05T23:09:51Z

pkg/sql/catalog/systemschema/system.go

@@ -1423,12 +1428,12 @@ var (
 		},
 		NextIndexID: 2,
 		Privileges: descpb.NewCustomSuperuserPrivilegeDescriptor(
-			descpb.SystemAllowedPrivileges[keys.ReplicationStatsTableID], security.NodeUserName()),
+			descpb.SystemAllowedPrivileges[keys.ProtectedTimestampsMetaTableID], security.NodeUserName()),


bad copy pasta

postamar · 2021-03-05T23:13:12Z

pkg/sql/opt/optbuilder/scope.go

@@ -35,7 +35,7 @@ import (
 // been bound within the current scope as columnProps. Variables bound in the
 // parent scope are also visible in this scope.
 //
-// See builder.go for more details.
+// See table_desc_builder.go for more details.


I'll fix this

postamar · 2021-03-05T23:13:37Z

pkg/sql/rename_table.go

-	} else if err != nil {
-		return err
-	}
-


This check is entirely redundant.

postamar · 2021-03-05T23:15:03Z

pkg/sql/repair.go

@@ -569,7 +567,7 @@ func (p *planner) UnsafeDeleteDescriptor(ctx context.Context, descID int64, forc
 	mut, err := p.Descriptors().GetMutableDescriptorByID(ctx, id, p.txn)
 	var forceNoticeString string // for the event
 	if err != nil {
-		if !errors.Is(err, catalog.ErrDescriptorNotFound) && force {
+		if force {


I think force is enough here. The other test was failing because ErrDescriptorNotFound was being returned by a failing back-reference check.

postamar · 2021-03-05T23:17:22Z

pkg/sql/show_create.go

+		desc, err = lCtx.getTableByID(desc.GetID())
+		if err != nil {
+			return "", err
+		}


Previously this used to work without this because we were shallow-copying on descriptor construction. Deep-copy makes this necessary.

postamar · 2021-03-05T23:17:48Z

pkg/sql/tests/repair_test.go

        { "privileges": 2, "user_proto": "newuser1" },
-        { "privileges": 2, "user_proto": "newuser2" }
+        { "privileges": 2, "user_proto": "newuser2" },
+        { "privileges": 2, "user_proto": "root" }


Apparently order matters here.

postamar · 2021-03-05T23:18:13Z

pkg/sql/tests/repair_test.go

@@ -103,7 +103,7 @@ func TestDescriptorRepairOrphanedDescriptors(t *testing.T) {
 		// the dance of adding back a parent database in order to drop the table.
 		require.NoError(t, crdb.ExecuteTx(ctx, db, nil, func(tx *gosql.Tx) error {
 			if _, err := tx.Exec(
-				"SELECT crdb_internal.unsafe_delete_descriptor($1);",
+				"SELECT crdb_internal.unsafe_delete_descriptor($1, true);",


Required due to more stringent validation-on-read.

postamar · 2021-03-05T23:19:10Z

pkg/sql/catalog/tabledesc/table_desc_builder.go

+	dg catalog.DescGetter,
+	desc *descpb.TableDescriptor,
+	skipFKsWithNoMatchingTable bool,
+) (changes PostDeserializationTableDescriptorChanges, err error) {


moved as-is from structured.go, along with dependent functions

postamar · 2021-03-05T23:20:12Z

pkg/sql/catalog/catalogkv/desc_getter.go

@@ -0,0 +1,91 @@
+// Copyright 2021 The Cockroach Authors.


moved all of this out of catalogkv.go, only big new thing is fromKeyValue method

ajwerner

Reviewed 148 of 153 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @postamar)

pkg/ccl/backupccl/backup_test.go, line 2367 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

This message change and others like it are caused by having unified the descriptor collision logic in CheckObjectCollision in catalogkv.

aye, not great though; now the error can be pretty ambiguous.

pkg/ccl/backupccl/restore_job.go, line 436 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

I looked into this in detail while chasing an unrelated bug and realized that there's no point to these checks here because the validation-on-write checks kick in very soon after.

nice.

pkg/ccl/backupccl/restore_job.go, line 2069 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

I believe this is a bug, we still have to update privileges on system tables when doing a cluster restore.

say more?

pkg/ccl/backupccl/targets.go, line 194 at r1 (raw file):

				// Collect the prior IDs of table descriptors, as the ID may have been
				// changed during truncate.

can you add a prior to 20.2 after truncate. We no longer change the IDs.

pkg/ccl/changefeedccl/schemafeed/schema_feed.go, line 624 at r1 (raw file):

				b := catalogkv.NewBuilderWithMVCCTimestamp(&desc, k.Timestamp)
				if b != nil && (b.TypeName() == catalog.Table || b.TypeName() == catalog.Database) {

should this second one be catalog.Type?

pkg/sql/show_create.go, line 263 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

Previously this used to work without this because we were shallow-copying on descriptor construction. Deep-copy makes this necessary.

oh man

pkg/sql/catalog/descriptor.go, line 56 at r1 (raw file):

	// TypeName returns a symbol identifying the type of the descriptor
	// built by this builder.
	TypeName() DescriptorType

nit: how about you call this method DescriptorType()?

pkg/sql/catalog/descriptor.go, line 60 at r1 (raw file):

	// RunPostDeserializationChanges attempts to perform post-deserialization
	// changes to the descriptor being built.
	RunPostDeserializationChanges(ctx context.Context, dg DescGetter) error

can you comment on the contract with the DescGetter?

pkg/sql/catalog/descriptor.go, line 109 at r1 (raw file):

	GetPrivileges() *descpb.PrivilegeDescriptor
	TypeName() DescriptorType

how about calling this DescriptorType() too?

pkg/sql/catalog/catalogkv/catalogkv.go, line 176 at r1 (raw file):

// builderFromKeyValue is a utility function for descriptorFromKeyValue which
// unmarshals the proto  and checks that it exists and that it matches the

nit: extra space

pkg/sql/catalog/catalogkv/unwrap_validation_test.go, line 65 at r1 (raw file):

		desc, err := m.GetDesc(context.Background(), id)
		require.NoErrorf(t, err, "id: %d", id)
		require.NotNil(t, desc, "id: %d", id)

NotNilf?

pkg/sql/catalog/descpb/descriptor.go, line 200 at r1 (raw file):

// FromDescriptorWithMVCCTimestamp is a replacement for
// Get(Table|Database|TypeSchema)() methods which seeks to ensure that clients

Type|Schema?

pkg/sql/catalog/descpb/privilege.go, line 302 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

For some reason the error messages here describe everything as "system" even when they're not system tables or anything.

I'm sure it's just code rot.

pkg/sql/catalog/descpb/privilege_test.go, line 353 at r1 (raw file):

	rootWrongPrivilegesErr := "user root must have exactly SELECT, GRANT " +
		"privileges on s?y?s?t?e?m? ?table with ID=.*"

why not just (system )?

pkg/sql/catalog/systemschema/system.go, line 1431 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

bad copy pasta

🤦

pkg/sql/logictest/testdata/logic_test/enums, line 28 at r1 (raw file):

SELECT * FROM db2.t

statement error pq: type "t" already exists

this highlights something interesting. Getting the error handling right here so that the right level understands enough context to construct a detailed error is not currently very easy.

pkg/sql/rowexec/tablereader.go, line 91 at r1 (raw file):

	tr.maxTimestampAge = time.Duration(spec.MaxTimestampAgeNanos)

	tableDesc := tabledesc.NewBuilder(&spec.Table).BuildImmutableTable()

so, I feel like the copying behavior of the builder is going to hurt here. We end up calling this right here a lot. We should fix that so that instead we look up a cached descriptor but this new set of allocations per table reader most gives me pause to backporting this.

ajwerner

Reviewed 2 of 153 files at r1, 87 of 87 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dt and @postamar)

postamar

Thanks for the review. Although we discussed things verbally I'll still respond to the comments here for the record.

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

pkg/ccl/backupccl/backup_test.go, line 2367 at r1 (raw file):

Previously, ajwerner wrote…

aye, not great though; now the error can be pretty ambiguous.

Fixed by having CheckObjectCollision take a tree.ObjectName instead.

pkg/ccl/backupccl/restore_job.go, line 2069 at r1 (raw file):

Previously, ajwerner wrote…

say more?

To clarify: the existing behaviour was correct except when restoring to the temporary system DB. Those backed-up tables came from system tables and as such may not have r+w permissions for superusers, causing validation to freak out.

We never noticed this until now because r+w permissions for superusers used to be reset by the privilege descriptor surgery in ValidateSelf.

This change here grants those tables the same permissions as restoreTempSystemDB which is good enough, the contents of these restored tables get inserted into the real system tables later on.

pkg/ccl/backupccl/targets.go, line 194 at r1 (raw file):

Previously, ajwerner wrote…

can you add a prior to 20.2 after truncate. We no longer change the IDs.

Done.

pkg/ccl/changefeedccl/schemafeed/schema_feed.go, line 624 at r1 (raw file):

Previously, ajwerner wrote…

should this second one be catalog.Type?

Fixed.

pkg/sql/catalog/descriptor.go, line 56 at r1 (raw file):

Previously, ajwerner wrote…

nit: how about you call this method DescriptorType()?

Done.

pkg/sql/catalog/descriptor.go, line 60 at r1 (raw file):

Previously, ajwerner wrote…

can you comment on the contract with the DescGetter?

Done.

pkg/sql/catalog/descriptor.go, line 109 at r1 (raw file):

Previously, ajwerner wrote…

how about calling this DescriptorType() too?

Done.

pkg/sql/catalog/catalogkv/catalogkv.go, line 176 at r1 (raw file):

Previously, ajwerner wrote…

nit: extra space

Done.

pkg/sql/catalog/catalogkv/unwrap_validation_test.go, line 65 at r1 (raw file):

Previously, ajwerner wrote…

NotNilf?

Done.

pkg/sql/catalog/descpb/descriptor.go, line 200 at r1 (raw file):

Previously, ajwerner wrote…

Type|Schema?

Done.

pkg/sql/catalog/descpb/privilege_test.go, line 353 at r1 (raw file):

Previously, ajwerner wrote…

why not just (system )?

Done

pkg/sql/rowexec/tablereader.go, line 91 at r1 (raw file):

Previously, ajwerner wrote…

so, I feel like the copying behavior of the builder is going to hurt here. We end up calling this right here a lot. We should fix that so that instead we look up a cached descriptor but this new set of allocations per table reader most gives me pause to backporting this.

I ran the following microbenchmark: BenchmarkKV/SQL with the kv table modified to gratuitously fatten its descriptor. I did this by adding an extra 10 stored computed columns and an extra 10 secondary indexes. Using the master branch as a control I found the difference in allocs/op to not be significant. I was honestly expecting it to be worse than that. My working hypothesis right now is that the cost of these deep-copy allocations is amortized by the other allocations we already do when creating a tabledesc.immutable.

postamar · 2021-03-11T16:06:55Z

bors r+

Release justification: bug fixes and low-risk updates to new functionality. This commit is, at heart, a refactor of how catalog.Descriptor objects are created; either when read from storage via the catalogkv API or when constructed from an in-memory descpb.*Descriptor struct. The original motivation for this change was the fact that previously, calling the descriptor validation methods could sometimes modify the descriptor itself. Specifically, ValidateSelf would perform "privilege surgery" and modify the privileges descriptor. This appears to have been around since version 2 and is, in fact, responsible for a few bugs, which this commit also fixes. Taking a broader perspective it appears necessary that all such descriptor surgeries should be performed together when read from storage regardless of descriptor subtype (table, db, type, schema). These surgeries are typically no-ops: table format upgrade, foreign key representation upgrade, etc. Considering that catalog.Descriptor objects can be created either from storage or from memory, this involves: 1. unifying the constructors in the (table|db|type|schema)desc packages, 2. unifying the descriptor unwrapping logic in catalogkv. To address (1) this commit introduces catalog.DescriptorBuilder, a builder interface which replaces the NewImmutable, NewExistingMutable, NewCreatedMutable, etc. constructors. The builder does the same thing as these except it uses a deep copy of the proto descriptor instead of a shallow copy. This has, in itself, uncovered a few minor bugs. Still, the main reason for the existence of this builder object is that we can now perform all post-deserialization descriptor changes in a method to be called before building the catalog.Descriptor object proper. This commit also changes how we update the descriptor modification time using the MVCC timestamp. This is done at builder creation time, or, in cases where we stick to descpb structs, using a new descp.FromDescriptorWithMVCCTimestamp function which replaces descpb.TableFromDescriptor and the descpb.Descriptor.GetTable, GetDatabase, GetType and GetSchema methods. These methods are now all forbidden from casual use by a new linter check (it used to be just GetTable). To address (2) this commit uses the new builder to unify all descriptor wrapping logic in catalogkv into a new descriptorFromKeyValue function. This function is what backs the catalogkv API now. This commit also changes the API somewhat, the old GetDescriptorFromID function with its many flag arguments is gone in favour of many lightweight public getter functions (complementing existing ones) which all delegate to a common private getDescriptorByID function. The most important side benefit to doing all this is that we now no longer skip cross-reference validation when getting mutable descriptors through the catalogkv API. Cross-reference validation is now the default, and the exceptions to this are few: - The catalogkv DescGetter implemementation, aka the oneLevelUncachedDescGetter, does not perform these checks, because they, in turn, require a DescGetter. In theory we could get around this infinite recursion with caching but right now there's no point. - GetAllDescriptorsUnvalidated explicitly performs no validation at all. Doing all of this has uncovered a few bugs, the most serious of which is that the protected_ts_meta system table didn't have the correct permissions applied. Furthermore this commit also unifies namespace collision detection logic in catalogkv. A side-effect of this is that error messages for PG codes 42710 and 42P07 contain qualified names more often than before. Release note (bug fix): Fixed privileges for system.protected_ts_meta.

craig · 2021-03-11T17:01:11Z

Canceled.

postamar · 2021-03-11T17:01:40Z

rebased to fix merge conflict

postamar · 2021-03-11T17:04:24Z

bors r+

craig · 2021-03-11T18:58:48Z

Build succeeded:

GitHub CI (Cockroach)

postamar changed the title ~~sql: add descriptor builder~~ [WIP] sql: add descriptor builder Mar 3, 2021

postamar force-pushed the descriptor-builder branch 2 times, most recently from 7499e1e to 151dd95 Compare March 4, 2021 22:04

postamar changed the title ~~[WIP] sql: add descriptor builder~~ catalog: add descriptor builder Mar 4, 2021

postamar force-pushed the descriptor-builder branch 2 times, most recently from ae77b36 to 3c3e135 Compare March 5, 2021 21:12

postamar changed the title ~~catalog: add descriptor builder~~ catalog, catalogkv: add and use descriptor builder Mar 5, 2021

postamar force-pushed the descriptor-builder branch 3 times, most recently from 3dd8fbd to 1cbd4ba Compare March 5, 2021 22:34

postamar commented Mar 5, 2021

View reviewed changes

pkg/ccl/backupccl/targets.go

}

return nil

}

Copy link

Contributor Author

postamar Mar 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replaced this with CheckObjectCollision because it's the same thing.

postamar commented Mar 5, 2021

View reviewed changes

pkg/sql/rename_table.go

} else if err != nil {

return err

}

Copy link

Contributor Author

postamar Mar 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is entirely redundant.

postamar commented Mar 5, 2021

View reviewed changes

postamar marked this pull request as ready for review March 5, 2021 23:21

postamar requested a review from a team March 5, 2021 23:21

postamar requested review from a team as code owners March 5, 2021 23:21

postamar requested review from dt and a team and removed request for a team March 5, 2021 23:21

ajwerner reviewed Mar 9, 2021

View reviewed changes

postamar force-pushed the descriptor-builder branch from 1cbd4ba to 5a83477 Compare March 10, 2021 02:36

ajwerner approved these changes Mar 11, 2021

View reviewed changes

postamar mentioned this pull request Mar 11, 2021

release-21.1: catalog, catalogkv: add and use descriptor builder #61842

Merged

postamar commented Mar 11, 2021

View reviewed changes

postamar force-pushed the descriptor-builder branch from 5a83477 to 15ee387 Compare March 11, 2021 17:01

craig bot merged commit 2b8d784 into cockroachdb:master Mar 11, 2021

erikgrinaker mentioned this pull request Mar 23, 2021

20.2 → 21.1 roachperf benchmark regressions #62322

Closed

15 tasks

postamar mentioned this pull request Mar 29, 2021

sql,descpb: remove descpb.MaybeFixPrivileges #61070

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

catalog, catalogkv: add and use descriptor builder #61429

catalog, catalogkv: add and use descriptor builder #61429

postamar commented Mar 3, 2021 •

edited

Loading

cockroach-teamcity commented Mar 3, 2021

postamar Mar 5, 2021

postamar Mar 5, 2021

postamar Mar 5, 2021

postamar Mar 5, 2021

postamar Mar 5, 2021

postamar Mar 5, 2021

postamar Mar 5, 2021

postamar Mar 5, 2021

postamar Mar 5, 2021

postamar Mar 5, 2021

postamar Mar 5, 2021

postamar Mar 5, 2021

postamar Mar 5, 2021

postamar Mar 5, 2021

postamar Mar 5, 2021

postamar Mar 5, 2021

postamar Mar 5, 2021

postamar Mar 5, 2021

postamar Mar 5, 2021

postamar Mar 5, 2021

postamar Mar 5, 2021

postamar Mar 5, 2021

postamar Mar 5, 2021

ajwerner left a comment

ajwerner left a comment

postamar left a comment

postamar commented Mar 11, 2021

craig bot commented Mar 11, 2021

postamar commented Mar 11, 2021

postamar commented Mar 11, 2021

craig bot commented Mar 11, 2021


		var updatedPrivileges *descpb.PrivilegeDescriptor

		) (updatedPrivileges *descpb.PrivilegeDescriptor, err error) {

catalog, catalogkv: add and use descriptor builder #61429

catalog, catalogkv: add and use descriptor builder #61429

Conversation

postamar commented Mar 3, 2021 • edited Loading

cockroach-teamcity commented Mar 3, 2021

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

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

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

postamar left a comment

Choose a reason for hiding this comment

postamar commented Mar 11, 2021

craig bot commented Mar 11, 2021

postamar commented Mar 11, 2021

postamar commented Mar 11, 2021

craig bot commented Mar 11, 2021

postamar commented Mar 3, 2021 •

edited

Loading