Thanks for your review, Aleksander!
> On 16 Jan 2024, at 18:00, Aleksander Alekseev <aleksan...@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?
> Also I would like to point out that part of the documentation is
> missing, but I guess at this stage of the game it's OK.
>
> Last but not least: maybe we should support casting Timestamp[Tz] to
> UUIDv7 and vice versa? Shouldn't be difficult to implement and I
> suspect somebody will request this eventually. During the cast to UUID
> we will always get the same value for the given Timestamp[Tz], which
> probably can be useful in certain applications. It can't be done with
> gen_uuid_v7() and its volatility doesn't permit it.
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.
