Re: UUID v7

On Sat, Oct 26, 2024 at 10:05 AM Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
>
> >
> > ---
> > - 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.
>

I think we typically avoid this kind of check failure by assigning
uuidv7() and uuidv7(interval) different C functions that call the
common function. That is, we have pg_proc entries like:

{ 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 with a timestamp
shifted on specific interval',
  proname => 'uuidv7', proleakproof => 't', provolatile => 'v',
  prorettype => 'uuid', proargtypes => 'interval', prosrc =>
'uuidv7_interval' },

And then have C codes like:

static Datum
generate_uuidv7(FunctionCallInfo fcinfo)
{
    static uint64 previous_ns = 0;:
:
    PG_RETURN_UUID_P(uuid);
}

Datum
uuidv7(PG_FUNCTION_ARGS)
{
    return generate_uuidv7(fcinfo);
}

Datum
uuidv7_interval(PG_FUNCTION_ARGS)
{
    return generate_uuidv7(fcinfo);
}

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

My point is that we should either support full functionality for
UUIDv6 (such as generation and extraction) or none of them. I'm not
really sure we want UUIDv6 as well, but if we want it, it should be
implemented in a separate patch.

Regards,

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