The challenge was presumed to be the need to expose MVCC timestamps in our
client library. It turns out we seem to do that already (how did nobody know
that?!).

<Raises hand sheepishly>, though note that ScanRequest will only return timestamps if the slower KEY_VALUES format is used.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @lucy-zhang)

---

pkg/ccl/changefeedccl/table_history.go, line 286 at r1 (raw file):

				}
				if tableDesc := desc.GetTable(); tableDesc != nil {
					tableDesc.MaybeSetModificationTimeFromMVCCTimestamp(ctx, it.UnsafeKey().Timestamp)

How bad is it going to be if you miss one of the places that table descriptors are decoded? I'm wondering if there is anything that could be done to make this less error prone. For example, could we change the signature for desc.GetTable() to pass in a timestamp which would force you to update all of the callers. Seems like there are too many call sites for that, but maybe that sparks a better idea.

---

pkg/internal/client/db.go, line 34 at r1 (raw file):

type KeyValue struct {
	Key roachpb.Key
	// TODO(ajwerner): figure out why it says timestamp will always be zero

When using ScanFormat.BATCH_RESPONSE, the ScanResponse.BatchResponses field will contain encoded key/value pairs which do not have a timestamp. See enginepb.ScanDecodeKeyValueNoTS. I think the value in GetResponse will always contain a timestamp, though I'm not 100% sure.

---

pkg/sql/sqlbase/structured.go, line 1509 at r1 (raw file):

	// transaction will commit at the CommitTimestamp().
	//
	// TODO(vivek): Stop needing to do this by deprecating the

You're addressing this TODO in this PR. Is there anything left to do afterwards?

---

pkg/sql/sqlbase/structured.go, line 3722 at r1 (raw file):

			"its MVCC timestamp: has %v, expected less than %v %+v",
			desc.ModificationTime, ts, desc)
	}

I was expecting desc.ModificationTime to be set to ts if desc.ModificationTime < ts. It took me a moment to understand that desc.ModificationTime.IsEmpty() indicates that the modification time should be set from the MVCC timestamp and any non-empty values means that it was set by MaybeIncrementVersion. I see this is all explained in the comment for ModificationTime, but I just hadn't reached that part of the review yet. Wouldn't hurt to copy some of those words here as well.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @lucy-zhang and @petermattis)

---

pkg/ccl/changefeedccl/table_history.go, line 286 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

How bad is it going to be if you miss one of the places that table descriptors are decoded? I'm wondering if there is anything that could be done to make this less error prone. For example, could we change the signature for desc.GetTable() to pass in a timestamp which would force you to update all of the callers. Seems like there are too many call sites for that, but maybe that sparks a better idea.

It can be pretty bad. I missed the case in ConditionalGetTableDescFromTxn and it made the test fail. That specific method probably doesn't need to decode the table descriptor itself and could probably use one of the helpers from that package.

In an earlier iteration I attempted to rename the proto TableDescriptor to RawTableDescriptor and then have a TableDescriptor type that wraps the RawTableDescriptor to enforce that the modification time was properly updated. That turned out to be pretty heavy and I was making a mess and preventing myself from getting to a place where I could understand if this change works. Maybe it is worth walking down that path.

I don't know to how hard it would be to write a linter that checks if any function unmarshals into a TableDescriptor or Descriptor and then doesn't call MaybeSetModificationTimeFromMVCCTimestamp before returning. I guess now could be a good time to try out https://godoc.org/golang.org/x/tools/go/analysis

---

pkg/internal/client/db.go, line 34 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

When using ScanFormat.BATCH_RESPONSE, the ScanResponse.BatchResponses field will contain encoded key/value pairs which do not have a timestamp. See enginepb.ScanDecodeKeyValueNoTS. I think the value in GetResponse will always contain a timestamp, though I'm not 100% sure.

Yeah I noticed that. We only ever seem to use that in the KV fetcher here

and we don't ever fetch these things as sql rows so it should be okay. I'll update the comment to explain when this is zero.
how

---

pkg/sql/sqlbase/structured.go, line 1509 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

You're addressing this TODO in this PR. Is there anything left to do afterwards?

Nope. Done.

---

pkg/sql/sqlbase/structured.go, line 3722 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I was expecting desc.ModificationTime to be set to ts if desc.ModificationTime < ts. It took me a moment to understand that desc.ModificationTime.IsEmpty() indicates that the modification time should be set from the MVCC timestamp and any non-empty values means that it was set by MaybeIncrementVersion. I see this is all explained in the comment for ModificationTime, but I just hadn't reached that part of the review yet. Wouldn't hurt to copy some of those words here as well.

I initially wanted it to be the case that if ModificationTime was non-zero then it would have to be exactly equal to the MVCC timestamp. Unfortunately there are places where we update these descriptors in place without touching the version or modification time (schema leases being the best example). Any time we do that now it will upgrade the descriptor to explicitly knowing about its ModificationTime.

This way ModificationTime will either be zero and we know that it's the first value for this version and its MVCC timestamp can be used or it's non-zero and either it was written by an older version of crdb or we've updated it in place. I've add more commentary.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @lucy-zhang, and @petermattis)

---

pkg/ccl/changefeedccl/table_history.go, line 286 at r1 (raw file):

Previously, ajwerner wrote…

It can be pretty bad. I missed the case in ConditionalGetTableDescFromTxn and it made the test fail. That specific method probably doesn't need to decode the table descriptor itself and could probably use one of the helpers from that package.

In an earlier iteration I attempted to rename the proto TableDescriptor to RawTableDescriptor and then have a TableDescriptor type that wraps the RawTableDescriptor to enforce that the modification time was properly updated. That turned out to be pretty heavy and I was making a mess and preventing myself from getting to a place where I could understand if this change works. Maybe it is worth walking down that path.

I don't know to how hard it would be to write a linter that checks if any function unmarshals into a TableDescriptor or Descriptor and then doesn't call MaybeSetModificationTimeFromMVCCTimestamp before returning. I guess now could be a good time to try out https://godoc.org/golang.org/x/tools/go/analysis

Well, there are only 46 callers of GetTable(), perhaps my idea of changing the signature isn't too onerous.

---

pkg/sql/sqlbase/structured.go, line 3718 at r2 (raw file):

//
// TODO(ajwerner): devise a mechanism to ensure that this method is always
// called when decoding table descriptors.

+1 to the TODO.

I added a new accessor method for the TableDescriptor which takes a timestamp and validates it. There's a heavy linter too, I wanted to make it do more but alas. RFAL.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @lucy-zhang, and @petermattis)

---

pkg/ccl/backupccl/backup.go, line 307 at r4 (raw file):

				"%s: unable to unmarshal SQL descriptor", row.Key)
		}
		if row.Value != nil {

This is the case I'd love a linter to catch but I suspect we'd find it with the assert when the timestamp wasn't set and the caller didn't have a reasonable timestamp.

---

pkg/ccl/changefeedccl/table_history.go, line 286 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Well, there are only 46 callers of GetTable(), perhaps my idea of changing the signature isn't too onerous.

GetTable() is generated. I added Table() that works this way and a linter to avoid calls of GetTable(). The linter was written in a way too heavy-weight way, I'm sure that something like the grep used in TestTimeutil could have worked. I had grander visions of detecting unmarshaling and requiring a call to Table().

---

pkg/internal/client/db.go, line 34 at r1 (raw file):

Previously, ajwerner wrote…

Yeah I noticed that. We only ever seem to use that in the KV fetcher here

cockroach/pkg/sql/row/kv_fetcher.go

Line 57 in 1ad55c5

key, rawBytes, f.batchResponse, err = enginepb.ScanDecodeKeyValueNoTS(f.batchResponse)

and we don't ever fetch these things as sql rows so it should be okay. I'll update the comment to explain when this is zero.
how

Done.

I didn't look at the new linter closely.

Reviewed 1 of 13 files at r1, 2 of 13 files at r2, 23 of 28 files at r3, 12 of 17 files at r5, 2 of 8 files at r7, 1 of 5 files at r8.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @BramGruneir, @lucy-zhang, and @petermattis)

---

pkg/ccl/backupccl/restore.go, line 48 at r6 (raw file):

	"github.com/cockroachdb/cockroach/pkg/util/tracing"
	"github.com/cockroachdb/errors"
	opentracing "github.com/opentracing/opentracing-go"

nit: Is this intentional?

---

pkg/sql/conn_executor_exec.go, line 489 at r8 (raw file):

	// with:
	//
	//  1. Leases at version V-2 for a descriptor being modified to version

I think this sentence got slightly mangled.

---

pkg/sql/conn_executor_exec.go, line 507 at r8 (raw file):

	{
		// If there are existing leases at V-2 for a descriptor being modified at
		// version V being help, the wait loop below that waits on a cluster wide

held

---

pkg/sql/conn_executor_exec.go, line 536 at r8 (raw file):

				filteredLeases = append(filteredLeases, l)
			} else if err := ex.planner.LeaseMgr().Release(l); err != nil {
				log.Warning(ctx, err)

What happens if we fail to release the lease here? Does it get caught in CountLeases and cause a retry? Do we wait somewhere else until it expires?

---

pkg/sql/logictest/testdata/logic_test/schema_change_retry, line 3 at r8 (raw file):

# This test reproduces https://github.com/cockroachdb/cockroach/issues/23979

# Schema changes that experienced retriable errors in RELEASE

nit: This comment should be updated

Just saw some test failures in teamcity related to the foreign key upgrade.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @BramGruneir, @lucy-zhang, and @petermattis)

---

pkg/sql/logictest/testdata/logic_test/schema_change_retry, line 1 at r7 (raw file):

# This test reproduces https://github.com/cockroachdb/cockroach/issues/23979

I just saw the test failure under local-mixed-19.1-19.2. I think this test just shouldn't run with that config (which simulates a cluster running a 19.2 binary where the cluster version is an earlier version, introduced during the FK table desc upgrade), since the behavior is actually different between the cluster versions.

# LogicTest: local fakedist fakedist-metadata fakedist-disk is the config we use for schema_change_in_txn and should be fine for this too. If you think it's important to also run this test in local-mixed-19.1-19.2, the solution we came up with was to have a separate file that only runs in that config (e.g., schema_change_in_txn-local-mixed-19.1-19.2).

---

pkg/sql/sqlbase/structured.go, line 797 at r10 (raw file):

	ctx context.Context, key interface{}, msg protoutil.Message,
) (hlc.Timestamp, error) {
	return hlc.Timestamp{}, m.getProto(ctx, key, msg)

I think the TestUpgradeDowngradeFKRepr panic is caused by this function not returning a valid timestamp and the test itself having table descriptors that don't have the ModificationTime set. The more realistic test would be to somehow have this method return a valid timestamp, but an easier fix would be to set the ModificationTimes.

Just saw some test failures in teamcity related to the foreign key upgrade.

fixed.

This should be close. @andreimatei let me know if you want to take a look.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @BramGruneir, @lucy-zhang, and @petermattis)

---

pkg/sql/conn_executor_exec.go, line 489 at r8 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

I think this sentence got slightly mangled.

Done.

---

pkg/sql/conn_executor_exec.go, line 507 at r8 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

held

Done.

---

pkg/sql/conn_executor_exec.go, line 536 at r8 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

What happens if we fail to release the lease here? Does it get caught in CountLeases and cause a retry? Do we wait somewhere else until it expires?

It's not clear to me why it would fail or what it would mean. This is what the logic under releaseLeases did before so it can't be worse than that. My guess is yes, if it fails we'll fail and then wait for them to expire in the loop below. The error could be an ambiguous result so we may as well proceed to the check below.

---

pkg/sql/logictest/testdata/logic_test/schema_change_retry, line 1 at r7 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

I just saw the test failure under local-mixed-19.1-19.2. I think this test just shouldn't run with that config (which simulates a cluster running a 19.2 binary where the cluster version is an earlier version, introduced during the FK table desc upgrade), since the behavior is actually different between the cluster versions.

# LogicTest: local fakedist fakedist-metadata fakedist-disk is the config we use for schema_change_in_txn and should be fine for this too. If you think it's important to also run this test in local-mixed-19.1-19.2, the solution we came up with was to have a separate file that only runs in that config (e.g., schema_change_in_txn-local-mixed-19.1-19.2).

So I modified this test because it didn't really make sense anymore. I don't have strong feeling about it. I've added the annotation you suggested.

I need to work harder to create a serializable retry. For now to make sure that the tests pass I'm removing this but I can add it back when fixed with the actual retry after. I just want to get this review into a reasonable state before I head out this evening.

---

pkg/sql/logictest/testdata/logic_test/schema_change_retry, line 3 at r8 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

nit: This comment should be updated

Sure, but what should it say.

---

pkg/sql/sqlbase/structured.go, line 3718 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

+1 to the TODO.

Done.

---

pkg/sql/sqlbase/structured.go, line 797 at r10 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

I think the TestUpgradeDowngradeFKRepr panic is caused by this function not returning a valid timestamp and the test itself having table descriptors that don't have the ModificationTime set. The more realistic test would be to somehow have this method return a valid timestamp, but an easier fix would be to set the ModificationTimes.

Done.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @BramGruneir, @lucy-zhang, and @petermattis)

---

pkg/internal/client/db.go, line 338 at r11 (raw file):

func (db *DB) GetProtoTs(
	ctx context.Context, key interface{}, msg protoutil.Message,
) (ts hlc.Timestamp, err error) {

nit: get rid of the names on the return values. They don't add anything. And they invite a nit on the return ts, err line about how it should be return hlc.Timestamp{}, err

---

pkg/sql/conn_executor_exec.go, line 477 at r11 (raw file):

// proceed.
func (ex *connExecutor) checkTableTwoVersionInvariant(ctx context.Context) error {
	tables := ex.extraTxnState.tables.getTablesWithNewVersion()

nit: move this tables below the big comments

---

pkg/sql/conn_executor_exec.go, line 490 at r11 (raw file):

	//
	//  1. Leases at version V-2 for a descriptor being modified to version V.
	//  These leases must be dropped immediately because we're going to assert

nit: we're not going to "assert" anything, we're going to "check" something. Saying assert seems to contradict the next sentence.

---

pkg/sql/conn_executor_exec.go, line 505 at r11 (raw file):

	// cases explicitly. We must release the 1. leases above before we attempt
	// to check the invariant, otherwise it's doomed to fail.
	// the other leases we need to wait until we commit to release.

s/the/The

---

pkg/sql/conn_executor_exec.go, line 512 at r11 (raw file):

		// version V being held, the wait loop below that waits on a cluster wide
		// release of old version leases will hang until these leases expire so
		// we need to drop these leases right now. It is not safe to release leases

can you expand on this "it is not safe to release..."? I assume it has to do with the fact that the current txn might have used them. But how come we're sure it hasn't used the v-2 ones? I believe that's probably true, but I'd like to see the argument.

And actually... Are you sure we can't release the V-1 ones too? What's the hazard? The hazard, I guess, is that this transaction would then be free to commit at a timestamp above the lease (which generally is not allowed because that might be outside of V-1's validity). But, if the transaction commits, it is writing V (or perhaps another transaction will write V, but definitely at a timestamp above the commit ts). So then, I think it is in fact safe to commit the current transaction, because we know that V-1's validity is being extended.

I don't know what I'm talking about, although I'm probably "the expert" on this stuff. But it's been a while and you seem to have done your homework, so perhaps you can school me.