kv: timetravel queries do not error on ClearRange'd data

[eriktrinh]

Describe the problem

After dropping a table and the underlying data has been deleted by the ClearRange command, queries with a transaction timestamp before the table has been dropped serve empty, invalid results. It should error because by the time ClearRange has been issued, the GC ttl should have already been passed.

This also affects drop index when the index truncation uses ClearRange, where if a transaction uses an older version of a table descriptor with the index then incorrect results are returned if the index is used.

To Reproduce

1. create table t(i int primary key);
2. insert some data into t
3. note timestamp from select now();
4. (optionally) alter table t configure zone with gc.ttlseconds={ttlseconds}
5. drop table t
6. wait GC period until the drop job has completed
7. select * from t as of system time '{timestamp from 3}'

Jira issue: CRDB-4786

[eriktrinh]

cc: @vivekmenezes

[andreimatei]

@vivekmenezes is there a plan here? This seems like a biggie to me; I don't think s-3 does it justice. Particularly since I think this now affects dropped indexes, which is much worse I'd say than dropped tables.

[vivekmenezes]

@andreimatei there is no plan for this. i think there is value here in considering returning an error from sql-land to handle this case.

[tbg]

Looking at #31504 which is a PR that attempted to address the issue, it seems that we got hung up on the semantics of moving the GCThreshold forward. The PR made setting a GCThreshold part of ClearRange, which would solve the issue observed here. However, since the GC threshold is per range, this raised questions about how this might affect unrelated colocated data not targeted by the ClearRange.

Also, has this been fixed? On ./cockroach demo on today's master:

[email protected]:26257/movr> create table t(i int primary key); insert into t values (1), (2), (3); select now();
                  now
---------------------------------------
  2021-03-08 14:11:52.597502+00:00:00
(1 row)

[email protected]:26257/movr> alter table t configure zone using gc.ttlseconds=5;
CONFIGURE ZONE 1

[email protected]:26257/movr> drop table t; select pg_sleep(10);
select * from t as of system time   pg_sleep
------------
    true
(1 row)

[email protected]:26257/movr> select * from t as of system time '2021-03-08 14:11:52.597502+00:00:00';
ERROR: relation "t" does not exist
SQLSTATE: 42P01

It seems as though we're smart enough to prevent use of the foo descriptor once it's deleted:

[email protected]:26257/movr> create table foo (id int primary key); drop table foo; select * from foo as of system time interval '-1h';
ERROR: relation "foo" does not exist
SQLSTATE: 42P01

Maybe @ajwerner can point to a specific change SQL has made here.

[ajwerner]

This is funny. I have a PR open right now to add back the ability to read data from a time before a descriptor was deleted. I “broke” it during this release. It turns out to be a somewhat important property for correctness in some cases, namely if you have a read only transaction which knows (via pg catalog or something like that) that some table exists but then cannot interact with it. It was one of the things breaking the randomized schema change tests.

#61493

This does seem problematic re:clear range. What I wish we’d do is set the GC threshold in the same batch.

[ajwerner]

The real motivation for the above pr isn’t about table data but rather about types and schemas which have their descriptors removed immediately without a ttl. However, the same logic is what enables to code to discover old table descriptor after the ttl has expired and the data has been removed.

[ajwerner]

I've stewed on this a bit and I now don't think that the GC threshold is necessarily the right thing. Unfortunately this space is sort of complex. Ideally we'd find a way to expose a consistent view of the catalog to time travel queries. Right now we just blindly move the read timestamp of the kv txn and things almost perfectly just work out. However, this has its own problems, namely that we use historical privileges (https://groups.google.com/a/cockroachlabs.com/g/sql-schema-team/c/L4oUTiceGY8/m/J030sIylAQAJ, #51861). Ideally what we'd do is know the status of these dropped descriptors.

However, it's even worse than all of this; we support setting a GC TTL for an index. We would need to track the status of that index somewhere and currently that somewhere is not on the descriptor. The point of all of this is that our time travel queries when it comes to schema are somewhat under-considered. Fixing them up will require a reasonably big investment. In the short term we are working slowly on building better abstractions. Namely on pulling the catalog functionality which drives virtual table usage into the catalog layer of the code. At that point we could talk about doing fancier things.

As far as the above PR, there is a more ideal thing we could do that still wouldn't be perfect. My issue is that I really want for tables which have not been GC'd to still be accessible -- but I'm fine with preventing access after the descriptor has been dropped. The problem is that right now, when an AS OF SYSTEM TIME query wants to find the relevant descriptor, the way we do it is by having in memory the current leased descriptor and then walking the chain backwards using the ModificationTime values. This means that if we don't have a descriptor newer than the current descriptor, then we can't find the relevant descriptor. The way that the code used to handle that is any time it cannot find a descriptor, it always just goes to the store to try to read it. In that case, we lose any insight into the present state of the descriptor.

The fix here is to instead take the timestamp we want and do an ExportRequest to get the version of the descriptor that applies as well as its successor. If we did that, then, in principle, we could also go grab the most recent version and detect whether the descriptor has actually been dropped. The only flaw with that approach is that there's now way to paginate the versions of a key so if a user somehow had a ton of versions of a big descriptor, we might be in trouble. FWIW that's the case today, we just would accumulate that memory more slowly so I'm inclined to not immediately count that as a disqualification. Cross referencing with #61623 which I just created as loosely related.

tl;dr

• Unfortunately we live with this for the time being
• Schema will track this as a broader problem to solve related to historical schema access
• There exists an intermediate state which allows us to solve this specific issue which we can pursue in 21.2 to revitalize the reverted change in sql/catalog/lease: revert fallback change and fallback also when doing by-id lookups #61493 and fail with a GC TTL error for tables which have been dropped fully.
• Dropped and clear-ranged indexes pose an interesting challenge that needs some additional work even with the above point.

cc @vy-ton as a problem area

[ajwerner]

The solution here should probably have something to do with #62585.

[ajwerner]

This should go away magically after #70429.

[ajwerner]

This is fixed for 23.1, closing.