sql,sqlbase,client: bootstrap TableDescriptor timestamps from MVCC

[ajwerner]

This PR adds back the changes in #40581 and deals with the fallout that led to #40781 in the 3rd commit.

Release Justification: Fixes a bug formerly considered to be a release blocker.

[cockroach-teamcity]

This change is 

[ajwerner]

There was some churn here because of a discovery: before 19.1 we did not set the ModificationTime on Version 1 of table descriptors and the CreateAsOfTime did not exist.

The logic in TableDescriptor.maybeSetTimeFromMVCCTimestamp() has been updated to reflect my now better understanding of CreateAsOfTime and the logic in restore has been made robust to missing values with a perhaps crude but effective fix in the last commit that now includes testing.

I'm hopeful we can agree to still put in this change as it's a real customer pain point and I understand that this revealed some added risk.

[thoszhang]

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

---

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

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

This got added again.

---

pkg/ccl/backupccl/restore.go, line 78 at r3 (raw file):

			return nil, errors.Wrapf(err, "failed to read backup descriptor")
		}
		for _, d := range desc.Descriptors {

I think all this should actually go in readBackupDescriptor(), which is ultimately what gets called whenever we read a backup descriptor (which happens in a few other places besides this one).

---

pkg/ccl/backupccl/restore.go, line 91 at r3 (raw file):

			if t := d.GetTable(); t == nil {
				continue
			} else if t.Version == 1 && t.ModificationTime.IsEmpty() {

I had thought that we would never make a >=19.1 backup of a table descriptor without a ModificationTime. Even in 19.2, a table descriptor must be read before it is written to the backup descriptor, at which point a timestamp would have been set if it hadn't been set already, right?

(Also, is it possible to just indiscriminately set the ModificationTime to 1 whenever we restored a table? Is there any advantage to keeping the old timestamp?)

---

pkg/sql/sqlbase/structured.go, line 3305 at r3 (raw file):

func (desc *TableDescriptor) maybeSetTimeFromMVCCTimestamp(ts hlc.Timestamp) {
	// CreateAsOfTime is used for CREATE TABLE ... AS SELECT ... and was
	// introduced in v19.1. In general it is not critical to set and we definitely

Sorry I forgot about the CREATE TABLE AS complications in my earlier review. I think this comment could be clearer about CreateAsOfTime being set if and only if the table was created from CREATE TABLE AS. (Though in practice I think it's harmless if the timestamp isn't set as long as the table isn't in the ADD state anymore. We only use it as a transaction timestamp when filling in the table.)

Along those same lines, maybe there should be an assertion that if table.Adding() && table.IsAs(), we either have the timestamp already set or have a value to set it to.

---

pkg/sql/sqlbase/structured.proto, line 826 at r3 (raw file):

  // version of a table and is populated from the MVCC timestamp of the read
  // like ModificationTime. See Descriptor.Table().
  // CreateAsOfSystemTime is used for CREATE TABLE ... AS SELECT ... and was

nit: CREATE TABLE ... AS ...

[ajwerner]

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

---

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

Previously, lucy-zhang (Lucy Zhang) wrote…

This got added again.

Done.

---

pkg/ccl/backupccl/restore.go, line 78 at r3 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

I think all this should actually go in readBackupDescriptor(), which is ultimately what gets called whenever we read a backup descriptor (which happens in a few other places besides this one).

Done.

---

pkg/ccl/backupccl/restore.go, line 91 at r3 (raw file):

I had thought that we would never make a >=19.1 backup of a table descriptor without a ModificationTime.

True but we need to support restoring from backups made from any version.

(Also, is it possible to just indiscriminately set the ModificationTime to 1 whenever we restored a table? Is there any advantage to keeping the old timestamp?)

Seems like it should be. The main value of the old timestamp as far as I can tell is that it will correspond to the MVCC time range of keys which contain that physical format. It's totally possible that I misunderstand how restoring a table interacts with MVCC but my expectation is that the restored keys will carry the same timestamp as when they were exported. If that's false then I feel way less strongly about preserving the ModificationTime.

---

pkg/sql/sqlbase/structured.go, line 3305 at r3 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

Sorry I forgot about the CREATE TABLE AS complications in my earlier review. I think this comment could be clearer about CreateAsOfTime being set if and only if the table was created from CREATE TABLE AS. (Though in practice I think it's harmless if the timestamp isn't set as long as the table isn't in the ADD state anymore. We only use it as a transaction timestamp when filling in the table.)

Along those same lines, maybe there should be an assertion that if table.Adding() && table.IsAs(), we either have the timestamp already set or have a value to set it to.

Done.

---

pkg/sql/sqlbase/structured.proto, line 826 at r3 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

nit: CREATE TABLE ... AS ...

Done.

[andreimatei]

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

---

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

		backupDescs[i] = desc
	}

nit: spurious diff

---

pkg/ccl/backupccl/restore_old_versions_test.go, line 38 at r6 (raw file):

	for _, dir := range dirs {
		require.True(t, dir.IsDir())
		t.Run(dir.Name(), restoreOldVersionTest(dir.Name()))

nit: pass path.join(exportDirs, dir) and localize exportDirs and testdataBase to this function.

---

pkg/ccl/backupccl/testdata/restore_old_versions/create.sql, line 1 at r6 (raw file):

-- The below SQL is used to create the data that is then exported with BACKUP

I'm really happy that you wrote these tests, but can you please also paste in here somewhere the exact commands to produce the ssts?

---

pkg/sql/sqlbase/structured.go, line 3318 at r6 (raw file):

		desc.CreateAsOfTime = ts
	}

spurious diff

[ajwerner]

TFTR!

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

---

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

Previously, andreimatei (Andrei Matei) wrote…

nit: spurious diff

Done.

---

pkg/ccl/backupccl/restore_old_versions_test.go, line 38 at r6 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

nit: pass path.join(exportDirs, dir) and localize exportDirs and testdataBase to this function.

Done.

---

pkg/ccl/backupccl/testdata/restore_old_versions/create.sql, line 1 at r6 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I'm really happy that you wrote these tests, but can you please also paste in here somewhere the exact commands to produce the ssts?

Added it to the test comment.

---

pkg/sql/sqlbase/structured.go, line 3318 at r6 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

spurious diff

I think I like this one.

[ajwerner]

@lucy-zhang can I have your stamp on this before I bors it?

[thoszhang]

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

---

pkg/ccl/backupccl/restore.go, line 91 at r3 (raw file):

True but we need to support restoring from backups made from any version.

Yeah, agreed, I actually just misread the comment/code previously.

---

pkg/sql/sqlbase/structured.go, line 3323 at r7 (raw file):

	if desc.Adding() && desc.IsAs() && desc.CreateAsOfTime.IsEmpty() {
		log.Fatalf(context.TODO(), "table descriptor for %q (%d.%d) is in the "+
			"ADD state and was creates with CREATE TABLE ... AS but does not have a "+

created

[ajwerner]

TFTR!

bors r+

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

---

pkg/sql/sqlbase/structured.go, line 3323 at r7 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

created

Done.

[craig]

Build succeeded

• GitHub CI (Cockroach)