metamorphic: add MVCC{CheckFor,}AcquireLock operations #113698

itsbilal · 2023-11-02T19:20:30Z

This change adds two new operations to the MVCC metamorphic testing package that were added in #110323. We check for determinism in the storage package by adding these operations to the metamorphic tests as well.

Fixes #109650.

Epic: none

Release note: None

blathers-crl · 2023-11-02T19:20:33Z

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

cockroach-teamcity · 2023-11-02T19:20:40Z

This change is

RaduBerinde

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

pkg/storage/metamorphic/operations.go line 271 at r1 (raw file):

	_, err := storage.MVCCInitPut(ctx, writer, m.key, txn.ReadTimestamp, m.value, false, storage.MVCCWriteOptions{Txn: txn})
	if err != nil {
		if writeTooOldErr := (*kvpb.WriteTooOldError)(nil); errors.As(err, &writeTooOldErr) {

[nit] It might make sense to separate this into a separate handleWriteError(txn, key, err) function (it's duplicated three times).

nvanbenschoten

Thanks @itsbilal! This is really great.

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

pkg/storage/metamorphic/operations.go line 294 at r1 (raw file):

func (m mvccCheckForAcquireLockOp) run(ctx context.Context) string {
	txn := m.m.getTxn(m.txn)
	txn.Sequence++

Here and below: we don't need to advance the write sequence for lock acquisition.

pkg/storage/metamorphic/operations.go line 297 at r1 (raw file):

	writer := m.m.getReadWriter(m.writer)

	err := storage.MVCCCheckForAcquireLock(ctx, writer, txn, m.strength, m.key, 64)

The maxLockConflicts value is interesting here. We set it to 64 for these two functions, but set it for other MVCC operations (hidden in MVCCGetOptions, MVCCScanOptions, and MVCCWriteOptions). Would it make sense to randomize this value for all users?

pkg/storage/metamorphic/operations.go line 302 at r1 (raw file):

	}

	// Update the txn's lock spans to account for this intent being written.

Stray comment.

pkg/storage/metamorphic/operations.go line 321 at r1 (raw file):

	err := storage.MVCCAcquireLock(ctx, writer, txn, m.strength, m.key, nil, 64)
	if err != nil {
		if writeTooOldErr := (*kvpb.WriteTooOldError)(nil); errors.As(err, &writeTooOldErr) {

I don't think this is needed for MVCCAcquireLock. It should never return a WriteTooOld error, because those are due to write-write version conflicts instead of locking conflicts.

pkg/storage/metamorphic/operations.go line 329 at r1 (raw file):

	}

	// Update the txn's lock spans to account for this intent being written.

s/intent being written/lock being acquired/.

pkg/storage/metamorphic/operations.go line 993 at r1 (raw file):

	},
	{
		name: "mvcc_acquire_lock",

nit: want to order these the same as the methods above?

pkg/storage/metamorphic/operations.go line 1004 at r1 (raw file):

			// Track this write in the txn generator. This ensures the batch will be
			// committed before the transaction is committed

nit: missing punctuation.

This change adds two new operations to the MVCC metamorphic testing package that were added in cockroachdb#110323. We check for determinism in the storage package by adding these operations to the metamorphic tests as well. Fixes cockroachdb#109650. Epic: none Release note: None

itsbilal

TFTR!

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

pkg/storage/metamorphic/operations.go line 271 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] It might make sense to separate this into a separate handleWriteError(txn, key, err) function (it's duplicated three times).

Done (not relevant anymore for this PR).

pkg/storage/metamorphic/operations.go line 294 at r1 (raw file):