currentTimeUUID creates duplicates when called at the same point in time

[haaawk]

The issue was reported on StackOverflow here

Internally currentTimeUUID calls get_time_UUID which creates UUID using current time and a clock_seq_and_node constant generated once per shard.

    static UUID get_time_UUID()
    {
        auto uuid = UUID(instance->create_time_safe(), clock_seq_and_node);
        assert(uuid.is_timestamp());
        return uuid;
    }

const thread_local int64_t UUID_gen::clock_seq_and_node = make_clock_seq_and_node();

[haaawk]

This issue will affect CDC as well because CDC generates TimeUUID for clustering keys in CDC Log

[haaawk]

Fix is here -> #6209

[tgrabiec]

It's not clear to me what the supposed bug is about. Please clarify in the description.

[haaawk]

https://stackoverflow.com/questions/61223391/how-to-guarantee-monotonically-increasing-timeuuid-when-selecting-from-scylla/61243270

The user used currentTimeUUID and got 20-40 duplicates per partition.

[tgrabiec]

https://stackoverflow.com/questions/61223391/how-to-guarantee-monotonically-increasing-timeuuid-when-selecting-from-scylla/61243270

The user used currentTimeUUID and got 20-40 duplicates per partition.

I am confused as to what he means by that.

He says:

I tried using currentTimeUUID function and it seems to work(monotonically increasing within the same partition key)

which suggests that currentTimeUUID() doesn't generate duplicates.

Also, createdAt is a clustering key so queries cannot return duplicates in that column.

[nyh]

I think you are mis-understanding the problem (or I am misunderstanding you...). The function which you quoted,

static UUID get_time_UUID()
    {
        auto uuid = UUID(instance->create_time_safe(), clock_seq_and_node);
        assert(uuid.is_timestamp());
        return uuid;
    }

doesn't need clock_seq_and_node to change to avoid returning the same id twice... It uses create_time_safe() which ensures that the same uuid is not generated even if called in the same millisecond on the same shard (unfortunately we use millisecond resolution, which sucks for other reasons, but wouldn't break uniqueness). This is the reason for the "safe()" in its name.

So I don't think that the C++ function get_time_UUID() actually has the problem that you think it has.

Another possibility is that our code doesn't call it but call one of its variants which takes a time argument? Those variants indeed appear broken because they don't check for ties. Those broken variants should be outright deleted - do you know why they even exist?

I propose that before you do anything on this issue, that you try to write a test which reproduces this issue (unit test of C++ functions, or dtest or pytest to see the end-to-end problem in CQL), and then you can confirm you understand what the real problem is.

[nyh]

@kostja observed, if I understand correctly, that the error causing the problem reported above isn't the details mentioned above. Rather, it is make_node() in utils/UUID_gen.cc.
This function should have been different in different nodes - e.g., a MAC address - but we take the same number on all nodes (just the shard number), and the result is that two nodes that calculate a timestamp around the same time produce the same UUID.
The code even has a FIXME that this needs to be fixed :-(

[kostja]

Apart from filling host id, the patch should make sure timeuuid compare respects host id.

[nyh]

Apart from filling host id, the patch should make sure timeuuid compare respects host id.

Do you suspect that compare_visitor (the timeuuid_type_impl& case) in types.cc` ignores some parts of the timeuuid? Or by "make sure" you just mean we need to check and verify that this is the case?

[kostja]

The logic of the compare function confused me. Apart from being very inefficient, it falls back to signed compare of the entire UUID if timeuuid_compare_bytes() returns 0. I didn't notice it at first.

[avikivity]

Fix present on all active branches, not backporting.