Subject: Re: UUID v7

From: Masahiko Sawada <[email-masked]>
Date: Tue, 25 Mar 2025 21:32:49 -0700

On Sun, Feb 9, 2025 at 9:07 AM Andrey Borodin <[email-masked]> wrote:
>
> I've took into account note from Sergey that "offset" is better name for uuidv7() argument than "shift".
>
> > On 5 Feb 2025, at 03:02, Masahiko Sawada <[email-masked]> wrote:
> >
> >>
> >> I was thinking about incorporating test like this.
> >>
> >>>> With this patch we can generate correct UUIDs in a very distant future.
> >>>> postgres=# select x,                                                                                                                 uuid_extract_timestamp(uuidv7((x::text || ' year'::text)::interval)),
> >>>> (x::text || ' year'::text)::interval
> >>>> from generate_series(1,9000,1000) x;
> >>>> x   |   uuid_extract_timestamp    |  interval
> >>>> ------+-----------------------------+------------
> >>>>   1 | 2026-01-31 12:00:53.084+05  | 1 year
> >>>> 1001 | 3026-01-31 12:00:53.084+05  | 1001 years
> >>>> 2001 | 4026-01-31 12:00:53.084+05  | 2001 years
> >>>> 3001 | 5026-01-31 12:00:53.084+05  | 3001 years
> >>>> 4001 | 6026-01-31 12:00:53.084+05  | 4001 years
> >>>> 5001 | 7026-01-31 12:00:53.085+05  | 5001 years
> >>>> 6001 | 8026-01-31 12:00:53.085+05  | 6001 years
> >>>> 7001 | 9026-01-31 12:00:53.085+05  | 7001 years
> >>>> 8001 | 10026-01-31 12:00:53.085+05 | 8001 years
> >>>> (9 rows)
> >>
> >
> > Something like following queries might be workable for example?
> >
> > create table test (c serial, d uuid, t timestamptz generated always as
> > (uuid_extract_timestamp(d)) stored);
> > insert into test (d) select uuidv7((n || 'years')::interval) from
> > generate_series(1, 2000) n;
> > select count(*) from (select t - lag(t) over (order by c) as diff from
> > test) where diff > '10 year' ;
>
> Yeah, makes sense. I reduced tolerance to 366+1 day. Must be stable if we've done all the time offset business right.
>
> > Here are some review comments:
> >
> > #define NS_PER_S       INT64CONST(1000000000)
> > #define NS_PER_MS      INT64CONST(1000000)
> > +#define US_PER_MS      INT64CONST(1000)
> > #define NS_PER_US      INT64CONST(1000)
> >
> > I think it's clear if we put US_PER_MS below NS_PER_US.
>
> OK.
>
> > ---
> >  *
> > - * ns is a number of nanoseconds since start of the UNIX epoch. This value is
> > + * unix_ts_ms is a number of milliseconds since start of the UNIX epoch,
> > + * ns_in_ms is a number of nanoseconds within millisecond. These values are
> >  * used for time-dependent bits of UUID.
> >
> > I think we can mention that the RFC describes that stored unix
> > timestamp as an unsigned integer.
>
> Done. Feel free to adjust my wordings, I've no sense of idiomatic English.
>
> >
> > ---
> > static pg_uuid_t *
> > -generate_uuidv7(int64 ns)
> > +generate_uuidv7(uint64 unix_ts_ms, uint32 ns_in_ms)
> >
> > How about renaming ns_in_ms with sub_ms?
>
> OK.
>
> >
> > ---
> > +        /* 64 bits is enough for real time, but not for a time range of UUID */
> >
> > I could not understand the point of this comment. It seems to say that
> > 64-bits is not enough for a time range of UUID, but doesn't the time
> > range of UUIDv7 use only 48 bits? It seems to need more comments.
>
> I've tried to say that acquiring current time as an int64 ns since UNIX epoch is still viable for the code (until year 2262).
>
>
> > ---
> > -        ns = (ts + (POSTGRES_EPOCH_JDATE - UNIX_EPOCH_JDATE) *
> > SECS_PER_DAY * USECS_PER_SEC)
> > -                * NS_PER_US + ns % NS_PER_US;
> > +        us = (ts + (POSTGRES_EPOCH_JDATE - UNIX_EPOCH_JDATE) *
> > SECS_PER_DAY * USECS_PER_SEC);
> >
> >         /* Generate an UUIDv7 */
> > -        uuid = generate_uuidv7(ns);
> > +        uuid = generate_uuidv7(us / US_PER_MS, (us % US_PER_MS) *
> > NS_PER_US + ns % NS_PER_US);
> >
> > Need to update comments in uuidv7_internval() such as:
> >
> >    /*
> >     * Shift the current timestamp by the given interval. To calculate time
> >     * shift correctly, we convert the UNIX epoch to TimestampTz and use
> >     * timestamptz_pl_interval(). Since this calculation is done with
> >     * microsecond precision, we carry nanoseconds from original ns value to
> >     * shifted ns value.
> >     */
> >
> > and
> >
> >    /*
> >     * Convert a TimestampTz value back to an UNIX epoch and back nanoseconds.
> >     */
>
> I've tried. I'm not very satisfied with comments, but could not come up with easier description.
>

Thank you for updating the patch. I had missed to track this patch.

I've updated the patch from your v4 patch. In this version, I excluded
the argument name change (from 'shift' to 'offset') as it's not
related to the bug fix and simplified the regression test case.

Please review it.

Regards,

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

---

From: Andrey Borodin <[email-masked]>
Date: Wed, 26 Mar 2025 17:00:19 +0400

> On 26 Mar 2025, at 08:32, Masahiko Sawada <[email-masked]> wrote:
> 
> Please review it.

The patch looks good to me except one nit.
+WITH uuidts AS (
+     SELECT y, ts as ts, lag(ts) OVER (ORDER BY y) AS prev_ts
+     FROM (SELECT y, uuid_extract_timestamp(uuidv7((y || ' years')::interval)) AS ts FROM generate_series(-50, 10889 - extract(year from now())::int) y)
+)
+SELECT y, ts, prev_ts FROM uuidts WHERE ts < prev_ts;

if "extract(year from now())::int)" runs slightly before new year and the rest of the test after - the test will fail. How about avoiding overflow by using 10888 instead of 10889?

If we are sure citizen time never will go back, IMO we can safely move other border back to -55.

Also the test is not proof to NTP time drift during New Year's edge, but it's hardly a problem. The test, NTP clock sync and New Year millisecond must coincide for a false failure.

Thank you!

Best regards, Andrey Borodin.

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 26 Mar 2025 10:06:10 -0700

> if "extract(year from now())::int)" runs slightly before new year and the rest of the test after - the test will fail. How about avoiding overflow by using 10888 instead of 10889?

Agreed. I've done this in the attached patch.

> If we are sure citizen time never will go back, IMO we can safely move other border back to -55.

Yes. Or I think we can verify the range from 1970 to 10888 like I did
in the updated patch.

> Also the test is not proof to NTP time drift during New Year's edge, but it's hardly a problem. The test, NTP clock sync and New Year millisecond must coincide for a false failure.

Agreed.

Regards,

---

From: Andrei Borodin <[email-masked]>
Date: Thu, 27 Mar 2025 00:32:36 +0500

26.03.2025, 21:06, "Masahiko Sawada" <[email-masked]>:

Great! The patch looks good to me.

---

From: Masahiko Sawada <[email-masked]>
Date: Thu, 27 Mar 2025 22:31:48 -0700

> Great! The patch looks good to me.

Thank you for reviewing it. I'm going to push the fix tomorrow,
barring further comments.

Regards,

---

From: Masahiko Sawada <[email-masked]>
Date: Fri, 28 Mar 2025 17:09:27 -0700

> barring further comments.

Pushed.

Regards,