Re: UUID v7

On 20.03.24 19:08, Andrey M. Borodin wrote:
>> On 19 Mar 2024, at 13:55, Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>>
>> On 16.03.24 18:43, Andrey M. Borodin wrote:
>>>> On 15 Mar 2024, at 14:47, Aleksander Alekseev <aleksander(at)timescale(dot)com> wrote:
>>>>
>>>> +1 to the idea. I doubt that anyone will miss it.
>>> PFA v22.
>>> Changes:
>>> 1. Squashed all editorialisation by Jelte
>>> 2. Fixed my erroneous comments on using Method 2 (we are using method 1 instead)
>>> 3. Remove all traces of uuid_extract_variant()
>>
>> I have committed a subset of this for now, namely the additions of uuid_extract_timestamp() and uuid_extract_version(). These seemed mature and agreed upon. You can rebase the rest of your patch on top of that.
>
> Great! Thank you! PFA v23 with rebase on HEAD.

I have been studying the uuidv() function.

I find this code extremely hard to follow.

We don't need to copy all that documentation from the RFC 4122bis
document. People can read that themselves. What I would like to see is
easy to find information what from there we are implementing. Like,

- UUID version 7
- fixed-length dedicated counter
- counter is 18 bits
- 4 bits are initialized as zero

That's more or less all I would need to know what is going on.

That said, I don't understand why you say it's an 18 bit counter, when
you overwrite 6 bits with variant and version. Then it's just a 12 bit
counter? Which is the size of the rand_a field, so that kind of makes
sense. But 12 bits is the recommended minimum, and (in this patch) we
don't use sub-millisecond timestamp precision, so we should probably use
more than the minimum?

Also, you are initializing 4 bits (I think?) to zero to guard against
counter rollovers (so it's really just an 8 bit counter?). But nothing
checks against such rollovers, so I don't understand the use of that.

The code code be organized better. In the not-increment_counter case,
you could use two separate pg_strong_random calls: One to initialize
rand_b, starting at &uuid->data[8], and one to initialize the counter.
Then the former could be shared between the two branches, and the code
to assign the sequence_counter to the uuid fields could also be shared.

I would also prefer if the normal case (not-increment_counter) were the
first if branch.

Some other notes on your patch:

- Your rebase duplicated the documentation of uuid_extract_timestamp and
uuid_extract_version.

- PostgreSQL code uses uint64 etc. instead of uint64_t etc.

- It seems the added includes

#include "access/xlog.h"
#include "utils/builtins.h"
#include "utils/datetime.h"

are not needed.

- The static variables sequence_counter and previous_timestamp could be
kept inside the uuidv7() function.