New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

storage: Collect IntentHistory for transactions #32688

Merged
merged 1 commit into from Dec 6, 2018

Conversation

Projects
None yet
4 participants
@ridwanmsharif
Copy link
Collaborator

ridwanmsharif commented Nov 28, 2018

First part of making transactions more idempotent. This change
adds history of all writes on a key by the transaction. This
can be used to create save points and rollback to certain points
inside a transaction. More on why this is needed is explained here:
#5861 (comment)

Release note: None

@ridwanmsharif ridwanmsharif requested a review from cockroachdb/core-prs as a code owner Nov 28, 2018

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

cockroach-teamcity commented Nov 28, 2018

This change is Reviewable

@nvanbenschoten

This comment has been minimized.

Copy link
Member

nvanbenschoten commented Nov 28, 2018

I'll take a look at this shortly, but this could really use some testing. mvcc_test.go is where I would start, then I'd add some higher-level tests to replica_test.go.

@ridwanmsharif ridwanmsharif force-pushed the ridwanmsharif:intent-history branch 5 times, most recently from 679711a to 6c62331 Nov 29, 2018

@bdarnell
Copy link
Member

bdarnell left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/storage/engine/enginepb/mvcc.proto, line 78 at r1 (raw file):

  // returns the value at a given sequence.
  // History will be empty for non-transactional requests.
  optional IntentHistory history = 8 [(gogoproto.nullable) = false];

Adding non-nullable optional fields to below-raft protos is not generally safe. Repeated fields are fine, though, so I think you can just remove the IntentHistory wrapper.

@ridwanmsharif ridwanmsharif force-pushed the ridwanmsharif:intent-history branch 2 times, most recently from 5718339 to d67b7fc Dec 3, 2018

@ridwanmsharif ridwanmsharif requested a review from nvanbenschoten Dec 3, 2018

@nvanbenschoten
Copy link
Member

nvanbenschoten left a comment

Haven't gotten to the tests yet, but this is looking good so far.

Reviewed 9 of 12 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/storage/engine/mvcc.go, line 1116 at r2 (raw file):

}

// getExistingValue returns the value of the data read at readTS from the buffer pool.

Let's inline this instead of pulling it out. We do so in mvccResolveWriteIntent and it doesn't seem to be a problem.


pkg/storage/engine/mvcc.go, line 1272 at r2 (raw file):

			cleanup()

			// TODO(ridwanmsharif): Confirm this is correct.

Yep 👍


pkg/storage/engine/mvcc.go, line 1277 at r2 (raw file):

			// version.  For example, a conditional put within same
			// transaction should read previous write.
			if value, err = maybeGetValue(

Can we avoid reading the previous intent value twice?


pkg/storage/engine/enginepb/mvcc.go, line 134 at r2 (raw file):

// AddToHistory adds the sequence and value to the intent history.
func (meta *MVCCMetadata) AddToHistory(seq int32, val []byte, deleted bool) {

s/AddToHistory/AddToIntentHistory/


pkg/storage/engine/enginepb/mvcc.proto, line 27 at r2 (raw file):

// tombstone - to be stored in an IntentHistory of a key during a transaction.
// Making it possible for a transaction to be more idempotent
message Entry {

This name should be more specific. What does enginepb.Entry mean? I'd make it a nested message within MVCCMetadata and give it a more specific name. Maybe SequencedIntent.


pkg/storage/engine/enginepb/mvcc.proto, line 33 at r2 (raw file):

  // IntentHistory.GetValue(seq) returns the value
  // in history if it exists.

This comment about a method on MVCCMetadata doesn't belong here.


pkg/storage/engine/enginepb/mvcc.proto, line 39 at r2 (raw file):

  optional bytes value = 2;
  // Is this value in the intent history a deletion tombstone?
  optional bool deleted = 3 [(gogoproto.nullable) = false];

Does this need to be separate from value == nil?


pkg/storage/engine/enginepb/mvcc.proto, line 69 at r2 (raw file):

  optional bytes raw_bytes = 6;
  // IntentHistory of the transaction stores all they values the txn wrote
  // for the key along with each values corresponding Sequence.

Not "all" versions, just all non-live versions, right?

@ridwanmsharif ridwanmsharif force-pushed the ridwanmsharif:intent-history branch from d67b7fc to 378b2bc Dec 4, 2018

@ridwanmsharif
Copy link
Collaborator

ridwanmsharif left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/storage/engine/mvcc.go, line 1116 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Let's inline this instead of pulling it out. We do so in mvccResolveWriteIntent and it doesn't seem to be a problem.

Done.


pkg/storage/engine/mvcc.go, line 1272 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Yep 👍

Done.


pkg/storage/engine/mvcc.go, line 1277 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Can we avoid reading the previous intent value twice?

Done.


pkg/storage/engine/enginepb/mvcc.go, line 134 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

s/AddToHistory/AddToIntentHistory/

Done.


pkg/storage/engine/enginepb/mvcc.proto, line 78 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Adding non-nullable optional fields to below-raft protos is not generally safe. Repeated fields are fine, though, so I think you can just remove the IntentHistory wrapper.

Done. The empty checksum has not changed but the populated checksum for the below raft protos test did, which is what we want I think.


pkg/storage/engine/enginepb/mvcc.proto, line 27 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This name should be more specific. What does enginepb.Entry mean? I'd make it a nested message within MVCCMetadata and give it a more specific name. Maybe SequencedIntent.

Done.


pkg/storage/engine/enginepb/mvcc.proto, line 33 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
  // IntentHistory.GetValue(seq) returns the value
  // in history if it exists.

This comment about a method on MVCCMetadata doesn't belong here.

Done.


pkg/storage/engine/enginepb/mvcc.proto, line 39 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Does this need to be separate from value == nil?

It doesn't, I'd misunderstood what the Deleted bool on the MVCCMetadata object was doing. Fixed.


pkg/storage/engine/enginepb/mvcc.proto, line 69 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Not "all" versions, just all non-live versions, right?

Done.

@ridwanmsharif ridwanmsharif requested a review from nvanbenschoten Dec 4, 2018

@nvanbenschoten
Copy link
Member

nvanbenschoten left a comment

Almost there. The testing is solid.

Reviewed 9 of 10 files at r3.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/storage/engine/mvcc.go, line 1221 at r3 (raw file):

	var maybeTooOldErr error
	var prevValSize int64
	var prevValBytes []byte

These are referring to two different "prev vals", right? One is the previous intent value is one is the previous committed value. I'd rename prevValBytes to prevIntentValBytes and prevSequence to prevIntentSequence.

Also, move this into the ok && meta.Txn != nil block since we only need it there.


pkg/storage/engine/mvcc.go, line 1255 at r3 (raw file):

				return err
			}
			if existingVal != nil {

Can this case exist? If not, should we throw an error?


pkg/storage/engine/mvcc.go, line 1259 at r3 (raw file):

			}
			prevSequence := meta.Txn.Sequence
			// Make sure we process valueFn before clearing any earlier

minor nit: add a line break above this comment.


pkg/storage/engine/mvcc.go, line 1268 at r3 (raw file):

				}
			}
			// Release the buffer after using the existing value.

We're leaking this in two different places. I'd defer it above.


pkg/storage/engine/mvcc.go, line 1305 at r3 (raw file):

				timestamp = metaTimestamp
			}
			// Since a transaction exists for the MVCCMetadata, we must add the

Not just any transaction, our transaction. I'd rephase to "since an intent with a smaller sequence number for the same transaction exists, we must ..."


pkg/storage/engine/mvcc.go, line 1307 at r3 (raw file):

			// Since a transaction exists for the MVCCMetadata, we must add the
			// previous value and sequence to the intent history.
			meta.AddToIntentHistory(prevSequence, prevValBytes)

I'm a little worried about modifying the meta directly. Can we avoid that?


pkg/storage/engine/mvcc_test.go, line 4384 at r3 (raw file):

// TestMVCCIntentHistory verifies that trying to write to a key that already was written
// to - results in the history being recorded in the MVCCMetadata.

Why the dash?


pkg/storage/engine/mvcc_test.go, line 4385 at r3 (raw file):

// TestMVCCIntentHistory verifies that trying to write to a key that already was written
// to - results in the history being recorded in the MVCCMetadata.
func TestMVCCIntentHistory(t *testing.T) {

Nice test!


pkg/storage/engine/enginepb/mvcc.proto, line 69 at r2 (raw file):

Previously, ridwanmsharif (Ridwan Sharif) wrote…

Done.

Add another sentence stating that it doesn't include the latest intent on the key, just those that have been overwritten.

@ridwanmsharif ridwanmsharif force-pushed the ridwanmsharif:intent-history branch from 378b2bc to b4832d6 Dec 5, 2018

@ridwanmsharif
Copy link
Collaborator

ridwanmsharif left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/storage/engine/mvcc.go, line 1221 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

These are referring to two different "prev vals", right? One is the previous intent value is one is the previous committed value. I'd rename prevValBytes to prevIntentValBytes and prevSequence to prevIntentSequence.

Also, move this into the ok && meta.Txn != nil block since we only need it there.

Done.


pkg/storage/engine/mvcc.go, line 1255 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Can this case exist? If not, should we throw an error?

So currently this can happen when a transaction encounters an intent with a lower epoch. As a result it can't read the value using mvccGetInternal. I added a comment here but essentially, this shouldn't be a problem because in this case, the intent history is blown away anyway.


pkg/storage/engine/mvcc.go, line 1268 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

We're leaking this in two different places. I'd defer it above.

Done.


pkg/storage/engine/mvcc.go, line 1305 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Not just any transaction, our transaction. I'd rephase to "since an intent with a smaller sequence number for the same transaction exists, we must ..."

Done.


pkg/storage/engine/mvcc.go, line 1307 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I'm a little worried about modifying the meta directly. Can we avoid that?

Done. Updating buf.newMeta instead.


pkg/storage/engine/mvcc_test.go, line 4384 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Why the dash?

Done.


pkg/storage/engine/mvcc_test.go, line 4385 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Nice test!

Done.


pkg/storage/engine/enginepb/mvcc.proto, line 69 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Add another sentence stating that it doesn't include the latest intent on the key, just those that have been overwritten.

Done.

@ridwanmsharif ridwanmsharif requested a review from nvanbenschoten Dec 5, 2018

@nvanbenschoten
Copy link
Member

nvanbenschoten left a comment

:lgtm: once the final two comments are addressed. Nice work on this!

Reviewed 1 of 4 files at r4.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained


pkg/storage/engine/mvcc.go, line 1255 at r3 (raw file):

Previously, ridwanmsharif (Ridwan Sharif) wrote…

So currently this can happen when a transaction encounters an intent with a lower epoch. As a result it can't read the value using mvccGetInternal. I added a comment here but essentially, this shouldn't be a problem because in this case, the intent history is blown away anyway.

Consider asserting that.


pkg/storage/engine/mvcc.go, line 1376 at r4 (raw file):

		}
		buf.newMeta.Txn = txnMeta
		buf.newMeta.Timestamp = hlc.LegacyTimestamp(timestamp)

Didn't we already do this above?

@bdarnell
Copy link
Member

bdarnell left a comment

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained


pkg/storage/engine/enginepb/mvcc.proto, line 78 at r1 (raw file):

Previously, ridwanmsharif (Ridwan Sharif) wrote…

Done. The empty checksum has not changed but the populated checksum for the below raft protos test did, which is what we want I think.

Yes, that's right.

@ridwanmsharif ridwanmsharif force-pushed the ridwanmsharif:intent-history branch from b4832d6 to 9b61df2 Dec 6, 2018

@ridwanmsharif
Copy link
Collaborator

ridwanmsharif left a comment

TFTR!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


pkg/storage/engine/mvcc.go, line 1255 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Consider asserting that.

Done.


pkg/storage/engine/mvcc.go, line 1376 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Didn't we already do this above?

Done.

@nvanbenschoten
Copy link
Member

nvanbenschoten left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


pkg/storage/engine/mvcc.go, line 1321 at r5 (raw file):

			// If the epoch of the transaction doesn't match the epoch of the
			// intent, blow away the intent history.
			if txn.Epoch <= meta.Txn.Epoch {

Do we have any tests for txn.Epoch < meta.Txn.Epoch? Don't we already return an error in that case?


pkg/storage/engine/mvcc.go, line 1322 at r5 (raw file):

			// intent, blow away the intent history.
			if txn.Epoch <= meta.Txn.Epoch {
				// I don't know where this case could pop up, but it is worth

nit: Comments are generally not in the first person.


pkg/storage/engine/mvcc.go, line 1326 at r5 (raw file):

				// to the history
				if existingVal == nil {
					return errors.Errorf("previous intent could not be read")

Leave a bit more context here. For instance, take a look at some of the warnings in mvccResolveWriteIntent.

@ridwanmsharif ridwanmsharif force-pushed the ridwanmsharif:intent-history branch from 9b61df2 to fc4d9bf Dec 6, 2018

@ridwanmsharif
Copy link
Collaborator

ridwanmsharif left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


pkg/storage/engine/mvcc.go, line 1321 at r5 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Do we have any tests for txn.Epoch < meta.Txn.Epoch? Don't we already return an error in that case?

Done. Should've been ==


pkg/storage/engine/mvcc.go, line 1326 at r5 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Leave a bit more context here. For instance, take a look at some of the warnings in mvccResolveWriteIntent.

Done.

@nvanbenschoten
Copy link
Member

nvanbenschoten left a comment

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@ridwanmsharif

This comment has been minimized.

Copy link
Collaborator

ridwanmsharif commented Dec 6, 2018

bors r+

@craig

This comment has been minimized.

Copy link

craig bot commented Dec 6, 2018

Merge conflict (retrying...)

@craig

This comment has been minimized.

Copy link

craig bot commented Dec 6, 2018

Merge conflict

storage: Collect IntentHistory for transactions
First part of making transactions more idempotent. This change
adds history of all writes on a key by the transaction. This
can be used to create save points and rollback to certain points
inside a transaction.

Release note: None

@ridwanmsharif ridwanmsharif force-pushed the ridwanmsharif:intent-history branch from fc4d9bf to b1e0f21 Dec 6, 2018

@ridwanmsharif

This comment has been minimized.

Copy link
Collaborator

ridwanmsharif commented Dec 6, 2018

bors r+

craig bot pushed a commit that referenced this pull request Dec 6, 2018

Merge #32688
32688: storage: Collect IntentHistory for transactions r=ridwanmsharif a=ridwanmsharif

First part of making transactions more idempotent. This change
adds history of all writes on a key by the transaction. This
can be used to create save points and rollback to certain points
inside a transaction. More on why this is needed is explained here:
#5861 (comment)

Release note: None

Co-authored-by: Ridwan Sharif <ridwan@cockroachlabs.com>
@craig

This comment has been minimized.

Copy link

craig bot commented Dec 6, 2018

Build succeeded

@craig craig bot merged commit b1e0f21 into cockroachdb:master Dec 6, 2018

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@ridwanmsharif ridwanmsharif deleted the ridwanmsharif:intent-history branch Dec 12, 2018

craig bot pushed a commit that referenced this pull request Dec 12, 2018

Merge #33001
33001: storage: allow transactions to run at a lower sequence r=ridwanmsharif a=ridwanmsharif

Continuing off #32688, final part of #5861 (comment).

Adds support to have transactions run at a lower sequence
at a given key. It asserts the value it computes with the
value written for the sequence in the intent history instead
or returning a retry-able error.

Release note: None

Co-authored-by: Ridwan Sharif <ridwan@cockroachlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment