Re: UUID v7 - Mailing list pgsql-hackers

On Sun, Feb 2, 2025 at 2:15 AM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
>
>
>
> > On 31 Jan 2025, at 23:49, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > Thank you for the patch! I agree with the basic direction of this fix.
> > Here are some review comments:
> >
> > ---
> > -static inline int64 get_real_time_ns_ascending();
> > +static inline uint64 get_real_time_ns_ascending();
> >
> > IIUC we don't need to replace int64 with uint64 if we have two
> > separate parameters for generate_uuidv7(). It seems to be conventional
> > to use a signed int for timestamps.
>
> OK, done.
>
> >
> > ---
> > Need to update the function comment of generate_uuidv7() as we changed
> > the function arguments.
>
> Done.
>
> >
> > ---
> > -       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 / 1000, (us % 1000) * 1000 + ns % NS_PER_US);
> >
> > I think we can have an inline function or a marco (or use TMODULO()?)
> > to split nanoseconds into milliseconds and sub-milliseconds so that
> > uuidv7() and uuidv7_interval() can pass them to generate_uuidv7().
>
> I doubt that such macro will make core more readable. I've replaced 1000 with macros.
>
> >
> > The comments in uuidv7_interval() also need to be updated accordingly.
>
> Done.
>
> >
> > ---
> > I think we need to consider how we can handle the timestamp shifting.
> > UUIDv7 contains 48 bits Unix timestamp at milliseconds precision,
> > which can represent timestamps approximately between 2493 BC and 6432
> > AC. If users specify an interval to shift the timestamp beyond the
> > range, 48-bits timestamp would be wrapped around and they would not be
> > able to get an expected result. Do we need to raise an error in that
> > case?
> >
> > ---
> > Another problem I found in uuid_extract_timestamp() is that it cannot
> > correctly extract a timestamp before 1970/1/1 stored in a UUIDv7
> > value:
> >
> > postgres(1:1795331)=# select year, uuid_extract_timestamp(uuidv7((year
> > || 'year ago')::interval)) from generate_series(54, 56) year;
> > year |   uuid_extract_timestamp
> > ------+-----------------------------
> >   54 | 1971-01-31 10:46:25.111-08
> >   55 | 1970-01-31 10:46:25.111-08
> >   56 | 10888-09-01 17:18:15.768-07
> > (3 rows)
> >
> > The problem is that we correctly store a negative timestamp value in a
> > UUIDv7 value but uuid_extract_timestamp() unconditionally treats it as
> > a positive timestamp value. I think this is a separate bug we need to
> > fix.
>
>
> RFC says unix_ts_ms is unsigned. So, luckily, no BC dates.

Good to know.

> I think it's totally fine to wrap UUID values around year 10598 without an error.

Okay.

>
> 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)
>
>
> or maybe something simple like
>
> with u as (select uuidv7() id) select uuid_extract_timestamp(uuidv7('9999-09-09 12:34:56.789+05' -
uuid_extract_timestamp(u.id)))from u; 
>
> But it would still be flaky, second call to uuidv7() can overflow a millisecond.
>

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' ;

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.

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

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

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

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

Regards,

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