sql: improve historical descriptor look up efficiency #71239

jameswsj10 · 2021-10-06T19:53:12Z

Fixes #70692. The existing implementation for looking up old
historical descriptors required multiple round trips to storage.
This improvement requires only 1, at most 2, KV calls to storage
by using a single ExportRequest.

Release note (performance improvement): reduce kv calls when
looking up old historical descriptors to 1 or at most 2.

cockroach-teamcity · 2021-10-06T19:53:19Z

This change is

ajwerner

It seems like your test is flakey but I haven't investigated.

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

pkg/sql/catalog/lease/lease.go, line 190 at r1 (raw file):

// Retrieves historical descriptors of given id within the lower and upper bound
// timestamp from the MVCC key range.

say more about the lower and upper bound and how they interact with the retrieved descriptors.

pkg/sql/catalog/lease/lease.go, line 193 at r1 (raw file):

func (m *Manager) getDescriptorsFromStoreForInterval(
	ctx context.Context, db *kv.DB, id descpb.ID, lowerBound, upperBound hlc.Timestamp,
) ([]catalog.Descriptor, error) {

the only use of m in this method is for its codec. Thoughts on making this a function rather than a method and passing in the codec?

pkg/sql/catalog/lease/lease.go, line 195 at r1 (raw file):

	// Create an export request (1 kv call) for all descriptors for given
	// descriptor ID in range [timestamp, endTimestamp]

note that `in` here means that it was written in that range, not that it was live in that time range.

nit: period on sentences, here and below

pkg/sql/catalog/lease/lease.go, line 210 at r1 (raw file):

	res, pErr := kv.SendWrappedWith(ctx, db.NonTransactionalSender(), batchRequestHeader, req)
	if pErr != nil {
		err := pErr.GoError()

nit: inline this call into the wrapf

pkg/sql/catalog/lease/lease.go, line 211 at r1 (raw file):

	if pErr != nil {
		err := pErr.GoError()
		return nil, errors.Wrapf(err, "Error in retrieving latest table descs as of timestamp %s", lowerBound)

nit: start errors without capitalization.

pkg/sql/catalog/lease/lease.go, line 237 at r1 (raw file):

Quoted 5 lines of code…

					return err
				}
				_, id, err := encoding.DecodeUvarintAscending(remaining)
				if err != nil {
					return err

let's wrap these errors here with assertion failure and some information about descriptor and timestamp/time range you're looking at.

pkg/sql/catalog/lease/lease.go, line 247 at r1 (raw file):

				unsafeValue := it.UnsafeValue()
				if unsafeValue == nil {
					name := fmt.Sprintf("desc(%d)", id)

you're already formatting on the next line, why the two-level formatting?

pkg/sql/catalog/lease/lease.go, line 248 at r1 (raw file):

				if unsafeValue == nil {
					name := fmt.Sprintf("desc(%d)", id)
					return errors.Errorf("%v was dropped or truncated", name)

🤔 let's think through this case in a bit more detail.

pkg/sql/catalog/lease/lease.go, line 252 at r1 (raw file):

				value := roachpb.Value{RawBytes: unsafeValue}
				var desc descpb.Descriptor
				if err = value.GetProto(&desc); err != nil {

nit: declare a new err here with :=.

pkg/sql/catalog/lease/lease.go, line 297 at r1 (raw file):

	}

	// Iterate from the latest modification time from retrieved descriptors.

Huh, I think we end up needing to do two extra requests if we don't know the expiration of the later descriptor.

[v@t1          ][v2@t3             ][v3@t5             ][v4@t7 
         [t2                                t6]

Consider the above case, what descriptors should we get back? Consider the cases where we know about v4 and the cases where we don't.

In my head it should be:

v1@t1,exp=t3
v2@t3,exp=t5
v3@t5,exp=t7

If we did not know about v4, then we'd need to make a request (which you have logic to do). What logic finds v1?

ajwerner

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

pkg/sql/catalog/lease/lease.go, line 297 at r1 (raw file):

Previously, ajwerner wrote…

Huh, I think we end up needing to do two extra requests if we don't know the expiration of the later descriptor.
[v@t1          ][v2@t3             ][v3@t5             ][v4@t7 
         [t2                                t6]
Consider the above case, what descriptors should we get back? Consider the cases where we know about v4 and the cases where we don't.

In my head it should be:

v1@t1,exp=t3

v2@t3,exp=t5

v3@t5,exp=t7

If we did not know about v4, then we'd need to make a request (which you have logic to do). What logic finds v1?

I'm reaching understanding. I think the core issue here is that I read subsequent as later but in fact it's earlier. I think my main ask is comment this stuff gratuitously. Maybe some examples with little ascii diagrams would be worthwhile.

jameswsj10

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

pkg/sql/catalog/lease/lease.go, line 190 at r1 (raw file):

Previously, ajwerner wrote…

// Retrieves historical descriptors of given id within the lower and upper bound
// timestamp from the MVCC key range.
say more about the lower and upper bound and how they interact with the retrieved descriptors.

Done.

pkg/sql/catalog/lease/lease.go, line 237 at r1 (raw file):

Previously, ajwerner wrote…

					return err
				}
				_, id, err := encoding.DecodeUvarintAscending(remaining)
				if err != nil {
					return err
let's wrap these errors here with assertion failure and some information about descriptor and timestamp/time range you're looking at.

Removed decoding logic as it's unnecessary when extracting descriptor. Added information about descriptor key and modification/expire timestamps for other errors.

pkg/sql/catalog/lease/lease.go, line 247 at r1 (raw file):

Previously, ajwerner wrote…

you're already formatting on the next line, why the two-level formatting?

Removed.

pkg/sql/catalog/lease/lease.go, line 248 at r1 (raw file):

Previously, ajwerner wrote…

🤔 let's think through this case in a bit more detail.

Modified to specify error accessing raw bytes of descriptor value

pkg/sql/catalog/lease/lease.go, line 297 at r1 (raw file):

Previously, ajwerner wrote…

I'm reaching understanding. I think the core issue here is that I read subsequent as later but in fact it's earlier. I think my main ask is comment this stuff gratuitously. Maybe some examples with little ascii diagrams would be worthwhile.

added small diagram in readOlderVersionForTimestamp to clarify.

ajwerner

Reviewed 3 of 6 files at r2, 1 of 2 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, @jameswsj10, and @postamar)

pkg/sql/catalog/lease/lease.go, line 191 at r3 (raw file):

// after the lower bound timestamp and before the upper bound timestamp will be
// retrieved through an export request.

Can you be more explicit about inclusivity of time bounds? I think a descriptor modified exactly at the lower bound would be visible.

pkg/sql/catalog/lease/lease.go, line 226 at r3 (raw file):

	// Unmarshal key span retrieved from export request to construct historical descs.
	var descriptorsRead []historicalDescriptor
	subsequentModificationTime := upperBound

Comment this variable. It's subtle.

pkg/sql/catalog/lease/lease.go, line 268 at r3 (raw file):

				descriptorsRead = append(descriptorsRead, histDesc)

				// update the expiration time for next iteration.

uber-nit: capitalize Update

pkg/sql/catalog/lease/lease.go, line 281 at r3 (raw file):

// with at most 2 KV calls. We unfortunately need to read more than one
// descriptor version just so that we can set the expiration time on the
// descriptor properly.

note that assumption that the Manager already has in memory a descriptor which corresponds to the end of the time interval for this descriptor. Coming back to our diagram:

-----time------>
                           [v4@t7 ]
    [timestamp]

We need it to be the case that there exists descriptor state and that it has an entry after timestamp (right?). We're going to fix that in a follow-up PR.

pkg/sql/catalog/lease/lease.go, line 303 at r3 (raw file):

Quoted 8 lines of code…

	// Make an export request for descriptors between the start and end
	// timestamps, returning descriptors in decreasing modification time order.
	//
	// In the following scenario v4 is our oldest active lease
	// [v1@t1			][v2@t3			][v3@t5				][v4@t7
	// 		 [start													end]
	// getDescriptorsFromStoreForInterval(..., start, end) will get back:
	// [v3, v2] (reverse order)

I think this comment belongs on getDescriptorsFromStoreForInterval

pkg/sql/catalog/lease/lease.go, line 321 at r3 (raw file):

	// another KV call.
	earliestModificationTime := descs[len(descs)-1].desc.GetModificationTime()
	if timestamp.Less(earliestModificationTime) {

what's the case where this is false?

pkg/sql/catalog/lease/lease_internal_test.go, line 1061 at r3 (raw file):

// Tests retrieving older versions within a given start and end timestamp of a
// table descriptor from store through an ExportRequest.

on the whole, this is a reasonable high-level test but I think we'll also want a lower-level test. I'm also not clear on what's going on at the end. This test ends up feeling both high and low level, which is a little weird to me.

func TestHistoricalExportRequestForTimeRange(t *testing.T) {
    	defer leaktest.AfterTest(t)()
	var stopper *stop.Stopper
	s, sqlDB, _ := serverutils.StartServer(t, base.TestServerArgs{})
	stopper = s.Stopper()
	ctx := context.Background()
	defer stopper.Stop(ctx)

        
        tdb := sqlutils.MakeSQLRunner(sqlDB)
        tdb.Exec("CREATE TABLE foo (i INT PRIMARY KEY)")
        var tableID descpb.ID
        tdb.QueryRow("SELECT id FROM system.namespace WHERE name = 'foo'").Scan(&tableID)
        
        manager := s.LeaseManager().(*Manager)
        const N = 5
        descs := make([]catalog.Descriptor, N+1)
        timestamps := make([]hlc.Timestamp, N+1)
        for i := 0; i < N; i++ {
             _, err := manager.Publish(ctx, tableID, func(desc catalog.MutableDescriptor) error {
                  descs[i] = desc.ImmutableCopy() // will be the previous version
             })
            require.NoError(t, err)
        }
        { 
            last, err := manager.Acquire(ctx, s.Clock().Now(), tableID)
            require.NoError(t, err)
            descs[N] = last.Underlying()
            last.Release(ctx)
       }
       // Now you've got N descriptors that each should have the `ModificationTime` set. 
       // Now let's test the new code as it explored various timestamps and make sure that it returns exactly the expected data.

pkg/sql/catalog/lease/lease_internal_test.go, line 1075 at r3 (raw file):

Quoted 7 lines of code…

	parseHLC := func(ts string) (hlc.Timestamp, error) {
		dec, _, err := apd.NewFromString(ts)
		if err != nil {
			return hlc.Timestamp{}, err
		}
		return tree.DecimalToHLC(dec)
	}

I'd prefer if you just move sql.ParseHLC to tree right next to DecimalToHLC rather than re-invent it here.

pkg/sql/catalog/lease/lease_internal_test.go, line 1141 at r3 (raw file):

Quoted 9 lines of code…

	manager.mu.Lock()
	for _, mDesc := range manager.mu.descriptors {
		mDesc.mu.Lock()
		if mDesc.id == descriptorID {
			mDesc.mu.active.data = nil
		}
		mDesc.mu.Unlock()
	}
	manager.mu.Unlock()

why are you doing this? I don't see anything looking inside the manager's state again after you clear it.

pkg/sql/catalog/lease/lease_internal_test.go, line 1147 at r3 (raw file):

	// Assert returned descriptors modification times are between ts1 and ts2 and the IDs match our query historicalDesc id
	for _, historicalDesc := range historicalDescs {

wouldn't this pass if the set of returned descriptors was empty?

jameswsj10

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

pkg/sql/catalog/lease/lease.go, line 191 at r3 (raw file):

Previously, ajwerner wrote…

// after the lower bound timestamp and before the upper bound timestamp will be
// retrieved through an export request.
Can you be more explicit about inclusivity of time bounds? I think a descriptor modified exactly at the lower bound would be visible.

Specified interval as [lower bound, upper bound)

pkg/sql/catalog/lease/lease.go, line 281 at r3 (raw file):

Previously, ajwerner wrote…

note that assumption that the Manager already has in memory a descriptor which corresponds to the end of the time interval for this descriptor. Coming back to our diagram:
-----time------>
                           [v4@t7 ]
    [timestamp]
We need it to be the case that there exists descriptor state and that it has an entry after timestamp (right?). We're going to fix that in a follow-up PR.

Mentioned as a TODO for a follow-up PR.

pkg/sql/catalog/lease/lease.go, line 303 at r3 (raw file):

Previously, ajwerner wrote…

	// Make an export request for descriptors between the start and end
	// timestamps, returning descriptors in decreasing modification time order.
	//
	// In the following scenario v4 is our oldest active lease
	// [v1@t1			][v2@t3			][v3@t5				][v4@t7
	// 		 [start													end]
	// getDescriptorsFromStoreForInterval(..., start, end) will get back:
	// [v3, v2] (reverse order)

I think this comment belongs on getDescriptorsFromStoreForInterval

Moved diagram and granular description to getDescriptorsFromStoreForInterval

pkg/sql/catalog/lease/lease.go, line 321 at r3 (raw file):

Previously, ajwerner wrote…

what's the case where this is false?

False when the timestamp is (magically) exactly at the earliestModificationTime. Added comment to address subtlety.

pkg/sql/catalog/lease/lease_internal_test.go, line 1141 at r3 (raw file):

Previously, ajwerner wrote…

	manager.mu.Lock()
	for _, mDesc := range manager.mu.descriptors {
		mDesc.mu.Lock()
		if mDesc.id == descriptorID {
			mDesc.mu.active.data = nil
		}
		mDesc.mu.Unlock()
	}
	manager.mu.Unlock()
why are you doing this? I don't see anything looking inside the manager's state again after you clear it.

I initially thought it'd be a good idea to clear out the active leases in memory in manager's state before calling the export request, but I realized my test doesn't require clearing it out. Removed.

ajwerner

This is a good change. I believe this is enough to let us lift the limitation of the lease manager. Just some minor stuff related to the test and it'll be good to go. Also, you need to regenerate the bazel things.

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

pkg/sql/catalog/lease/lease_internal_test.go, line 1067 at r4 (raw file):

	ctx := context.Background()
	defer stopper.Stop(ctx)

the flake you're seeing in this test is due to background lease acquisition. You can disable that by using the sql.tablecache.lease.refresh_limit cluster setting and setting it to 0.

pkg/sql/catalog/lease/lease_internal_test.go, line 1150 at r4 (raw file):

		t.Run(fmt.Sprintf("%v@%v->%v", tc.before, tc.ts, tc.expected), func(t *testing.T) {
			// Reset the descriptor state to before versions.
			descStates := manager.mu.descriptors

use the mutex to change this state.

ajwerner

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

pkg/sql/catalog/lease/lease_internal_test.go, line 1152 at r5 (raw file):

		},
	} {
		t.Run(fmt.Sprintf("%v@%v->%v", tc.before, tc.ts, tc.expected), func(t *testing.T) {

this makes for inconsistent names of the test because the timestamp is dynamic. Let's add a tsStr field to the struct to name thing timestamp. eg:

		{
			before:   []version{0, 1, 2, 3, 4, 5},
			ts:       versionTS(3),
                        tsStr:    "ts3",
                        expected: []version{},
                }

pkg/sql/catalog/lease/lease_internal_test.go, line 1154 at r5 (raw file):

		t.Run(fmt.Sprintf("%v@%v->%v", tc.before, tc.ts, tc.expected), func(t *testing.T) {
			// Reset the descriptor state to before versions.
			manager.mu.Lock()

nit: pull all of this resetting out into a helper closure or function.

pkg/sql/catalog/lease/lease_internal_test.go, line 1157 at r5 (raw file):

			descStates[tableID] = nil
			descStates[tableID] = &descriptorState{m: manager, id: tableID}

you don't need to set it to nil if you're assigning over it.

pkg/sql/catalog/lease/lease_internal_test.go, line 1181 at r5 (raw file):

				expectedTS = append(expectedTS, versionTS(e))
			}
			require.Equal(t, len(tc.expected), len(actualTS), "\nexpected: %v\nactual: %v\n", expectedTS, actualTS)

Can you also transpose the descriptors into a []version slice and validate that it matches the expectations?

jameswsj10

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

pkg/sql/catalog/lease/lease_internal_test.go, line 1152 at r5 (raw file):

Previously, ajwerner wrote…

this makes for inconsistent names of the test because the timestamp is dynamic. Let's add a tsStr field to the struct to name thing timestamp. eg:
		{
			before:   []version{0, 1, 2, 3, 4, 5},
			ts:       versionTS(3),
                        tsStr:    "ts3",
                        expected: []version{},
                }

done.

pkg/sql/catalog/lease/lease_internal_test.go, line 1157 at r5 (raw file):

Previously, ajwerner wrote…

			descStates[tableID] = nil
			descStates[tableID] = &descriptorState{m: manager, id: tableID}
you don't need to set it to nil if you're assigning over it.

Done.

pkg/sql/catalog/lease/lease_internal_test.go, line 1181 at r5 (raw file):

Previously, ajwerner wrote…

Can you also transpose the descriptors into a []version slice and validate that it matches the expectations?

Done.

ajwerner

Reviewed 1 of 10 files at r4, 1 of 2 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, @jameswsj10, and @postamar)

pkg/ccl/backupccl/backup_test.go, line 6779 at r6 (raw file):

		},
		TestingResponseFilter: func(ctx context.Context, ba roachpb.BatchRequest, br *roachpb.BatchResponse) *roachpb.Error {
			for _, ru := range br.Responses {

seems from the test failure that you need to do something here too

pkg/sql/catalog/lease/lease.go, line 190 at r1 (raw file):

Previously, jameswsj10 (James Jung) wrote…

Done.

say what happens if lowerBound or upperBound are zero. I think we should return an error if lowerBound is zero.

pkg/sql/catalog/lease/lease_internal_test.go, line 1181 at r5 (raw file):

Previously, jameswsj10 (James Jung) wrote…

Done.

rather than looking them up by timestamp below, it'd be better to actually pull the version out of the descriptor and use that.

pkg/sql/catalog/lease/lease_internal_test.go, line 1169 at r6 (raw file):

		t.Run(fmt.Sprintf("%v@%v->%v", tc.before, tc.tsStr, tc.expected), func(t *testing.T) {
			// Reset the descriptor state to before versions.
			func() {

I'd prefer if you pulled this out above the test cases and named it something like resetDescriptorState

pkg/sql/catalog/lease/lease_internal_test.go, line 1181 at r6 (raw file):

					)
				}
				manager.mu.Unlock()

defer this now that you're in a function

jameswsj10

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

pkg/ccl/backupccl/backup_test.go, line 6779 at r6 (raw file):

Previously, ajwerner wrote…

seems from the test failure that you need to do something here too

Added in check for if exportRequest is from leasing descriptors for the request filters.

pkg/sql/catalog/lease/lease.go, line 190 at r1 (raw file):

Previously, ajwerner wrote…

say what happens if lowerBound or upperBound are zero. I think we should return an error if lowerBound is zero.

Added an initial check to see if timestamps are 0 to return error or not.

pkg/sql/catalog/lease/lease_internal_test.go, line 1181 at r6 (raw file):

Previously, ajwerner wrote…

defer this now that you're in a function

Done.

ajwerner

really almost there

Reviewed 1 of 3 files at r7.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, @jameswsj10, and @postamar)

pkg/ccl/backupccl/backup_test.go, line 6782 at r7 (raw file):

				if exportRequest, ok := ba.Requests[i].GetInner().(*roachpb.ExportRequest); ok &&
					!isLeasingExportRequest(exportRequest) {
					exportResponse := ru.GetInner().(*roachpb.ExportResponse)

no need to do this type assertion anymore

pkg/sql/catalog/lease/lease_internal_test.go, line 1086 at r7 (raw file):

Quoted 29 lines of code…

type version int
type testCase struct {
	before   []version
	ts       hlc.Timestamp
	tsStr    string
	expected []version
}

// Sets the manager's descriptor state to contain the specific testCase's
// versions of the descriptor.
func resetDescriptorState(
	manager *Manager, tableID descpb.ID, tc testCase, versionDesc func(v version) catalog.Descriptor,
) {
	manager.mu.Lock()
	defer manager.mu.Unlock()
	descStates := manager.mu.descriptors
	descStates[tableID] = &descriptorState{m: manager, id: tableID}
	for _, v := range tc.before {
		addedDescVState := &descriptorVersionState{
			t:          descStates[tableID],
			Descriptor: versionDesc(v),
		}
		addedDescVState.mu.Lock()
		addedDescVState.mu.expiration = hlc.MaxTimestamp
		addedDescVState.mu.Unlock()
		descStates[tableID].mu.active.insert(addedDescVState)
	}
}

nit: define all of this inside the test function. I was just asking for a closure with a name, not a new function

pkg/sql/catalog/lease/lease_internal_test.go, line 1217 at r7 (raw file):

Quoted 6 lines of code…

			func() {
				manager.mu.Lock()
				defer manager.mu.Unlock()
				descStates := manager.mu.descriptors
				t.Logf("Manager's descriptor state for desc %d: %v", tableID, descStates[tableID].mu.active.data)
			}()

what is this about?

pkg/sql/catalog/lease/lease_internal_test.go, line 1227 at r7 (raw file):

Quoted 29 lines of code…


			var actualVersions []descpb.DescriptorVersion
			for _, desc := range retrieved {
				actualTS = append([]hlc.Timestamp{desc.desc.GetModificationTime()}, actualTS...)
				actualVersions = append([]descpb.DescriptorVersion{desc.desc.GetVersion()}, actualVersions...)
			}

			// Validate retrieved descriptors match expected versions.
			var expectedTS []hlc.Timestamp
			for _, e := range tc.expected {
				expectedTS = append(expectedTS, versionTS(e))
			}

			func() {
				manager.mu.Lock()
				defer manager.mu.Unlock()
				descStates := manager.mu.descriptors
				t.Logf("Manager's descriptor state for desc %d: %v", tableID, descStates[tableID].mu.active.data)
			}()

			require.Equal(t, len(tc.expected), len(actualTS), "\nexpected: %v\nactual: %v\n", expectedTS, actualTS)
			for i, eTS := range expectedTS {
				aTS := actualTS[i]
				require.Equal(t, eTS, aTS, "expected: %v, actual: %v\n", eTS, aTS)

				aVersion := version(actualVersions[i])
				eVersion := tc.expected[i]
				require.Equal(t, eVersion, aVersion, "expected: %v, actual : %v\n", eVersion, aVersion)
			}

I feel like all that I'm looking for here is:

			var retrievedVersions []version
			for _, desc := range retrieved {
				retrievedVersions = append(retrievedVersions, version(desc.desc.GetVersion())
			}
			require.Equal(t, tc.expected, tc.retreivedVersions)

jameswsj10

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

pkg/ccl/backupccl/backup_test.go, line 6782 at r7 (raw file):

Previously, ajwerner wrote…

no need to do this type assertion anymore

I believe the type assertion is required since the above type assertion is for roachpb.ExportRequest. Without type assertion for roachpb.ExportResponse, we wouldn't be able to access the files variable a few lines below.

pkg/sql/catalog/lease/lease_internal_test.go, line 1217 at r7 (raw file):

Previously, ajwerner wrote…

			func() {
				manager.mu.Lock()
				defer manager.mu.Unlock()
				descStates := manager.mu.descriptors
				t.Logf("Manager's descriptor state for desc %d: %v", tableID, descStates[tableID].mu.active.data)
			}()

what is this about?

this was the logging for the manager's descriptor state I used for debugging earlier. Removed.

pkg/sql/catalog/lease/lease_internal_test.go, line 1227 at r7 (raw file):

Previously, ajwerner wrote…


			var actualVersions []descpb.DescriptorVersion
			for _, desc := range retrieved {
				actualTS = append([]hlc.Timestamp{desc.desc.GetModificationTime()}, actualTS...)
				actualVersions = append([]descpb.DescriptorVersion{desc.desc.GetVersion()}, actualVersions...)
			}

			// Validate retrieved descriptors match expected versions.
			var expectedTS []hlc.Timestamp
			for _, e := range tc.expected {
				expectedTS = append(expectedTS, versionTS(e))
			}

			func() {
				manager.mu.Lock()
				defer manager.mu.Unlock()
				descStates := manager.mu.descriptors
				t.Logf("Manager's descriptor state for desc %d: %v", tableID, descStates[tableID].mu.active.data)
			}()

			require.Equal(t, len(tc.expected), len(actualTS), "\nexpected: %v\nactual: %v\n", expectedTS, actualTS)
			for i, eTS := range expectedTS {
				aTS := actualTS[i]
				require.Equal(t, eTS, aTS, "expected: %v, actual: %v\n", eTS, aTS)

				aVersion := version(actualVersions[i])
				eVersion := tc.expected[i]
				require.Equal(t, eVersion, aVersion, "expected: %v, actual : %v\n", eVersion, aVersion)
			}

I feel like all that I'm looking for here is:

			var retrievedVersions []version
			for _, desc := range retrieved {
				retrievedVersions = append(retrievedVersions, version(desc.desc.GetVersion())
			}
			require.Equal(t, tc.expected, tc.retreivedVersions)

Done.

ajwerner

Reviewed 7 of 10 files at r4, 2 of 4 files at r5, 1 of 3 files at r7, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, @jameswsj10, and @postamar)

pkg/ccl/backupccl/backup_test.go, line 6782 at r7 (raw file):

Previously, jameswsj10 (James Jung) wrote…

I believe the type assertion is required since the above type assertion is for roachpb.ExportRequest. Without type assertion for roachpb.ExportResponse, we wouldn't be able to access the files variable a few lines below.

right, sorry

pkg/sql/catalog/lease/lease_internal_test.go, line 1189 at r8 (raw file):

Quoted 19 lines of code…

			// Reset the descriptor state to before versions.
			resetDescriptorState := func(
				manager *Manager, tableID descpb.ID, tc testCase,
			) {
				manager.mu.Lock()
				defer manager.mu.Unlock()
				descStates := manager.mu.descriptors
				descStates[tableID] = &descriptorState{m: manager, id: tableID}
				for _, v := range tc.before {
					addedDescVState := &descriptorVersionState{
						t:          descStates[tableID],
						Descriptor: versionDesc(v),
					}
					addedDescVState.mu.Lock()
					addedDescVState.mu.expiration = hlc.MaxTimestamp
					addedDescVState.mu.Unlock()
					descStates[tableID].mu.active.insert(addedDescVState)
				}
			}

heh I was thinking above the body of this function but whatever at this point.

it can close over tableID but not over t if you did move it.

Fixes cockroachdb#70692. The existing implementation for looking up old historical descriptors required multiple round trips to storage. This improvement requires only 1, at most 2, KV calls to storage by using a single ExportRequest. Release note (performance improvement): improve efficiency of looking up old historical descriptors.

jameswsj10

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

pkg/sql/catalog/lease/lease_internal_test.go, line 1189 at r8 (raw file):

Previously, ajwerner wrote…

			// Reset the descriptor state to before versions.
			resetDescriptorState := func(
				manager *Manager, tableID descpb.ID, tc testCase,
			) {
				manager.mu.Lock()
				defer manager.mu.Unlock()
				descStates := manager.mu.descriptors
				descStates[tableID] = &descriptorState{m: manager, id: tableID}
				for _, v := range tc.before {
					addedDescVState := &descriptorVersionState{
						t:          descStates[tableID],
						Descriptor: versionDesc(v),
					}
					addedDescVState.mu.Lock()
					addedDescVState.mu.expiration = hlc.MaxTimestamp
					addedDescVState.mu.Unlock()
					descStates[tableID].mu.active.insert(addedDescVState)
				}
			}

heh I was thinking above the body of this function but whatever at this point.

it can close over tableID but not over t if you did move it.

I think it's worth making the change -- didn't realize I was defining that in a for loop for every iteration.

jameswsj10 · 2021-11-03T18:16:11Z

bors r+

craig · 2021-11-03T21:00:37Z

Build succeeded:

GitHub CI (Cockroach)

@jameswsj10

In cockroachdb#71239, we added a new mechanism to look up historical descriptors. I erroneously informed @jameswsj10 that we would never have gaps in the descriptor history, and, thus, when looking up historical descriptors, we could always use the earliest descriptor's modification time as the bounds for the relevant query. This turns out to not be true. Consider the case where version 3 is a historical version and then version 4 pops up and gets leased. Version 3 will get removed if it is not referenced. In the meantime, version 3 existed when we went to go find version 2. At that point, we'll inject version 2 and have version 4 leased. We need to make sure we can handle the case where we need to go fetch version 3. In the meantime, this change also removes some logic added to support the eventual resurrection of cockroachdb#59606 whereby we'll use the export request to fetch descriptor history to power historical queries even in the face of descriptors having been deleted. Fixes cockroachdb#72706. Release note: None

@jameswsj10

72669: bazel: properly generate `.eg.go` code in `pkg/sql/colconv` via bazel r=rail a=rickystewart Release note: None 72714: sql/catalog/lease: permit gaps in descriptor history r=ajwerner a=ajwerner In #71239, we added a new mechanism to look up historical descriptors. I erroneously informed @jameswsj10 that we would never have gaps in the descriptor history, and, thus, when looking up historical descriptors, we could always use the earliest descriptor's modification time as the bounds for the relevant query. This turns out to not be true. Consider the case where version 3 is a historical version and then version 4 pops up and gets leased. Version 3 will get removed if it is not referenced. In the meantime, version 3 existed when we went to go find version 2. At that point, we'll inject version 2 and have version 4 leased. We need to make sure we can handle the case where we need to go fetch version 3. In the meantime, this change also removes some logic added to support the eventual resurrection of #59606 whereby we'll use the export request to fetch descriptor history to power historical queries even in the face of descriptors having been deleted. Fixes #72706. Release note: None 72740: sql/catalog/descs: fix perf regression r=ajwerner a=ajwerner This commit in #71936 had the unfortunate side-effect of allocating and forcing reads on the `uncommittedDescriptors` set even when we aren't looking for the system database. This has an outsized impact on the performance of the single-node, high-core-count KV runs. Instead of always initializing the system database, just do it when we access it. ``` name old ops/s new ops/s delta KV95-throughput 88.6k ± 0% 94.8k ± 1% +7.00% (p=0.008 n=5+5) name old ms/s new ms/s delta KV95-P50 1.60 ± 0% 1.40 ± 0% -12.50% (p=0.008 n=5+5) KV95-Avg 0.60 ± 0% 0.50 ± 0% -16.67% (p=0.008 n=5+5) ``` The second commit is more speculative and came from looking at a profile where 1.6% of the allocated garbage was due to that `NameInfo` even though we'll never, ever hit it. <img width="2345" alt="Screen Shot 2021-11-15 at 12 57 31 AM" src="https://user-images.githubusercontent.com/1839234/141729924-d00eebab-b35c-42bd-8d0b-ee39f3ac7d46.png"> Fixes #72499 Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com> Co-authored-by: Andrew Werner <awerner32@gmail.com>

jameswsj10 requested review from postamar and a team October 6, 2021 19:53

jameswsj10 force-pushed the improve-historical-descriptor-lookups branch 3 times, most recently from 371a9b4 to ac17112 Compare October 12, 2021 19:25

postamar requested a review from ajwerner October 13, 2021 15:38

jameswsj10 changed the title ~~sql: improving historical descriptor look up efficiency~~ sql: improve historical descriptor look up efficiency Oct 13, 2021

ajwerner reviewed Oct 13, 2021

View reviewed changes

jameswsj10 force-pushed the improve-historical-descriptor-lookups branch from ac17112 to 3b7fa11 Compare October 15, 2021 21:04

jameswsj10 requested review from a team and adityamaru and removed request for a team October 15, 2021 21:04

jameswsj10 commented Oct 15, 2021

View reviewed changes

jameswsj10 requested a review from ajwerner October 15, 2021 21:05

jameswsj10 force-pushed the improve-historical-descriptor-lookups branch from 3b7fa11 to 2b888cc Compare October 18, 2021 15:25

ajwerner reviewed Oct 18, 2021

View reviewed changes

jameswsj10 force-pushed the improve-historical-descriptor-lookups branch from 2b888cc to 2dbcc78 Compare October 27, 2021 16:01

jameswsj10 requested a review from a team as a code owner October 27, 2021 16:01

jameswsj10 commented Oct 27, 2021

View reviewed changes

ajwerner reviewed Oct 27, 2021

View reviewed changes

jameswsj10 force-pushed the improve-historical-descriptor-lookups branch from 2dbcc78 to e96ddba Compare October 27, 2021 21:31

ajwerner reviewed Oct 27, 2021

View reviewed changes

jameswsj10 force-pushed the improve-historical-descriptor-lookups branch from e96ddba to f2dd622 Compare November 2, 2021 05:57

jameswsj10 commented Nov 2, 2021

View reviewed changes

ajwerner reviewed Nov 2, 2021

View reviewed changes

jameswsj10 commented Nov 2, 2021

View reviewed changes

jameswsj10 force-pushed the improve-historical-descriptor-lookups branch from f2dd622 to 91f4097 Compare November 2, 2021 19:58

ajwerner reviewed Nov 2, 2021

View reviewed changes

jameswsj10 force-pushed the improve-historical-descriptor-lookups branch from 91f4097 to 4cbfbe7 Compare November 2, 2021 21:25

jameswsj10 commented Nov 2, 2021

View reviewed changes

ajwerner approved these changes Nov 2, 2021

View reviewed changes

jameswsj10 force-pushed the improve-historical-descriptor-lookups branch from 4cbfbe7 to c4d0b04 Compare November 3, 2021 16:07

jameswsj10 commented Nov 3, 2021

View reviewed changes

craig bot merged commit 3bc13c2 into cockroachdb:master Nov 3, 2021

cockroach-teamcity mentioned this pull request Nov 3, 2021

sql: improve historical descriptor look up efficiency cockroachdb/docs#12178

Closed

ajwerner mentioned this pull request Nov 12, 2021

cdc: changefeed TeamCity flakiness #72706

Closed

ajwerner mentioned this pull request Nov 12, 2021

sql/catalog/lease: permit gaps in descriptor history #72714

Merged

ajwerner mentioned this pull request Mar 11, 2022

sql/catalog: lease manager won't return prior versions of dropped descriptors #53299

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: improve historical descriptor look up efficiency #71239

sql: improve historical descriptor look up efficiency #71239

jameswsj10 commented Oct 6, 2021

cockroach-teamcity commented Oct 6, 2021

ajwerner left a comment

ajwerner left a comment

jameswsj10 left a comment

ajwerner left a comment

jameswsj10 left a comment

ajwerner left a comment

ajwerner left a comment

jameswsj10 left a comment

ajwerner left a comment

jameswsj10 left a comment

ajwerner left a comment

jameswsj10 left a comment

ajwerner left a comment

jameswsj10 left a comment

jameswsj10 commented Nov 3, 2021

craig bot commented Nov 3, 2021

sql: improve historical descriptor look up efficiency #71239

sql: improve historical descriptor look up efficiency #71239

Conversation

jameswsj10 commented Oct 6, 2021

cockroach-teamcity commented Oct 6, 2021

ajwerner left a comment

Choose a reason for hiding this comment

note that in here means that it was written in that range, not that it was live in that time range.

ajwerner left a comment

Choose a reason for hiding this comment

jameswsj10 left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

jameswsj10 left a 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

jameswsj10 left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

jameswsj10 left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

jameswsj10 left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

jameswsj10 left a comment

Choose a reason for hiding this comment

jameswsj10 commented Nov 3, 2021

craig bot commented Nov 3, 2021

note that `in` here means that it was written in that range, not that it was live in that time range.