Re: UUID v7

Thanks for your review, Aleksander!

> On 16 Jan 2024, at 18:00, Aleksander Alekseev <aleksander@timescale.com> wrote:
>
>
> ```
> +Datum
> +pg_node_tree_in(PG_FUNCTION_ARGS)
> +{
> +    if (!IsBootstrapProcessingMode())
> +        elog(ERROR, "cannot accept a value of type pg_node_tree_in");
> +    return textin(fcinfo);
> +}
> ```
>
> Not 100% sure what this is for. Any chance this could be part of another patch?
Nope, it’s necessary there. Without these changes catalog functions cannot have defaults for arguments. These defaults have type pg_node_tree which has no-op in function.

> One thing I don't particularly like about the tests is the fact that
> they don't check if a correct UUID was actually generated. I realize
> that's not quite trivial due to the random nature of the function, but
> maybe we could use some substring/regex magic here? Something like:
>
> ```
> select gen_uuid_v7() :: text ~ '^[0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12}$';
> ?column?
> ----------
> t
>
> select regexp_replace(gen_uuid_v7('2024-01-16 15:45:33 MSK') :: text,
> '[0-9a-f]{4}-[0-9a-f]{12}$', 'XXXX-' || repeat('X', 12));
>            regexp_replace
> --------------------------------------
> 018d124e-39c8-74c7-XXXX-XXXXXXXXXXXX
> ```
Any 8 bytes which have ver and var bits (6 bits total) are correct UUID.
This is checked by tests when uuid_var() and uuid_ver() functions are exercised.

> ```
> +  proname => 'uuid_v7_time', proleakproof => 't', provolatile => 'i',
> ```
>
> I don't think we conventionally specify IMMUTABLE volatility, it's the
> default. Other values also are worth checking.
Makes sense, I’ll drop this values in next version.
BTW I’m in doubt if provided functions are leakproof. They ERROR-out with messages that can give a clue about several bits of UUID. Does this break leakproofness? I think yest, but I’m not sure.
gen_uuid_v7() seems leakproof to me.

> Another question: how did you choose between using TimestampTz and
> Timestamp types? I realize that internally it's all the same. Maybe
> Timestamp will be slightly better since the way it is displayed
> doesn't depend on the session settings. Many people I talked to find
> this part of TimestampTz confusing.

I mean, this argument is expected to be used to implement K-way sorted identifiers. In this context, it seems to me, it’s good to remember to developer that time shift also depend on timezones.
But this is too vague.
Do you have any reasons that apply to UUID generation?
I’m strongly opposed to doing this cast. I was not adding this function to extract timestamp from UUID, because standard does not recommend it. But a lot of people asked for this.
But supporting easy way to do unrecommended thing seem bad.


Best regards, Andrey Borodin.