From ad7847baa4b7d4e461ef3452dbd71eb1e263ebe9 Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Fri, 17 Apr 2020 17:57:54 -0400 Subject: [PATCH] sql: use Clock.PhysicalTime in beginTransactionTimestampsAndReadMode 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. --- pkg/sql/conn_executor_exec.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/sql/conn_executor_exec.go b/pkg/sql/conn_executor_exec.go index 0d2334212aeb..fd5ceb712358 100644 --- a/pkg/sql/conn_executor_exec.go +++ b/pkg/sql/conn_executor_exec.go @@ -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