Re: UUID v7

On 27.11.24 00:11, Masahiko Sawada wrote:
> On Tue, Nov 26, 2024 at 1:55 PM Jelte Fennema-Nio <postgres(at)jeltef(dot)nl> wrote:
>>
>> On Tue, 26 Nov 2024 at 21:48, Sergey Prokhorenko
>> <sergeyprokhorenko(at)yahoo(dot)com(dot)au> wrote:
>>> gen_uuidv7() is OK
>>
>> I'd very much prefer to not have a gen_ or get_ prefix as argued before[1][2].
>>
>> My vote is still for simply uuidv7() and uuidv4()
>>
>>> uuid-ossp is outdated, slow and not supported by the author. UUIDv7 is the renaissance of UUIDs. So we should not depend on legacy technology names
>>
>> agreed
>>
>
> It seems that we agreed to use 'uuidv7' instead of 'uuid_v7()'. There
> is discussion whether we should add 'gen_' or 'get_' but let's go back
> to the previously-agreed function name 'uuidv7()' for now. We can
> rename it later if we find a better name.
>
> I've attached the new version patch that incorporated all comments and
> renamed the functions. Also I avoided using 'if defined(__darwin__) ||
> defined(_MSC_VER)' twice.

* doc/src/sgml/func.sgml

The function variant uuidv7(interval) is not documented.

* src/backend/utils/adt/uuid.c

+/* Set the given UUID version and the variant bits */
+static inline void
+uuid_set_version(pg_uuid_t *uuid, unsigned char version)
+{

This should be a block comment, like

/*
* Set the ...
*/

+/*
+ * Generate UUID version 7 per RFC 9562, with the given timestamp.
+ *
...
+static pg_attribute_always_inline pg_uuid_t *
+generate_uuidv7(int64 ns)

Is "ns" the timestamp argument? What format is it? Explain.

+ /*
+ * Shift the current timestamp by the given interval. To make correct
+ * calculating the time shift, we convert the UNIX epoch to TimestampTz
+ * and use timestamptz_pl_interval(). Since this calculation is done
with
+ * microsecond precision, we carry back the nanoseconds.
+ */

This needs a bit of grammar tweaking, I think: "To make correct calculating"

I don't know what the meaning of "carry back" is.

+ Interval *span = PG_GETARG_INTERVAL_P(0);

Not sure why this is named "span"? Maybe "shift" would be better?

* src/include/catalog/pg_proc.dat

+{ oid => '9897', descr => 'generate UUID version 7 with a timestamp
shifted on specific interval',

better "shifted by"?

* src/test/regress/expected/opr_sanity.out

+uuidv4()
+uuidv7()
+uuidv7(interval)

Functions without arguments don't need to be marked leakproof.

uuidv7(interval) internally calls timestamptz_pl_interval(), which is
not leakproof, so I don't think that classification is sound.

* src/test/regress/sql/uuid.sql

+SELECT uuid_extract_version(uuidv4()); --4
+SELECT uuid_extract_version(uuidv7()); --7

Make the whitespace of the comment consistent with the rest of the file.

-SELECT uuid_extract_timestamp('C232AB00-9414-11EC-B3C8-9F6BDECED846') =
'Tuesday, February 22, 2022 2:22:22.00 PM GMT+05:00'; -- RFC 4122bis
test vector
+SELECT uuid_extract_timestamp('C232AB00-9414-11EC-B3C8-9F6BDECED846') =
'Tuesday, February 22, 2022 2:22:22.00 PM GMT+05:00'; -- RFC 9562 test
vector for v1

Here as well.