db: add local table size metrics

[sumeerbhola]

The Metrics object lacks a total size of local sstables, which causes DiskSpaceUsage to overestimate the local disk space used. The new metrics track live, zombie and obsolete local file sizes.

Fixes #3391

[cockroach-teamcity]

This change is 

[sumeerbhola]

This lacks tests with non-local files. I'll add if the approach looks ok.

Reviewable status: 0 of 13 files reviewed, all discussions resolved (waiting on @RaduBerinde)

[RaduBerinde]

It feels like there's a more general problem, where in the pebble layer we should have easier access to the ObjectMetadata for a FileMetadata (ideally they'd be both part of the same structure). But I don't see a nice way to do it given that the container for them is Version.

Reviewable status: 0 of 14 files reviewed, 6 unresolved discussions (waiting on @sumeerbhola)

---

compaction.go line 2573 at r1 (raw file):

}

type fileMetaEtc struct {

[nit] maybe compactionOutput?

---

compaction.go line 2574 at r1 (raw file):

type fileMetaEtc struct {
	*fileMetadata

Do we need to embed this? I don't think we use it in too many places.

---

version_set.go line 694 at r1 (raw file):

func getZombiesAndUpdateVirtualBackings(
	ve *versionEdit, virtualBackings *manifest.VirtualBackings, provider objstorage.Provider,
) (zombies map[base.DiskFileNum]tableInfo, localLiveSizeDelta int64) {

I think this will be easier to do with the small refactoring in #3400 (we can just go through removedPhysicalBackings)

---

version_set.go line 768 at r1 (raw file):

}

func computeLocalSize(

[nit] maybe sizeIfLocal. Also add a comment, e.g. "returns backing.Size if the backing is a local file or 0 otherwise"

---

version_set.go line 773 at r1 (raw file):

	meta, err := provider.Lookup(fileTypeTable, backing.DiskFileNum)
	if err != nil {
		panic(errors.Wrapf(err, "disk file %s not found", backing.DiskFileNum))

This error can be misleading, it's not really the file itself. I think the provider error already has all the information (file %s (type %d) unknown to the objstorage provider).

I would add an objstorage.MustLookupTable(provider, diskFileNum) and objstorage.IsLocal(provider, diskFileNum) convenience wrappers. I think we'd use them in more places.

---

version_set.go line 1003 at r1 (raw file):

		// deleted from disk.
		//
		// TODO(sumeer): this means that the zombie metrics, like ZombieSize,

Good point.. I think it probably doesn't matter much in practice unless the deleter falls behind

[sumeerbhola]

TFTR!

It feels like there's a more general problem, where in the pebble layer we should have easier access to the ObjectMetadata for a FileMetadata (ideally they'd be both part of the same structure).

Our current decomposition seems reasonable. The provider manifest has an object added before the Pebble manifest, and removed from the provider manifest after removal from the Pebble manifest. And we can do a cheap join between those two when an object is still known to Pebble.

Reviewable status: 0 of 15 files reviewed, 4 unresolved discussions (waiting on @RaduBerinde)

---

compaction.go line 2573 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] maybe compactionOutput?

Done

---

compaction.go line 2574 at r1 (raw file):

Previously, RaduBerinde wrote…

Do we need to embed this? I don't think we use it in too many places.

Removed embedding

---

version_set.go line 694 at r1 (raw file):

Previously, RaduBerinde wrote…

I think this will be easier to do with the small refactoring in #3400 (we can just go through removedPhysicalBackings)

Ack. I'll wait until that settles, since it seems to me that we don't need removedPhysicalBackings either.

---

version_set.go line 768 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] maybe sizeIfLocal. Also add a comment, e.g. "returns backing.Size if the backing is a local file or 0 otherwise"

Done

---

version_set.go line 773 at r1 (raw file):

This error can be misleading, it's not really the file itself. I think the provider error already has all the information (file %s (type %d) unknown to the objstorage provider).

Removed the wrapping

I would add an objstorage.MustLookupTable(provider, diskFileNum) and objstorage.IsLocal(provider, diskFileNum) convenience wrappers. I think we'd use them in more places.

Added objstorage.MustIsLocalTable and used here and in one other place in compaction.go.

---

version_set.go line 1003 at r1 (raw file):

Previously, RaduBerinde wrote…

Good point.. I think it probably doesn't matter much in practice unless the deleter falls behind

Agreed

[sumeerbhola]

I've added a test. I'll rebase on your PR when it is merged.

Reviewable status: 0 of 15 files reviewed, 4 unresolved discussions (waiting on @RaduBerinde)

[RaduBerinde]

Reviewable status: 0 of 15 files reviewed, 2 unresolved discussions (waiting on @sumeerbhola)

---

version_set.go line 748 at r4 (raw file):

	for _, b := range ve.CreatedBackingTables {
		stillUsed[b.DiskFileNum] = struct{}{}
		_, localFileDelta := sizeIfLocal(b, provider)

[nit] Feels like this belongs in the loop below, next to AddAndRef

[sumeerbhola]

Reviewable status: 0 of 16 files reviewed, 1 unresolved discussion (waiting on @RaduBerinde)

---

version_set.go line 748 at r4 (raw file):

Previously, RaduBerinde wrote…

[nit] Feels like this belongs in the loop below, next to AddAndRef

Done

[RaduBerinde]

Reviewable status: 0 of 16 files reviewed, all discussions resolved

[sumeerbhola]

created #3421 for flaky test

[sumeerbhola]

TFTR!