Re: UUID v7

On Sun, Aug 4, 2024 at 3:51 AM Andrey M. Borodin <x4mmm(at)yandex-team(dot)ru> wrote:
>
>
>
> > On 28 Jul 2024, at 23:44, Andrey M. Borodin <x4mmm(at)yandex-team(dot)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