sql: ensure that a table cannot be read before its creation time

[vivekmenezes]

No description provided.

[cockroach-teamcity]

This change is 

[knz]

I think you also need to set ModificationTime on the descriptor created by createViewNode.

---

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[vivekmenezes]

Good point! I'll rework this PR to take care of that, but doesn't bode well for how we're organizing the code!

---

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[knz]

The correct way to do this is to capture the common logic in an API, i.e. move forward on #17188 and solve this problem there.

[vivekmenezes]

@andreimatei this is ready for review. Thanks!

[knz]

Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

pkg/sql/create.go, line 628 at r2 (raw file):

	// Initialize ModificationTime to the creation time.
	desc.ModificationTime = creationTime

Instead, please make all 3 functions makeTableDescIfAs, MakeTableDesc and makeViewTableDesc call your new function initTableDescriptor.
This way you don't need this line here. Also this way we're much more immune to inconsistent descriptors if in the future we implement other statements that create descriptors.

---

pkg/sql/create.go, line 1226 at r2 (raw file):

	var creationTime hlc.Timestamp
	if txn != nil {
		creationTime = txn.OrigTimestamp()

I strongly recommend extending the prototype of MakeTableDesc with the timestamp and let the caller decide the timestamp, instead of using txn != nil. For example for the new bulk I/O functionality there is no transaction but there should be a single timestamp common to all the descriptors being created (pkg/ccl/sqlccl/load.go and parseCSVTableDescriptor in csv.go). Speak to @mjibson.

---

Comments from Reviewable

[vivekmenezes]

Review status: 2 of 7 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

pkg/sql/create.go, line 628 at r2 (raw file):

Previously, knz (kena) wrote…

Instead, please make all 3 functions makeTableDescIfAs, MakeTableDesc and makeViewTableDesc call your new function initTableDescriptor.
This way you don't need this line here. Also this way we're much more immune to inconsistent descriptors if in the future we implement other statements that create descriptors.

Done.

---

pkg/sql/create.go, line 1226 at r2 (raw file):

Previously, knz (kena) wrote…

I strongly recommend extending the prototype of MakeTableDesc with the timestamp and let the caller decide the timestamp, instead of using txn != nil. For example for the new bulk I/O functionality there is no transaction but there should be a single timestamp common to all the descriptors being created (pkg/ccl/sqlccl/load.go and parseCSVTableDescriptor in csv.go). Speak to @mjibson.

Done. I've added a new commit to this PR for further work. I'll ask @mjibson help me figure out what timestamp I aught to pass in here

---

Comments from Reviewable

[knz]

LGTM but let's take some input from a member of the bulk i/o team.

---

Reviewed 5 of 5 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

pkg/ccl/sqlccl/csv.go, line 164 at r3 (raw file):

		parentID,
		tableID,
		hlc.Timestamp{},

Maybe add a TODO comment here and below. Unless Matt has a suggestion straight away.

---

Comments from Reviewable

[vivekmenezes]

@mjibson I'd like your input on what value to pass in as the creation timestamp for the tables being restored. This PR is blocked on that. Thanks!

[maddyblue]

We should pass in the start time of the backup descriptor.

[vivekmenezes]

@mjibson I've merged this PR. Let us resolve #17526 separately.