Skip to content

Commit

Permalink
sql: use Clock.PhysicalTime in beginTransactionTimestampsAndReadMode
Browse files Browse the repository at this point in the history
Synchronizing with the HLC clock doesn't look to be necessary. I'm confused
about this though. The comment on beginTransactionTimestampsAndReadMode says
that "txnSQLTimestamp propagates to become the TxnTimestamp". Is this trying to
say that the timestamp makes it way into the kv.Txn? Because that's not true.

Regardless, the one reason not to make this change is that PhysicalTime is not
guaranteed to be monotonic on some systems and can generally diverge from the
HLC's clock. If we're worried about that though, we should use the HLC here and
feed that directly into the kv.Txn. We shouldn't need to grab two timestamps
from the HLC per txn.
  • Loading branch information
nvanbenschoten committed Apr 17, 2020
1 parent 4d05757 commit ad7847b
Showing 1 changed file with 3 additions and 3 deletions.
6 changes: 3 additions & 3 deletions pkg/sql/conn_executor_exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -916,14 +916,14 @@ func (ex *connExecutor) beginTransactionTimestampsAndReadMode(
historicalTimestamp *hlc.Timestamp,
err error,
) {
now := ex.server.cfg.Clock.Now()
now := ex.server.cfg.Clock.PhysicalTime()
if s.Modes.AsOf.Expr == nil {
rwMode = ex.readWriteModeWithSessionDefault(s.Modes.ReadWriteMode)
return rwMode, now.GoTime(), nil, nil
return rwMode, now, nil, nil
}
ex.statsCollector.reset(&ex.server.sqlStats, ex.appStats, &ex.phaseTimes)
p := &ex.planner
ex.resetPlanner(ctx, p, nil /* txn */, now.GoTime())
ex.resetPlanner(ctx, p, nil /* txn */, now)
ts, err := p.EvalAsOfTimestamp(s.Modes.AsOf)
if err != nil {
return 0, time.Time{}, nil, err
Expand Down

0 comments on commit ad7847b

Please sign in to comment.