storage: override MVCCMetadata.TxnDidNotUpdateMeta in mixed version c… #70267

sumeerbhola · 2021-09-15T16:13:04Z

…luster

Specifically, if the cluster version indicates that there can
be nodes with the broken SingleDelete logic for separated
intent resolution, the MVCCMetadata.TxnDidNotUpdateMeta
field will never be set to true.

See code comments and #69891 (comment)

Informs #69891

Release justification: fix for a release blocker that causes incorrect
behavior for transactional writes.

Release note: None

cockroach-teamcity · 2021-09-15T16:13:14Z

This change is

sumeerbhola · 2021-09-16T19:12:17Z

@tbg @nvanbenschoten reminder to take a look, since this is potentially a release blocker.

tbg

Apologies for the delay, I should have looked at this much sooner.

Reviewed 16 of 16 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @nicktrav, @nvanbenschoten, and @sumeerbhola)

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

// PutIntent.
//
// Note that all nodes in this cluster understand separated intents, so can

Could you give this comment another pass? I think it is a bit confusing to read. Here are some ideas

Note that separated intents are understood by v21.1 already, though we assume they were never actively used there (the cluster setting is experimental, and there are known bugs). When migrating into 21.2 however, if 21.2

pkg/storage/testdata/mvcc_histories/conditional_put_with_txn_enable_separated_mixed_cluster, line 16 at r1 (raw file):

>> at end:
txn: "A" meta={id=00000000 key=/Min pri=0.00000000 epo=0 ts=123.000000000,0 min=0,0 seq=1} lock=true stat=PENDING rts=123.000000000,0 wto=false gul=0,0
meta: "k"/0,0 -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=123.000000000,0 min=0,0 seq=1} ts=123.000000000,0 del=false klen=12 vlen=6 mergeTs=<nil> txnDidNotUpdateMeta=false

the txnDidNotUpdateMeta=false is the important bit, right? Mind adding this in a comment.

pkg/storage/testdata/mvcc_histories/intent_history_enable_separated_mixed_cluster, line 31 at r1 (raw file):

>> put v=first k=a t=A
called PutIntent("a", _, NoExistingIntent, TDNUM(true), 00000000-0000-0000-0000-000000000002)
meta: "a"/0,0 -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=2.000000000,0 min=0,0 seq=0} ts=2.000000000,0 del=false klen=12 vlen=10 mergeTs=<nil> txnDidNotUpdateMeta=false

ditto

pkg/storage/testdata/mvcc_histories/intent_with_write_tracing_enable_separated_mixed_cluster, line 10 at r1 (raw file):

>> put k=k1 v=v1 t=A
called PutIntent("k1", _, NoExistingIntent, TDNUM(true), 00000000-0000-0000-0000-000000000001)
meta: "k1"/0,0 -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=2.000000000,0 min=0,0 seq=0} ts=2.000000000,0 del=false klen=12 vlen=7 mergeTs=<nil> txnDidNotUpdateMeta=false

ditto

pkg/storage/testdata/mvcc_histories/no_read_after_abort_enable_separated_mixed_cluster, line 14 at r1 (raw file):

>> put v=cde t=A k=a
called PutIntent("a", _, NoExistingIntent, TDNUM(true), 00000000-0000-0000-0000-000000000001)
meta: "a"/0,0 -> txn={id=00000000 key="a" pri=0.00000000 epo=0 ts=22.000000000,0 min=0,0 seq=0} ts=22.000000000,0 del=false klen=12 vlen=8 mergeTs=<nil> txnDidNotUpdateMeta=false

ditto (here and all the other files). Maybe a header in all of these separated_mixed files explaning that the absence of txnDidNotUpdateMeta=true is the important bit.

tbg

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

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

}

func (t txnDidNotUpdateMetaHelper) valueForPutIntent() bool {

If the cluster version isn't active yet, this is always going to return false, right? Because a true value would have to be emitted from populateMeta at some point in the past, and that is impossible, right? Just checking my understanding.

sumeerbhola

no problem, and thanks for the review!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @nicktrav, @nvanbenschoten, and @tbg)

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