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