kv,storage: omit MVCC timestamps for values read from intents of the current transaction

[ajwerner]

Is your feature request related to a problem? Please describe.

The KV layer provides MVCC timestamps with data returned by reading requests. These MVCC timestamps power the crdb_internal_mvcc_timestamp (https://github.com/cockroachdb/cockroach/pull/51494). They are also used by the SQL layer to determine the validity interval of descriptor versions (https://github.com/cockroachdb/cockroach/pull/40793).

One oddity is that the timestamps carried on intents is volatile; transactions can get pushed and may commit with a later timestamp. Given that, it's not really safe to use that timestamp to power internals required for correctness and it's confusing for users of crdb_internal_mvcc_timestamp which observe their own writes.

Describe the solution you'd like

In order to avoid these hazards, the MVCC layer could omit populating the timestamps from values due to intents. Then, in the SQL layer which renders these timestamps, could convert the value to NULL.

Additional context

One thing to note is that I believe that we are (but soon won't be) doing something hazardous that was only subtly safe. In particular, we had a crash due to an assertion violation because when we re-resolve mutable descriptors inside a transaction we would set the ModificationTime to the provisional commit timestamp (https://github.com/cockroachdb/cockroach/pull/52358). That will be fixed in https://github.com/cockroachdb/cockroach/pull/56663 and bring back a bit of sanity.

In fact, now that I'm typing this, I'm a bit afraid we've got a bug lurking where we may populate the ModificationTime with a timestamp that precedes the commit timestamp. In practice, I'm hopeful that this could be okay, but I definitely have some concerns.

Jira issue: CRDB-3442

[blathers-crl[bot]]

Hi @ajwerner, I've guessed the C-ategory of your issue and suitably labeled it. Please re-label if inaccurate.

While you're here, please consider adding an A- label to help keep our repository tidy.

_{:owl: Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[knz]

cc @nvanbenschoten @tbg for triage

[ajwerner]

I'm putting this in the KV backlog even though the bulk of the business logic will be in the MVCC layer because, well, it's really about the implication to the KV protocol and not about the behavior of the MVCC layer. I'm still a bit unclear on what is owned by storage vs. kv when it comes to the MVCC code.

[github-actions[bot]]

We have marked this issue as stale because it has been inactive for 18 months. If this issue is still relevant, removing the stale label or adding a comment will keep it active. Otherwise, we'll close it in 10 days to keep the issue queue tidy. Thank you for your contribution to CockroachDB!

[knz]

still relevant.