Feedback-ID: ie0a040ee:Fastmail
Message-ID: <2cf9eaf5-057f-4dfa-ae4d-9b23c5339abe@eisentraut.org>
Date: Fri, 22 Mar 2024 15:15:05 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: UUID v7
Content-Language: en-US
To: "Andrey M. Borodin" <x4mmm@yandex-team.ru>
Cc: Aleksander Alekseev <aleksander@timescale.com>,
 pgsql-hackers mailing list <pgsql-hackers@postgresql.org>,
 Sergey Prokhorenko <sergeyprokhorenko@yahoo.com.au>,
 Jelte Fennema-Nio <postgres@jeltef.nl>,
 =?UTF-8?Q?Przemys=C5=82aw_Sztoch?= <przemyslaw@sztoch.pl>,
 "David G. Johnston" <david.g.johnston@gmail.com>,
 Mat Arye <mat@timescaledb.com>,
 Matthias van de Meent <boekewurm+postgres@gmail.com>,
 Nikolay Samokhvalov <samokhvalov@gmail.com>, Junwang Zhao <zhjwpku@gmail.com>
References: <CAAhFRxitJv=yoGnXUgeLB_O+M7J2BJAmb5jqAT9gZ3bij3uLDA@mail.gmail.com>
 <CAGECzQTVi1JUE43uisQ+uKz46bQ+e+uwR9PUsmNfo2rjKaqwoA@mail.gmail.com>
 <8AA22330-7D3A-46CF-B311-2FF874D7C713@yandex-team.ru>
 <1971811009.2132347.1706599708209@mail.yahoo.com>
 <72C1E7CB-E2EC-46CF-AD2C-B3180CED885B@yandex-team.ru>
 <CAEG8a3JUgkY3EMp393kJ+bcye8q=VHfQDTQz7Ci-moNVjhPLZA@mail.gmail.com>
 <0906F8BA-CA52-4956-AA68-E9193E50DFDF@yandex-team.ru>
 <10553e4c-6b66-44b9-86a7-80bb1958a767@eisentraut.org>
 <BFDA918D-1CB0-4980-A31F-C1AC95BE68F6@yandex-team.ru>
 <d59d7cea-fe4a-411d-a63c-2f871cb68a51@eisentraut.org>
 <DB00ED53-ABD2-4E73-A746-90086964115B@yandex-team.ru>
 <ea72ac58-a120-4f51-a1b1-c376b96ec2fd@eisentraut.org>
 <0C26E3AC-983E-43A4-B39E-270FF87A0464@yandex-team.ru>
 <CAJ7c6TNTwHJR3QJ=wSwJJyBvEX39wh11W6eBQTZv-6apWJqVrg@mail.gmail.com>
 <C2C7EAD1-4574-45C2-92F2-96C91D4FCABA@yandex-team.ru>
 <0b1b5e91-177e-4362-8caf-d227d5a1ae3c@eisentraut.org>
 <FFDEE01F-7E3B-4DCC-8830-F722307C2412@yandex-team.ru>
From: Peter Eisentraut <peter@eisentraut.org>
In-Reply-To: <FFDEE01F-7E3B-4DCC-8830-F722307C2412@yandex-team.ru>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/2cf9eaf5-057f-4dfa-ae4d-9b23c5339abe%40eisentraut.org>
Precedence: bulk

On 20.03.24 19:08, Andrey M. Borodin wrote:
>> On 19 Mar 2024, at 13:55, Peter Eisentraut <peter@eisentraut.org> wrote:
>>
>> On 16.03.24 18:43, Andrey M. Borodin wrote:
>>>> On 15 Mar 2024, at 14:47, Aleksander Alekseev <aleksander@timescale.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.