[SERVER-21861] Better Timestamp object comparison in mongo shell Created: 11/Dec/15  Updated: 09/Sep/20  Resolved: 05/Aug/20

Status: Closed
Project: Core Server
Component/s: Shell
Affects Version/s: 3.2.0
Fix Version/s: 4.7.0

Type: Improvement Priority: Major - P3
Reporter: Randolph Tan Assignee: Daniel Gottlieb (Inactive)
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Duplicate
is duplicated by SERVER-31301 Shell incorrectly compares timestamps Closed
is duplicated by SERVER-49422 Prevent Timestamp comparison mistakes... Closed
Problem/Incident
Related
related to SERVER-24628 ReplSetTest.awaitLastOpCommitted can ... Closed
related to SERVER-1672 (new NumberLong(3754)==new NumberLong... Closed
related to SERVER-21941 Automatically handle Timestamp compar... Closed
Backwards Compatibility: Minor Change
Sprint: STM 2020-08-10
Participants:
Linked BF Score: 16
Story Points: 3

 Description   

It would be great if the <, >, <=, >= operators on Timestamp objects would work as expected. The bsonWoCompare can be used for the mean time even though it will end up comparing the member functions as well as long as 't' fields are compared first over the 'i' fields.



 Comments   
Comment by Daniel Gottlieb (Inactive) [ 05/Aug/20 ]

The pushed patch offers some guard rails when using Timestamp objects. Technically, the mongo shell is unchanged. Timestamps can still be compared (i.e: toString -ed) without an error. However, when the shell is invoked from within resmoke.py will inject an error when calling Timestamp.toString.

Most notably, this makes logging timestamps an error. The easiest path forward is to instead log `tojson(timestampVariable)`, similar to what one would do when logging a bson object.

Comment by Githook User [ 05/Aug/20 ]

Author:

{'name': 'Daniel Gottlieb', 'email': 'daniel.gottlieb@mongodb.com', 'username': 'dgottlieb'}

Message: SERVER-21861: Prohibit Timestamp.prototype.toString via resmoke injection.
Branch: master
https://github.com/mongodb/mongo/commit/1e6511738d9e0a189a2d60aabf16411de495e7d1

Comment by Githook User [ 05/Aug/20 ]

Author:

{'name': 'Daniel Gottlieb', 'email': 'daniel.gottlieb@mongodb.com', 'username': 'dgottlieb'}

Message: SERVER-21861: Prohibit Timestamp.prototype.toString via resmoke injection.
Branch: master
https://github.com/10gen/mongo-enterprise-modules/commit/adf835ce81813ae09cc6bc373f4a3265706a64ff

Comment by Daniel Gottlieb (Inactive) [ 14/Jul/20 ]

I'm going to explore the first option. I could be mistaking how change two or three would manifest, but I'm worried that options which output a different timestamp string will be a problematic user change. For example, when spelunking test logs, it's common to copy a timestamp object from a server response that's output by the shell. One would no longer be able to simply paste that string as-is into a search bar for cross-referencing with server logs emitting the timestamp – the server logs won't be zero-padded.

Comment by Robert Guo (Inactive) [ 14/Jul/20 ]

jesse We're going to try the 1st one. Brooke's first sentence was referring to an initial idea that we later rejected.

Comment by A. Jesse Jiryu Davis [ 14/Jul/20 ]

Thanks - that seems a bit contradictory. Which suggestion are we exploring?

Comment by Brooke Miller [ 14/Jul/20 ]

We discussed this ticket in STM's estimation session today and individuals encouraged that we avoid the first two suggestions outlined above because resmoke.py shouldn't cover up Javascript issues in the shell. However, we discussed that the first suggestion is the best option and we should attempt that route to see how it goes and if there are fallouts.

Comment by A. Jesse Jiryu Davis [ 10/Jul/20 ]

To mitigate the pain for now, I got some ideas from max.hirschhorn and matthew.saltz, and I propose either:

  • resmoke.py overrides Timestamp.prototype.toString at startup, to make toString throw an error. Make sure toJSON still works, and make a method toStringIncomparable that we can call in tests for output, without risking silently wrong comparisons
  • resmoke.py overrides Timestamp.prototype.toString at startup to make a correctly comparable representation (by left-padding its "t" and "i" numbers with a large number of zeroes)
  • we always left-pad "t" and "i" in toString so the string is correctly comparable, at the risk of a breaking change for users (but perhaps also benefitting users who want to compare Timestamps)
Comment by Brooke Miller [ 10/Mar/20 ]

Based on our conversation with the STM team during Triaging, this is something we would like to consider down the road when Javascript has better support for operator overloading. 

Comment by Kevin Pulo [ 01/Jul/16 ]

We've already added new types ... like NumberLong

This is the crux of the problem — they're not actually "Types" (even though that's how we think about them), they're just Objects (to Javascript).

Comment by Max Hirschhorn [ 30/Jun/16 ]

NumberLong instances are impacted in the same way as Timestamp instances. The + operator will call the NumberLong.prototype.valueOf() function, which converts the value to a double-precision 64-bit IEEE 754 value because that's the only number type that JavaScript really has.

> NumberLong('9007199254740991') + NumberLong('9007199254740991') + NumberLong('9007199254740991')
27021597764222972
> NumberLong('27021597764222973') - NumberLong('27021597764222972')
0

It seems like a risky and non-trivial amount of work to modify the JS::Value class in SpiderMonkey in order to be able to accurately represent an int64_t type.

Comment by Spencer Brody (Inactive) [ 30/Jun/16 ]

We've already added new types to the mongo shell beyond what exists in pure javascript, like NumberLong. Can we not do something similar with Timestamp?

Comment by Kevin Pulo [ 30/Jun/16 ]

It really isn't possible to do this "properly" (ie. ts1 < ts2) without hacking the language to be Not-Quite-Javascript ("MongoScript"?). See SERVER-1672.

Javascript simply has not been designed with any consideration for the idea that objects might want to act like native types. Even with the above valueOf() hack in place, == and != still won't work as expected (and there is no kludge that can make them work, short of hacking the guts of the JS interpreter/JIT compiler). This would mean that you can have a and b such that a >= b && a <= b, yet a != b, which is arguably worse.

Value objects are likely to be able to address this, but they are currently a long way from being in an ES standard.

The only real option is to have ugly comparison methods and use them everywhere, eg. ts1.eq(ts2), ts1.lte(ts2). But there's still no way of prohibiting a jstest or user from accidentally doing ts1 == ts2 or ts1 <= ts2.

Comment by Spencer Brody (Inactive) [ 16/Jun/16 ]

Is there no way to make the comparison happen in C++ rather than javascript, where it could do the right thing?

I just wasted a full day diagnosing a build failure that turned out to be a direct result of this. I filed SERVER-24628 for fixing this specific instance where we're incorrectly relying on Timestamp comparisons, but it would be great to do something to prevent issues like that from coming up again in the future.

Comment by Kevin Pulo [ 01/Jan/16 ]

A simpler, easier alternative is to make Timestamp's valueOf() return a string representation that has the necessary lexicographic order. Sufficient is to zero-pad the numbers, since they are fixed precision 32 bit ints. And use hex to make them be a bit shorter, but still readable.

> Timestamp(1,900) < Timestamp(1,99)
true
> Timestamp.prototype.valueOf = function() { return "Timestamp(0x" + this.t.toString(16).pad(8, false, "0") + ", 0x" + this.i.toString(16).pad(8, false, "0") + ")"; }
function () { return "Timestamp(0x" + this.t.toString(16).pad(8, false, "0") + ", 0x" + this.i.toString(16).pad(8, false, "0") + ")"; }
> Timestamp(1,900) < Timestamp(1,99)
false

Converting to and comparing strings is a bit slow, but the implicit conversion is doing that already anyway. This way is only slightly slower (the strings are always 33 bytes long, rather than 15 to 33 bytes), but has the significant advantage of being correct in all cases — instead of often right but sometimes silently wrong.

This also has the nice upshot of allowing the standard assert helpers (assert.lt, etc) to continue working without modification, thereby obviating SERVER-21941 (cc kamran.khan).

Comment by Max Hirschhorn [ 11/Dec/15 ]

I don't think it's possible to have <, >, <=, and >= work as expected for all Timestamp instances. Timestamp instances are converted to strings when compared using relational operators. We could change Timestamp.prototype.valueOf() to convert them to a Number instance as follows:

Timestamp.prototype.valueOf = function() {
    var high = this.getTime() >>> 0;
    var low = this.getInc() >>> 0;
    return high * 0x100000000 + low;
}

However, that only gives us 21 bits to use from high before we'd exceed Number.MAX_SAFE_INTEGER, which is insufficient for representing the number of seconds that have elapsed since the Unix epoch.

Using bsonWoCompare() would provide a complete solution because a similar issue also affects large NumberLong instances. We could even define Timestamp.prototype.compare(other), etc. functions for convenience.

Generated at Thu Feb 08 03:58:38 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.