Re: UUID v7

Thanks for the review!

> On 18 Oct 2024, at 02:16, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> 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.

Fixed.

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

Fixed.

>
> 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.

Done.

>
> ---
> + * 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.

Fixed.

>
> ---
> +{ 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.

I've slightly extended the description... not it's 'generate UUID version 7 with a timestamp shifted on specific interval'. Perhaps, we can come up with something better.

>
> ---
> - 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?

We have 4 options:
0. Remove uuidv7(interval). But it brings imporatne functionality to the table: we can avoid contention points while massively insert data.
1. Give different names to uuidv7() and uuidv7(interval).
2. Allow importing pg_node_tree (see v7 of the patch)
3. Change this query. Comment to this query suggest that it checks for exactly this case: same function is declared with different number of arguments.

IMO approach number 3 is best. However, I do not understand why this query check was introduced in the first place. Maybe, there are string arguments why we should not do same-named functions with different number of arguments.

>
> ---
> + 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.

RFC urges to use UUIDv7 instead of UUIDv6 when possible. I'm fine with providing implementation, it's trivial. PFA patch with implementation.

Best regards, Andrey Borodin.