sql/catalog: avoid making copies of table descriptors

[fqazi]

Previously, we would copy table descriptors even if no PostDeserialization changes would apply eagerly. This could lead to extra over head when accessing a large number of tables in the pg_catalog tables. To address this, this patch will make a copy only if we need to modify the original descriptor because of a post deserialization change.

Fixes: #121579

Release note: None

[cockroach-teamcity]

This change is 

[spilchen]

I had a few suggestions to help with understanding the code, but otherwise the changes makes sense to me.

Reviewed 1 of 1 files at r1, 3 of 4 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @fqazi)

---

pkg/sql/catalog/catprivilege/fix.go line 29 at r3 (raw file):

// dummyPrivilegeDescriptorBuilder implements PrivilegeDescriptorBuilder with
// the assumption that the privilege descriptor is mutable.
type dummyPrivilegeDescriptorBuilder struct {

nit: The term dummy suggests that it doesn’t actually do anything or is only used for testing purposes, at least that’s my impression. I believe we use it to fix actual descriptors, though it might be suboptimal in terms of memory allocations. Perhaps we could call it SimplePrivilegeDescriptorBuilder, or if we intend to eventually replace all uses of this builder and want to discourage new usage, maybe DeprecatedPrivilegeDescriptorBuilder.

---

pkg/sql/catalog/catprivilege/fix.go line 34 at r3 (raw file):

// GetLatestPrivilege implements PrivilegeDescriptorBuilder.
func (d dummyPrivilegeDescriptorBuilder) GetLatestPrivilege() *catpb.PrivilegeDescriptor {

nit: I find it odd that GetLatest... may not be the most recent if a change was made to the descriptor returned from GetMutable.... Perhaps a better name is GetReadOnlyPrivilege()?

---

pkg/sql/catalog/tabledesc/table_desc_builder.go line 946 at r3 (raw file):

		if len(s) < len(ref.ColumnIDs) {
			d = builder.getOrInitModifiedDesc()
			ref.ColumnIDs = s

Do we need to update ref? I think it still points to something in the read-only version of the descriptor

---

pkg/sql/catalog/tabledesc/table_desc_builder.go line 955 at r3 (raw file):

		if s := cleanedIDs(col.UsesSequenceIds); len(s) < len(col.UsesSequenceIds) {
			d = builder.getOrInitModifiedDesc()
			col.UsesSequenceIds = s

Same comment here but for col.

---

pkg/sql/catalog/tabledesc/table_desc_builder.go line 960 at r3 (raw file):

		if s := cleanedIDs(col.OwnsSequenceIds); len(s) < len(col.OwnsSequenceIds) {
			d = builder.getOrInitModifiedDesc()
			col.OwnsSequenceIds = s

Same comment here

---

pkg/sql/catalog/tabledesc/table_desc_builder.go line 988 at r3 (raw file):

		if !mutationIDs.Contains(int(backref.MutationID)) {
			tbl = builder.getOrInitModifiedDesc()
			hasChanged = true

nit: could we break out of the loop once we set tbl?

[fqazi]

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

---

pkg/sql/catalog/catprivilege/fix.go line 29 at r3 (raw file):

Previously, spilchen wrote…

nit: The term dummy suggests that it doesn’t actually do anything or is only used for testing purposes, at least that’s my impression. I believe we use it to fix actual descriptors, though it might be suboptimal in terms of memory allocations. Perhaps we could call it SimplePrivilegeDescriptorBuilder, or if we intend to eventually replace all uses of this builder and want to discourage new usage, maybe DeprecatedPrivilegeDescriptorBuilder.

Done.

Renamed to SimplePrivilegeDescriptorBuilder, and hopefully we will just remove it in future PRs.

---

pkg/sql/catalog/tabledesc/table_desc_builder.go line 946 at r3 (raw file):

Previously, spilchen wrote…

Do we need to update ref? I think it still points to something in the read-only version of the descriptor

Done.

---

pkg/sql/catalog/tabledesc/table_desc_builder.go line 955 at r3 (raw file):

Previously, spilchen wrote…

Same comment here but for col.

Done.

---

pkg/sql/catalog/tabledesc/table_desc_builder.go line 960 at r3 (raw file):

Previously, spilchen wrote…

Same comment here

Done.

[spilchen]

Reviewed 6 of 6 files at r4, 4 of 4 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dt)

[spilchen]

I took another pass since you found a few more cases. It looks good to me, just had a few minor suggestions.

Reviewed 3 of 6 files at r7, 3 of 4 files at r8, 1 of 2 files at r9, 7 of 7 files at r10, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @dt and @fqazi)

---

pkg/sql/catalog/catprivilege/fix.go line 114 at r10 (raw file):

			} else {
				privs, _ = reader.GetMutablePrivilege().FindUser(user)
				privs.Privileges = allowedPrivilegesBits

Could you check if oldPrivilegeBits != allowedPrivilegesBits before calling GetMutablePrivilege? Not sure how common that will be to make a difference though.

---

pkg/sql/catalog/tabledesc/table_desc_builder.go line 120 at r10 (raw file):

	// Set the ModificationTime field before doing anything else.
	// Other changes may depend on it.
	mustSetModTime, err := descpb.MustSetModificationTime(

nit: maybe have a comment that says the error will be handled later via another call to MustSetModificationTime in a code path that can deal with errors.

[fqazi]

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

---

pkg/sql/catalog/catprivilege/fix.go line 114 at r10 (raw file):

Previously, spilchen wrote…

Could you check if oldPrivilegeBits != allowedPrivilegesBits before calling GetMutablePrivilege? Not sure how common that will be to make a difference though.

Done.

[fqazi]

@spilchen TFTR!

bors r+

[craig]

Build succeeded:

• local_roachtest
• check_generated_code
• macos_arm64_build
• docker_image_amd64
• unit_tests
• linux_amd64_fips_build
• macos_amd64_build
• lint
• acceptance
• linux_arm64_build
• local_roachtest_fips
• examples_orms
• windows_build
• linux_amd64_build