Add timestamp support u64::MAX

[yongman]

It is useful to get with the latest commit in transaction, we can create a new readonly transaction (not a snapshot wraps transaction)

let txn = client.new_transaction(Timestamp::from_version(u64::MAX), options.read_only())
let res = txn.get(key).await;

Signed-off-by: yongman [email protected]

[yongman]

@ekexium @iosmanthus PTAL

[ekexium]

I don't think it a good idea to reveal the concept of timestamp to users. What's your requirement of supporting this? Do you need only point-read or scans as well?

[yongman]

@ekexium I want take u64::MAX as timestamp to perform newest snapshot read, and I need the transaction context of the snapshot. Used like this https://github.com/tidbcloud/tikv-service/blob/1fc28af630db850d5eeccf087705bde52e18577e/src/tikv/string.rs#L55

[ekexium]

Using u64::MAX then it's not snapshot read anymore. You may read inconsistent data. We must first check how TiKV handles the sepecial timestamp and then carefully decide how to use it.

[yongman]

@ekexium Yes, it is just used for one key read in use case above.

[yongman]

@ekexium Any suggestions to use u64::MAX timestamp for just single key read case in a transaction, or we can add a new method like begin_latest_readonly() to client?

[ekexium]

I suggest making a new Transaction type that only provides point-get methods to avoid misuse

[yongman]

I think we should not restrict to get multiple keys with the latest committed in a transaction, just like create a new snapshot with timestamp u64::MAX, but want to use the inner readonly transaction directly. I agree that we should not expose the timestamp to user to create a transaction, how about just use client.begin_latest_read() or add an options to TransactionOption to set timestamp automatically, and specific comments should also be added to docs.

[ekexium]

Point-gets, namely get and batch_get are fine. And they are enough, I suppose. You can't treat read requests using timestamp::max() as a simple snapshot with a super large timestamp. Read operations can affect writes and the correctness of transactions. We can only use timestamp::max() where TiKV has handled it specially, e.g. in point_reader.

And it's not a usual case. So I'd not suggest mix it with current Transaction

[yongman]

Read operations can affect writes and the correctness of transactions. We can only use timestamp::max() where TiKV has handled it specially

I checked the logic in client-go, and it provides option to create a transaction with startTs specified which can be a u64::MAX to do the latest commit read. As you pointing out, it may have side affects if user misuse a big u64 (not the u64::MAX) as the startTs.

We can only use timestamp::max() where TiKV has handled it specially, e.g. in point_reader.

Agree. scan command is also fine for point read when use u64::MAX as timestamp, all read apis are implemented Snapshot, with an inner readonly transaction. But in the implementation of transaction I found the readonly option is not checked when doing write operations.

So is it will be safe to create a u64::MAX readonly transaction with write permission check?

[ekexium]

So is it will be safe to create a u64::MAX readonly transaction with write permission check?

Then Snapshot already gives what you need?

But in the implementation of transaction I found the readonly option is not checked when doing write operations.

Cause it's supposed to be only used by Snapshot which doesn't provide write operations.

[yongman]

Then Snapshot already gives what you need?

No, I prefer to using an inner transaction directly which can keep my code simple. :-)

[ekexium]

Ah that's a problem of ease of use. Makes sense.

[yongman]

The pub new_transaction removed and a new begin_latest_read api added. And the write operations will be return OperationReadOnlyError error if called in a readonly transaction.