sql: cleanup AS OF SYSTEM TIME query descriptor cache lookup

[vivekmenezes]

This change removes the fixedTimestamp bit introduced in #31716

It also explains problem #18354 with LeaseManager.Acquire() through
a comment and implements a work around bypassing the cache and
reading the descriptor directly from the store.

It also fixes a problem with UncachedPhysicalAccessor.GetObjectDesc
tested through TestTxnCanStillResolveOldName.

related to #30967

fixes #18354
fixes #19925

Release note: None

[cockroach-teamcity]

This change is 

[andreimatei]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/physical_schema_accessors.go, line 139 at r1 (raw file):

		// We have a descriptor. Is it in the right state?
		if err := filterTableState(desc); err != nil {
			// No: let's see the flag.

what "flag" is this comment talking about?

---

pkg/sql/physical_schema_accessors.go, line 141 at r1 (raw file):

			// No: let's see the flag.
			if err == errTableAdding {
				// We'll keep that despite the ADD state.

what's "that" in this sentence? Also, this whole comment seems to be simply stating the line below (in a particularly cryptic way). I think it'd benefit from more words.

---

pkg/sql/table.go, line 196 at r1 (raw file):

// return a nil descriptor and no error if the table does not exist.
//
// It might also add a transaction deadline to the transaction that is

if the only reason why flags.txn exists is for its timestamp and for setting a deadline on it, I think it'd be much better if this method took the timestamp explicitly and returned the deadline.

---

pkg/sql/table.go, line 276 at r1 (raw file):

	table, expiration, err := tc.leaseMgr.AcquireByName(ctx, origTimestamp, dbID, tn.Table())
	if err != nil {
		// AcquireByName() is unable to function correctly on a timestamp

nit: I think some/all of the limitations of AcquireByName() should be documented on the method (and then can be hinted to here).

---

pkg/sql/table.go, line 281 at r1 (raw file):

		// While AcquireByName can use the timestamp and get the correct name->id
		// mapping at the timestamp, it uses Acquire() to get a descriptor with
		// the corresponding id. Acquire() in finding a descriptor for the timestamp

I don't quite follow - why would Acquire() fail exactly? I don't understand how this use of Acquire() by AcquireByName() is special.

---

pkg/sql/table.go, line 282 at r1 (raw file):

		// mapping at the timestamp, it uses Acquire() to get a descriptor with
		// the corresponding id. Acquire() in finding a descriptor for the timestamp
		// needs to acquire a lease for the descriptor and fails. See #18354.

I don't understand the reference to #18354; the current PR claims to fix it. What's this reference telling me exactly?

[andreimatei]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/table.go, line 281 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I don't quite follow - why would Acquire() fail exactly? I don't understand how this use of Acquire() by AcquireByName() is special.

Oh is this all about drop being incompatible with MVCC cause it uses ClearRange?

[vivekmenezes]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/physical_schema_accessors.go, line 139 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

what "flag" is this comment talking about?

Looks like this comment was removed through #31794

---

pkg/sql/physical_schema_accessors.go, line 141 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

what's "that" in this sentence? Also, this whole comment seems to be simply stating the line below (in a particularly cryptic way). I think it'd benefit from more words.

Done.

---

pkg/sql/table.go, line 196 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

if the only reason why flags.txn exists is for its timestamp and for setting a deadline on it, I think it'd be much better if this method took the timestamp explicitly and returned the deadline.

looks like we're passing flags.required. I think we can clean this up. Let me add another commit to this PR with that.

---

pkg/sql/table.go, line 281 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Oh is this all about drop being incompatible with MVCC cause it uses ClearRange?

I hope you are able to understand this based on the new comments in this change.

---

pkg/sql/table.go, line 282 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I don't understand the reference to #18354; the current PR claims to fix it. What's this reference telling me exactly?

I've taken out the reference to that issue because this PR fixes it.

[andreimatei]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/lease.go, line 1521 at r2 (raw file):

//
// Known limitation: Acquire() can return an error after the table with
// the tableID has been dropped. This is even when using a timestamp

nit: this is true even...

---

pkg/sql/table.go, line 281 at r1 (raw file):

Previously, vivekmenezes wrote…

I hope you are able to understand this based on the new comments in this change.

You must have seen this coming :P - can we not fix LeaseMgr.Acquire()? It seems to me that this here is the wrong layer at which to paper over various errors; it seems like these errors should not be created in the first place ideally. Or do you disagree?

Relatedly, I don't remember the details, but I think @eriktrinh was working on making a drop wipe a table's history (as soon as it completes?). If that's still happening, I think maybe we don't need to handle anything about dropped tables as anything we do has an expiration time?

Notwithstanding that, if it helps in any way with improving LeaseMgr.Acquire(), I can volunteer to provide a db.Get() version that returns the full history for a key.

[eriktrinh]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/table.go, line 281 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

You must have seen this coming :P - can we not fix LeaseMgr.Acquire()? It seems to me that this here is the wrong layer at which to paper over various errors; it seems like these errors should not be created in the first place ideally. Or do you disagree?

Relatedly, I don't remember the details, but I think @eriktrinh was working on making a drop wipe a table's history (as soon as it completes?). If that's still happening, I think maybe we don't need to handle anything about dropped tables as anything we do has an expiration time?

Notwithstanding that, if it helps in any way with improving LeaseMgr.Acquire(), I can volunteer to provide a db.Get() version that returns the full history for a key.

Even if there is an expiration time for a dropped table the descriptor should still be able to be resolved with the original transaction timestamp. The gc window from when the table is dropped to when the table data is wiped is also rather large.

[vivekmenezes]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/table.go, line 281 at r1 (raw file):

Previously, eriktrinh (Erik Trinh) wrote…

Even if there is an expiration time for a dropped table the descriptor should still be able to be resolved with the original transaction timestamp. The gc window from when the table is dropped to when the table data is wiped is also rather large.

I don't think we can fix LeaseMgr.Acquire() without at least some amount of thought on what needs to be done here. I'd like to not invest more of my time because I'd like to completely ditch this cache implementation for 2.2 and rethink it. Given that the limitation is only for a not so important case spending more time here is not what I'd like to do. I'm happy to create an issue detailing this limitation and adding it to the list of problems the next implementation needs to fix.

Part of the problem is leases continue to be tied to the cache. In the future implementation I'd like to tie leases only to the expiration time of a schema.

[andreimatei]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/table.go, line 281 at r1 (raw file):

Previously, vivekmenezes wrote…

I don't think we can fix LeaseMgr.Acquire() without at least some amount of thought on what needs to be done here. I'd like to not invest more of my time because I'd like to completely ditch this cache implementation for 2.2 and rethink it. Given that the limitation is only for a not so important case spending more time here is not what I'd like to do. I'm happy to create an issue detailing this limitation and adding it to the list of problems the next implementation needs to fix.

Part of the problem is leases continue to be tied to the cache. In the future implementation I'd like to tie leases only to the expiration time of a schema.

What do you think of simply not handling these errors and documenting that you can't time travel across a drop/truncate? (and then ensuring that you can't time travel, which might mean a little more than deleting this error handling - maybe drop some entries from the cache would be needed, I dunno).
Besides getting rid of this unfortunate code, I think generally we should rethink what dropping a table does and ensure that space gets reclaimed relatively quickly. Right now, space gets reclaimed 25h later, and the user doesn't even theoretically have a way to empty the trash can earlier - you can't change the TTL on a dropped table. I think ideally there'd be a way to undo a drop for a (short) while, but not by time traveling.

[vivekmenezes]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/table.go, line 281 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

What do you think of simply not handling these errors and documenting that you can't time travel across a drop/truncate? (and then ensuring that you can't time travel, which might mean a little more than deleting this error handling - maybe drop some entries from the cache would be needed, I dunno).
Besides getting rid of this unfortunate code, I think generally we should rethink what dropping a table does and ensure that space gets reclaimed relatively quickly. Right now, space gets reclaimed 25h later, and the user doesn't even theoretically have a way to empty the trash can earlier - you can't change the TTL on a dropped table. I think ideally there'd be a way to undo a drop for a (short) while, but not by time traveling.

I think not supporting AS OF SYSTEM TIME on a truncated table would be bad for users. But besides that we also have a big can of worms being tested through TestRestoreAsOfSystemTime that relies on us not returning such an error. I'd definitely need to rewrite that test.

You raise a good point about prioritizing #26476 which we should definitely fix for 2.2

How about I create an issue for this limitation so we get to work on it later.

[andreimatei]

LGTM

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/table.go, line 196 at r1 (raw file):

Previously, vivekmenezes wrote…

looks like we're passing flags.required. I think we can clean this up. Let me add another commit to this PR with that.

My beef is specifically with the existence of flags.txn; I didn't understand your remark. Anyway, is there still another commit to come?

---

pkg/sql/table.go, line 281 at r1 (raw file):

Previously, vivekmenezes wrote…

I think not supporting AS OF SYSTEM TIME on a truncated table would be bad for users. But besides that we also have a big can of worms being tested through TestRestoreAsOfSystemTime that relies on us not returning such an error. I'd definitely need to rewrite that test.

You raise a good point about prioritizing #26476 which we should definitely fix for 2.2

How about I create an issue for this limitation so we get to work on it later.

#26476 is about being able to change the TTL of a dropped table. My personal preference is for more bias towards deleting the data sooner than a regular TTL. Having to issue something like ALTER TABLE foo EXPERIMENTAL CONFIGURE ZONE '{gc: {ttlseconds: 30}}' doesn't seem appropriate to me for getting rid of dropped table data. So, coming from here, I'd call citation needed on "not supporting AS OF SYSTEM TIME on a truncated table would be bad for users".
Perhaps @bdarnell would like to chime in here too.

Anyway, as far as this PR goes, this code LGTM - although I'm curious if we could at least handle a single error rather than two. But I don't really personally want to understand if that'd be possible or not, so if you say it isn't I trust you :).

[vivekmenezes]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/table.go, line 196 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

My beef is specifically with the existence of flags.txn; I didn't understand your remark. Anyway, is there still another commit to come?

I looked into this but it does look like returning a deadline is the wrong thing to do because I'd have to change the entire resolver API to return a deadline and I see no point to it because all callers would then add the deadline to their txn. There are two solutions that feel better:

1. Add a txn to the TableCollection that would cement the idea that a TableCollection is indeed associated with a txn, which it is.
2. or, keep track of deadlines within a TableCollection and only set the deadline at commit time, but then I don't know how to handle the autocommit case

I think step 1 is an improvement and I know I can do that, but I do like 2 if you have any ideas how to trigger a lookup into SQL in the autocommit case which is called just before commit.

---

pkg/sql/table.go, line 281 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

#26476 is about being able to change the TTL of a dropped table. My personal preference is for more bias towards deleting the data sooner than a regular TTL. Having to issue something like ALTER TABLE foo EXPERIMENTAL CONFIGURE ZONE '{gc: {ttlseconds: 30}}' doesn't seem appropriate to me for getting rid of dropped table data. So, coming from here, I'd call citation needed on "not supporting AS OF SYSTEM TIME on a truncated table would be bad for users".
Perhaps @bdarnell would like to chime in here too.

Anyway, as far as this PR goes, this code LGTM - although I'm curious if we could at least handle a single error rather than two. But I don't really personally want to understand if that'd be possible or not, so if you say it isn't I trust you :).

I think from a product perspective, I'd like such dropped tables to move to a "trash" database with a drop done there after the GC interval. That way a user can issue a drop to delete that garbage if they really cared about deleting it faster by issuing ALTER TABLE DROP trash.67 (where 67 is the table id).

[andreimatei]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/table.go, line 196 at r1 (raw file):

Previously, vivekmenezes wrote…

I looked into this but it does look like returning a deadline is the wrong thing to do because I'd have to change the entire resolver API to return a deadline and I see no point to it because all callers would then add the deadline to their txn. There are two solutions that feel better:

1. Add a txn to the TableCollection that would cement the idea that a TableCollection is indeed associated with a txn, which it is.
2. or, keep track of deadlines within a TableCollection and only set the deadline at commit time, but then I don't know how to handle the autocommit case

I think step 1 is an improvement and I know I can do that, but I do like 2 if you have any ideas how to trigger a lookup into SQL in the autocommit case which is called just before commit.

I like something like 2) - as long as there is a deadline, I think that setting the deadline late is the right direction - as ideally that the computation would not just consult a stored min value, but instead it would look at the current expiration of leases. But I wouldn't phrase it as the txn needing to call back into SQL. Instead, I'd phrase it as SQL needing to ensure that it attaches the right deadline to the EndTransaction, in all cases. In the autocommit case, this would translate into the tableWriterBase.finalize() not using the vanilla txn.CommitInBatch() as it does now, but instead it would set the deadline on the txn first - or actually at this point the deadline would not be an attribute of the txn any more, but instead it would be an argument to CommitInBatch().
For safety, we could also have some check in the client package that ensure that a deadline is also set on sql txns, somehow - to make sure we haven't missed any code paths.

But if we significantly reshape how deadlines look in the future (particularly if there might not be deadlines, but epochs), then this might not be worth doing.

---

pkg/sql/table.go, line 281 at r1 (raw file):

Previously, vivekmenezes wrote…

I think from a product perspective, I'd like such dropped tables to move to a "trash" database with a drop done there after the GC interval. That way a user can issue a drop to delete that garbage if they really cared about deleting it faster by issuing ALTER TABLE DROP trash.67 (where 67 is the table id).

I agree. But I see all this proposal as even more reason to disallow AOST queries across a drop - users have other ways of accessing that data, and also whether they work or not becomes more unpredictable (as the answer depends not only on some TTL, but also on user actions on the magic database trash).

[bdarnell]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/table.go, line 281 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I agree. But I see all this proposal as even more reason to disallow AOST queries across a drop - users have other ways of accessing that data, and also whether they work or not becomes more unpredictable (as the answer depends not only on some TTL, but also on user actions on the magic database trash).

It's important that DROP TABLE be undoable. Today (as I understand it), that's possible by using AOST to query before the table was dropped, and we don't want to break that. But if we had an UNDROP command (or ALTER TABLE trash.67 RENAME TO whatever_it_used_to_be), that would be even better, and I think if we had that I'd be OK with losing the ability to do AOST across a drop.

[vivekmenezes]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/table.go, line 281 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

It's important that DROP TABLE be undoable. Today (as I understand it), that's possible by using AOST to query before the table was dropped, and we don't want to break that. But if we had an UNDROP command (or ALTER TABLE trash.67 RENAME TO whatever_it_used_to_be), that would be even better, and I think if we had that I'd be OK with losing the ability to do AOST across a drop.

updated #26476 to reflect these thought.

[vivekmenezes]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/table.go, line 196 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I like something like 2) - as long as there is a deadline, I think that setting the deadline late is the right direction - as ideally that the computation would not just consult a stored min value, but instead it would look at the current expiration of leases. But I wouldn't phrase it as the txn needing to call back into SQL. Instead, I'd phrase it as SQL needing to ensure that it attaches the right deadline to the EndTransaction, in all cases. In the autocommit case, this would translate into the tableWriterBase.finalize() not using the vanilla txn.CommitInBatch() as it does now, but instead it would set the deadline on the txn first - or actually at this point the deadline would not be an attribute of the txn any more, but instead it would be an argument to CommitInBatch().
For safety, we could also have some check in the client package that ensure that a deadline is also set on sql txns, somehow - to make sure we haven't missed any code paths.

But if we significantly reshape how deadlines look in the future (particularly if there might not be deadlines, but epochs), then this might not be worth doing.

#18684 (comment)

It's best done in a separate PR. I'm merging this one. Thanks for your feedback.

[vivekmenezes]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)