New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: assigned timestamp at SQL should correspond to some KV timestamp #4393

Closed
tschottdorf opened this Issue Feb 16, 2016 · 5 comments

Comments

Projects
None yet
4 participants
@tschottdorf
Member

tschottdorf commented Feb 16, 2016

via @bdarnell:

When a BEGIN statement is executed, we set the transaction timestamp to time.Now() on the gateway node, so the SQL transaction_timestamp() bears no relation to the kv-level transaction timestamp.

This makes it somewhat hard to reason about timestamps in SQL transactions and, as a result, about observed consistency-related phenomena.

It seems adequate to chose the original timestamp of the transaction instead of time.Now(): It remains constant during the lifetime of a transaction, is equal to the commit timestamp for a serializable transaction, and (is equal to or) precedes any KV timestamps for writes during a SNAPSHOT transaction (which could be surprising, but the current behavior already has that phenomenon).

Implementation would be straightforward: Upon creation of a transaction, set its timestamp. Possibly a minor fix to TxnCoordSender is necessary so that it respects a preset timestamp in a new transaction proto.

@andreimatei

This comment has been minimized.

Show comment
Hide comment
@andreimatei

andreimatei Feb 16, 2016

Member

btw there's a comment in the Session proto that says these two timestamps are different. It will have to be changed.
https://github.com/cockroachdb/cockroach/blob/master/sql/session.proto#L32

Member

andreimatei commented Feb 16, 2016

btw there's a comment in the Session proto that says these two timestamps are different. It will have to be changed.
https://github.com/cockroachdb/cockroach/blob/master/sql/session.proto#L32

@petermattis petermattis changed the title from Assigned timestamp at SQL should correspond to some KV timestamp to sql: assigned timestamp at SQL should correspond to some KV timestamp Feb 25, 2016

@petermattis petermattis added this to the Beta milestone Feb 25, 2016

@tschottdorf

This comment has been minimized.

Show comment
Hide comment
@tschottdorf

tschottdorf Feb 26, 2016

Member

Any more comments here? I think we should make that change sooner or later. I'm not too familiar with the behavior of the various timestamp-returning functions, though.

Member

tschottdorf commented Feb 26, 2016

Any more comments here? I think we should make that change sooner or later. I'm not too familiar with the behavior of the various timestamp-returning functions, though.

@petermattis

This comment has been minimized.

Show comment
Hide comment
@petermattis

petermattis Feb 26, 2016

Contributor

I think the only reason we didn't use roachpb.Transaction.Timestamp for the SQL timestamp functions is that it isn't set when the transaction is created. If we can change that and allow the KV client to set the timestamp then the change on the SQL side should be straightforward.

Contributor

petermattis commented Feb 26, 2016

I think the only reason we didn't use roachpb.Transaction.Timestamp for the SQL timestamp functions is that it isn't set when the transaction is created. If we can change that and allow the KV client to set the timestamp then the change on the SQL side should be straightforward.

@tschottdorf

This comment has been minimized.

Show comment
Hide comment
@tschottdorf

tschottdorf Feb 27, 2016

Member

As discussed with @andreimatei: We should also update this timestamp on retries. Instead of using the one returned from the retry error, we want a current one from the node clock at the time of the client's restart (so not assigned when handling the error but with the subsequent next RPC from the client).

Member

tschottdorf commented Feb 27, 2016

As discussed with @andreimatei: We should also update this timestamp on retries. Instead of using the one returned from the retry error, we want a current one from the node clock at the time of the client's restart (so not assigned when handling the error but with the subsequent next RPC from the client).

@tschottdorf

This comment has been minimized.

Show comment
Hide comment
@tschottdorf

tschottdorf Feb 27, 2016

Member

We could (likely should) even put that into TxnCoordSender itself. When a txn proto comes in with an increased epoch, we update the timestamp to our local clock.

Member

tschottdorf commented Feb 27, 2016

We could (likely should) even put that into TxnCoordSender itself. When a txn proto comes in with an increased epoch, we update the timestamp to our local clock.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment