Re: UUID v7 - Mailing list pgsql-hackers

On Sun, Aug 4, 2024 at 3:51 AM Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
>
>
>
> > On 28 Jul 2024, at 23:44, Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
> >
> > PFA version accepting offset interval.
>
> There was a bug: when time was not moving on, I was updating used time by a nanosecond, instead of 1/4096 of
millisecond.
> V27 fixes that.
>
> Thanks!

I've reviewed the v27 patch and have some comments:

---
in datatype.sgml:

    The data type <type>uuid</type> stores Universally Unique Identifiers
    (UUID) as defined by <ulink
url="https://datatracker.ietf.org/doc/html/rfc4122">RFC 4122</ulink>,
    ISO/IEC 9834-8:2005, and related standards.

In funcs.sgml:
   This function extracts the version from a UUID of the variant described by
   <ulink url="https://datatracker.ietf.org/doc/html/rfc4122">RFC
4122</ulink>.  For

Maybe these references of RFC4122 need to be updated as well.

---
'git show --check' raises a warning:

src/backend/utils/adt/uuid.c:520: trailing whitespace.
+

---
+
+   if (PG_NARGS() > 0)
+   {
+       Interval *span;
+       TimestampTz ts = (TimestampTz) (ns / 1000) -
+           (POSTGRES_EPOCH_JDATE - UNIX_EPOCH_JDATE) * SECS_PER_DAY *
USECS_PER_SEC;
+       span = PG_GETARG_INTERVAL_P(0);
+       ts = DatumGetTimestampTz(DirectFunctionCall2(timestamptz_pl_interval,
+                                                    TimestampTzGetDatum(ts),
+                                                    IntervalPGetDatum(span)));
+       ns = (ts + (POSTGRES_EPOCH_JDATE - UNIX_EPOCH_JDATE) *
SECS_PER_DAY * USECS_PER_SEC)
+           * 1000 + ns % 1000;
+   }

We need to add a comment to describe what/why we're doing here.

---
+ * Monotonicity (regarding generation on given backend) is ensured with method
+ * "Replace Leftmost Random Bits with Increased Clock Precision (Method 3)"

Need a period at the end of this sentence.

---
+{ oid => '9896', descr => 'generate UUID version 7',
+  proname => 'uuidv7', proleakproof => 't', provolatile => 'v',
+  prorettype => 'uuid', proargtypes => '', prosrc => 'uuidv7' },
+{ oid => '9897', descr => 'generate UUID version 7',
+  proname => 'uuidv7', proleakproof => 't', provolatile => 'v',
+  prorettype => 'uuid', proargtypes => 'interval', prosrc => 'uuidv7' },

Both functions have the same description but work differently. I think
it's better to clarify the description of uuidv7() that takes an
interval.

---
- oid | proname | oid | proname
------+---------+-----+---------
-(0 rows)
+ oid  | proname | oid  | proname
+------+---------+------+---------
+ 9896 | uuidv7  | 9897 | uuidv7
+(1 row)

I think that we need to change these functions so that this check
query doesn't return anything, no?

---
+   if (version == 6)
+   {
+       tms = ((uint64) uuid->data[0]) << 52;
+       tms += ((uint64) uuid->data[1]) << 44;
+       tms += ((uint64) uuid->data[2]) << 36;
+       tms += ((uint64) uuid->data[3]) << 28;
+       tms += ((uint64) uuid->data[4]) << 20;
+       tms += ((uint64) uuid->data[5]) << 12;
+       tms += (((uint64) uuid->data[6]) & 0xf) << 8;
+       tms += ((uint64) uuid->data[7]);
+
+       /* convert 100-ns intervals to us, then adjust */
+       ts = (TimestampTz) (tms / 10) -
+           ((uint64) POSTGRES_EPOCH_JDATE - GREGORIAN_EPOCH_JDATE) *
SECS_PER_DAY * USECS_PER_SEC;
+
+       PG_RETURN_TIMESTAMPTZ(ts);
+   }

It's odd to me that only uuid_extract_timestamp() supports UUID v6 in
spite of not supporting UUID v6 generation. I think it makes more
sense to support UUID v6 generation as well, if the need for it is
high.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com