storageccl: don't update file registry when creating unencrypted files

[andyyang890]

This patch changes PebbleFileRegistry to not store an entry
for an unencrypted file. This should lead to a performance
improvement since the file registry is rewritten every time
it is updated.

Fixes #65430.

Release note: None

[cockroach-teamcity]

This change is 

[jbowens]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890, @itsbilal, @pbardea, and @sumeerbhola)

---

pkg/storage/pebble_file_registry.go, line 110 at r1 (raw file):

		return nil
	}

Do we need to verify that the file doesn't already exist in the registry? I believe the only caller of SetFileEntry is encryptedFS.Create. Calling Create on an existing path is permitted and truncates the file:

https://github.com/cockroachdb/pebble/blob/cbda11b8689ff2093da5868a84ff00b54120d21c/vfs/vfs.go#L45-L47

If the old file was encrypted and still in the registry, it looks like we'd have the incorrect registry state for the file unless we remove the existing entry.

[andyyang890]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @itsbilal, @jbowens, @pbardea, and @sumeerbhola)

---

pkg/storage/pebble_file_registry.go, line 110 at r1 (raw file):

Previously, jbowens (Jackson Owens) wrote…

Do we need to verify that the file doesn't already exist in the registry? I believe the only caller of SetFileEntry is encryptedFS.Create. Calling Create on an existing path is permitted and truncates the file:

https://github.com/cockroachdb/pebble/blob/cbda11b8689ff2093da5868a84ff00b54120d21c/vfs/vfs.go#L45-L47

If the old file was encrypted and still in the registry, it looks like we'd have the incorrect registry state for the file unless we remove the existing entry.

Good catch, fixed!

[pbardea]

Removing myself as a reviewer, deferring to storage folks but did want to mention that if this is a performance improvement (and it looks like it is), it may warrant a performance improvement release note.

[jbowens]

Test looks good!
I think we'll want to backport this to 21.1 and 20.2.

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andyyang890, @itsbilal, @jbowens, and @sumeerbhola)

---

pkg/storage/pebble_file_registry.go, line 105 at r2 (raw file):

// SetFileEntry sets filename => entry in the registry map and persists the registry.
func (r *PebbleFileRegistry) SetFileEntry(filename string, entry *enginepb.FileEntry) error {
	// We choose not store an entry for unencrypted files since the absence of

nit: s/not store/not to store/

[andyyang890]

Removing myself as a reviewer, deferring to storage folks but did want to mention that if this is a performance improvement (and it looks like it is), it may warrant a performance improvement release note.

Noted, will add a release note to the backports but not this commit since it will be replaced by an upcoming revamp of encryption-at-rest.

[andyyang890]

bors r=jbowens

[craig]

Build failed:

• GitHub CI (Cockroach)

[andyyang890]

bors r=jbowens

[craig]

Build succeeded:

• GitHub CI (Cockroach)