storage: support no-op PUSH_TIMESTAMP pushes on STAGING transactions #36253

nvanbenschoten · 2019-03-28T00:07:24Z

Caught while manually testing parallel commits last week.

PR #35763 made it an error to resolve an intent with a STAGING status.
This isn't a crazy thing to do in every circumstance though. The case
where this comes up is a PUSH_TIMESTAMP push on a STAGING transaction
whose timestamp is already sufficiently high. In this case, the pusher
can move the intent up out of its way without modifying the transaction
record or interfering with the parallel commit.

Concretely, this is allowed in pushes that hit this case:

cockroach/pkg/storage/batcheval/cmd_push_txn.go

Line 197 in 4ab679d

    
           if args.PushType == roachpb.PUSH_TIMESTAMP && !reply.PusheeTxn.Timestamp.Less(args.PushTo) {

To test this, the commit extends TestStoreResolveWriteIntentPushOnRead
to test scenarios in two different dimensions: PENDING vs. STAGING
transaction records and already pushed txns vs. not already pushed
txns. To test the latter dimensions, the commit had to refine how
far into the future transactions push conflicting intents. Previously,
transactions would push them all the way to hlc.Now() on the pushing
node so that there was no chance that they would be in their uncertainty
window after the push. This was pessimistic. The transaction only needs
to push the conflicting intent up to its observed timestamp for the
pushing node, which may be significantly lower than the current timestamp
on the pushing node. This should increase the number of no-op pushes we
see in the wild.

Release note: None

cockroach-teamcity · 2019-03-28T00:07:30Z

This change is

tbg

Reviewed 8 of 8 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)

pkg/storage/store.go, line 2993 at r1 (raw file):

					obsTS, ok := h.Txn.GetObservedTimestamp(ba.Replica.NodeID)
					if !ok {
						// We just set this above if it wasn't already set.

The "above" is a little far away. Maybe say "This was set earlier in this method, so it's completely unexpected to not be found now."

pkg/storage/store_test.go, line 1666 at r1 (raw file):

	testCases := []struct {
		pusherMaxPriority   bool

Rename this to pusherWillWin which is how this is used for all I can see

pkg/storage/store_test.go, line 1667 at r1 (raw file):

	testCases := []struct {
		pusherMaxPriority   bool
		pusheeAlreadyPushed bool

// If true, pushee's timestamp will be set above pusher's target timestamp

pkg/storage/store_test.go, line 1668 at r1 (raw file):

		pusherMaxPriority   bool
		pusheeAlreadyPushed bool
		pusheeStagingRecord bool

// If true, pushee is STAGING, otherwise PENDING

pkg/storage/store_test.go, line 1721 at r1 (raw file):

		},
		{
			// Already pushed, no-op.

// Already pushed the STAGING record, no-op.

pkg/storage/store_test.go, line 1729 at r1 (raw file):

		},
		{
			// Already pushed, no-op.

ditto.

pkg/storage/batcheval/cmd_resolve_intent.go, line 85 at r1 (raw file):

	}
	if args.Status == roachpb.STAGING {
		return result.Result{}, errors.Errorf("cannot resolve intent with STAGING status")

Does this affect your txn record diagram? Just checking.

pkg/storage/engine/mvcc.go, line 2404 at r1 (raw file):

	}

	// Otherwise, we're deleting the intent, which includes deleting the

Always makes me nervous to change something near this default behavior, but I triple checked and it checks out.

Caught while manually testing parallel commits last week. PR cockroachdb#35763 made it an error to resolve an intent with a STAGING status. This isn't a crazy thing to do in every circumstance though. The case where this comes up is a PUSH_TIMESTAMP push on a STAGING transaction whose timestamp is already sufficiently high. In this case, the pusher can move the intent up out of its way without modifying the transaction record or interfering with the parallel commit. Concretely, this is allowed in pushes that hit this case: https://github.com/cockroachdb/cockroach/blob/4ab679d978a8f566c1427b372547380ee012292f/pkg/storage/batcheval/cmd_push_txn.go#L197 To test this, the commit extends `TestStoreResolveWriteIntentPushOnRead` to test scenarios in two different dimensions: PENDING vs. STAGING transaction records and already pushed txns vs. not already pushed txns. To test the latter dimensions, the commit had to refine how far into the future transactions push conflicting intents. Previously, transactions would push them all the way to hlc.Now() on the pushing node so that there was no chance that they would be in their uncertainty window after the push. This was pessimistic. The transaction only needs to push the conflicting intent up to its observed timestamp for the pushing node, which may be significantly lower than the current timestamp on the pushing node. This should increase the number of no-op pushes we see in the wild. Release note: None

nvanbenschoten

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

pkg/storage/store.go, line 2993 at r1 (raw file):