roachtest: backup: use AOST time slightly in the past

[maddyblue]

Avoids problems where it's sometimes in the future according to the
AOST logic.

Fixes #34817

Release note: None

[cockroach-teamcity]

This change is 

[vivekmenezes]

I've seen some other roachtests that are failing with the same error. Do you know which change caused this problem.

[maddyblue]

I think it was #34547 which changed what the max time is.

[vivekmenezes]

I'm seeing a similar failure in #34922 (comment)

I suspect it's being cuased by using Now() in https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/conn_executor_exec.go#L597
in the call just prior to that one we use OrigTimestamp().

@ajwerner any thoughts on this?

[ajwerner]

Yes it does seem that the recent change is causing the problems. The code in #34547 makes AOST evaluation relevant to the stmt time and enforces that it proceeds that time by at least 1us. One easy way to enforce that you get a near real time value that is safe is to use a relative AOST specifier rather than an exact one. I'd recommend using '-1us' instead of rendering a go timestamp to a string.

[vivekmenezes]

@ajwerner I can either use the statement timestamp or a timestamp less by at least-1us right?

[ajwerner]

@ajwerner I can either use the statement timestamp or a timestamp less by at least-1us right?

Correct, if you pass a timestamp which is parsed by ParseDTimestamp that is not after the statement timestamp then it should be fine. It is interesting that you were able to create a time before issuing the query that created a value after the statement timestamp. It may have something to do with the resolution of the timestamp string. If you passed the timestamp in Decimal HLC format it should have sufficient resolution.

[vivekmenezes]

@ajwerner hmmm perhaps it's https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/conn_executor_exec.go#L563 then that uses the txn timestamp . I believe we do this to prevent clock uncertainty from pushing the txn. Perhaps I need to rethink using AOST for this use case. But could that be the problem?

[ajwerner]

@ajwerner hmmm perhaps it's https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/conn_executor_exec.go#L563 then that uses the txn timestamp . I believe we do this to prevent clock uncertainty from pushing the txn. Perhaps I need to rethink using AOST for this use case. But could that be the problem?

That shouldn't be the problem as the txn's timestamp should be before or equal to the statement timestamp. Furthermore, now that I've looked at the serialization, it seems to use the decimal HLC form. I'm honestly confused as to the source of the problem. I'll keep digging.

[vivekmenezes]

#27688 (comment) is another roachtest failure.

[ajwerner]

cockroach/pkg/sql/sem/tree/as_of.go

Line 128 in 79f4d36

} else if stmtTimestamp.Before(ts.GoTime()) {

that's the check

[maddyblue]

The -1us technique won't work for this test because it needs to do an AOST query later to compare the backup results to the query, thus it needs a specific time, not a relative time. Can someone LGTM this or suggest a better way?

[ajwerner]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @danhhz and @dt)

[maddyblue]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)