kvserver/concurrency: misc lock table tests to cover functionality by sumeerbhola · Pull Request #45769 · cockroachdb/cockroach

sumeerbhola · 2020-03-05T20:19:01Z

that was not tested before and three bug fixes

Some of the following was based on looking at test coverage results:

added wait_self: covers reservation by a different request from the
the same txn and lock acquisition by the same txn
we were not adding to the sequence nums in discoveredLock
which was a bug found when printing the sequence numbers
dup_access: added test when a request is both reading and writing
to a key and the lock is held after it stops waiting for write
added lock_changes: misc state changes for a lock that were not covered
by existing tests
added ignored seqnum tests to the update testdata file. It found
a bug
UpdateLocks now ignores updates with the same epoch but lower timestamp.
It used to trigger an error after the ts had been updated which was
severely muddled (tell the caller of the error after making the erroneous
state change!).

Release note: None

cockroach-teamcity · 2020-03-05T20:19:08Z

This change is

that was not tested before and three bug fixes Some of the following was based on looking at test coverage results: - added wait_self: covers reservation by a different request from the the same txn and lock acquisition by the same txn - we were not adding to the sequence nums in discoveredLock which was a bug found when printing the sequence numbers - dup_access: added test when a request is both reading and writing to a key and the lock is held after it stops waiting for write - added lock_changes: misc state changes for a lock that were not covered by existing tests - added ignored seqnum tests to the update testdata file. It found a bug - UpdateLocks now ignores updates with the same epoch but lower timestamp. It used to trigger an error after the ts had been updated which was severely muddled (tell the caller of the error after making the erroneous state change!). Release note: None

Needs a review to check that this is the desired behavior, after which I will add a test. Release note: None

sumeerbhola

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)

pkg/kv/kvserver/concurrency/lock_table.go, line 1454 at r2 (raw file):

		// Else txn.Epoch < holderTxn.Epoch, so only the timestamp has been
		// potentially updated.
		isLocked = true

Is this change from the second commit the desired behavior?

sumeerbhola

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)

pkg/kv/kvserver/concurrency/lock_table.go, line 1450 at r2 (raw file):

				continue
			}
			l.holder.holder[i].txn = txn

Should this assignment also be conditional on advancedTs. This unconditional assignment could mean that the timestamps inside txn and the sequence num are not monotonic.

nvb

I'm happy to see this testing expanded.

Reviewed 15 of 15 files at r1, 2 of 2 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @sumeerbhola)

pkg/kv/kvserver/concurrency/lock_table.go, line 699 at r1 (raw file):

	}
	waitingOnStr := func(txn *enginepb.TxnMeta, ts hlc.Timestamp) string {
		// TODO(sbhola): strip the leading 0 bytes from the UUID string since tests are assigning

If you're feeling inspired, you might consider doing something similar to a569548 to address this. You would only want to shorten the UUID when that doesn't lose information though because I expect that we will hook this up to debugging output sometime soon.

pkg/kv/kvserver/concurrency/lock_table.go, line 702 at r1 (raw file):

		// UUIDs using a counter and makes this output more readable.
		var seqStr string
		if txn.Sequence != 0 {

Should we get rid of this now?

pkg/kv/kvserver/concurrency/lock_table.go, line 707 at r1 (raw file):

		return fmt.Sprintf("txn: %v, ts: %v%s", txn.ID, ts, seqStr)
	}
	holderInfoStr := func() string {

Not that it really matters given the limited uses of this lockState.Format right now, but ideally we wouldn't create these intermediate strings and then write them to the top-level strings.Builder. This creates wasted memory allocations and copies.

pkg/kv/kvserver/concurrency/lock_table.go, line 718 at r1 (raw file):

			}
			first = false
			h := &l.holder.holder[i]

nit: move this directly below the for i := range l.holder.holder { line.

pkg/kv/kvserver/concurrency/lock_table.go, line 1414 at r1 (raw file):

	}

	ts := up.Txn.WriteTimestamp

minor nit: re-order this line and the next. We work with the tuple of (txn, ts) throughout this file.

pkg/kv/kvserver/concurrency/lock_table.go, line 1434 at r2 (raw file):

		// way of this request. For unreplicated locks the lock table is the
		// source of truth so we best-effort mirror the behavior of
		// mvccResolveWriteIntent() by updating the timestamp.

You might want to note that this isn't always going to be the case and that eventually, the lockTable will become the source of truth for replicated locks as well.

pkg/kv/kvserver/concurrency/lock_table.go, line 1450 at r2 (raw file):

Should this assignment also be conditional on advancedTs

Yes, I think so. I think we should be able to mirror the logic in MVCCResolveWriteIntent and only update the txn's WriteTimestamp

pkg/kv/kvserver/concurrency/lock_table.go, line 1454 at r2 (raw file):

Previously, sumeerbhola wrote…

Is this change from the second commit the desired behavior?

It seems desirable to me. Can you expand on why it wouldn't be?

pkg/kv/kvserver/concurrency/lock_table_test.go, line 79 at r1 (raw file):

 Releases locks for the named transaction.

update txn=<name> ts=<int>[,<int>] epoch=<int> span=<start>[,<end>] [ignored-seqs=<int>[,<int>]]

Any interest in supporting ranges of ignored-seqs? I think that would make this grammer look like [ignored-seqs=<int>[-<int>][,<int>[-<int>]]]. Would that exercise new code paths?

pkg/kv/kvserver/concurrency/lock_table_test.go, line 133 at r1 (raw file):

				make(map[string]lockTableGuard)
		}
		txnsByName, txnCounter, requestsByName, guardsByReqName := newState()

Is there a reason to create these before new-lock-table? We start each test off with new-lock-table, so we seem to be doing this all twice right at the beginning of a test.

I ask mostly because it would be nice to just inline the constructors in newState into new-lock-table.

pkg/kv/kvserver/concurrency/testdata/lock_table/dup_access, line 671 at r1 (raw file):

local: num=0

# req12 ignores the lock at "b" when it encounters it again as a reader.

What do you mean by "ignores"? Isn't it moving into the waitForDistinguished state? Do you mean that it's in this state as a writer and not a reader?

pkg/kv/kvserver/concurrency/testdata/lock_table/lock_changes, line 9 at r1 (raw file):

# Lock being released is held by a different transaction.
# ---------------------------------------------------------------------------------

I was recently reminded that datadriven tests have a notion of subtests. For instance, I think if you added subtest lock_released_by_different_transaction here, you would install a subtest until you either created a new subtest or issued subtest end. It might be worth playing around with because these sequences of tests are so structured.

pkg/kv/kvserver/concurrency/testdata/lock_table/lock_changes, line 26 at r1 (raw file):

local: num=0

print

Any reason why you're printing after each acquire and release, even though they also do the same?

pkg/kv/kvserver/concurrency/testdata/lock_table/lock_changes, line 72 at r1 (raw file):


# ---------------------------------------------------------------------------------
# Reader waits until it the timestamp of the lock is updated.

s/it the/the/

pkg/kv/kvserver/concurrency/testdata/lock_table/lock_changes, line 82 at r1 (raw file):

start-waiting: true

guard-state r=req3

Could you inspect the guard-state after the reader is released?

pkg/kv/kvserver/concurrency/testdata/lock_table/lock_changes, line 137 at r1 (raw file):

global: num=1
 lock: "a"
  holder: txn: 00000000-0000-0000-0000-000000000001, ts: 0.000000014,0, seq: 1, info: epoch: 1, seqs: [1], epoch: 1, seqs: [0, 1]

It seems like we should add the durability of the guard to the info text. This is ambiguous.

pkg/kv/kvserver/concurrency/testdata/lock_table/update, line 302 at r1 (raw file):

# Lock is held at multiple seqnums and then updated to ignore
# either a seqnum that is not in the held list or one that is
# in the list. When all the seqnumes are ignored, the lock is

s/seqnumes/seqnums/

pkg/kv/kvserver/concurrency/testdata/lock_table/update, line 303 at r1 (raw file):

# either a seqnum that is not in the held list or one that is
# in the list. When all the seqnumes are ignored, the lock is
# no longer held.

s/no longer held/released/

pkg/kv/kvserver/concurrency/testdata/lock_table/wait_self, line 29 at r1 (raw file):

----

new-request r=req4 txn=txn2 ts=10 spans=w@a

nit: I think this test would be easier to read if you renamed req2 and req4 to req2a and req2b to make it clear that they're part of the same transaction.

comments Release note: None

sumeerbhola

Thanks for the review. Addressed all comments and added a test for the changed UpdateLock() behavior.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)

pkg/kv/kvserver/concurrency/lock_table.go, line 699 at r1 (raw file):