Update to match changes in Postgres implementation

[ryanwinchester]

Postgres has changed their implementation from CommitFest to the code that is actually committed to their repo.

This library is intended to always match Postgres's implementation.

See: https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/uuid.c#L587

[hauleth]

I think that it would be better to simply use 1 as a @minimal_step_ns.

[hauleth]

That would be problematic when cross-compiling.

[ryanwinchester]

people successfully cross-compile?

[ryanwinchester]

i guess I could check at runtime :(

[hauleth]

Why not just set it to always be 1? I think it would be cleaner and still technically correct.

[ryanwinchester]

I was trying to match the Postgres implementation as close as possible, but maybe I will have to compromise here

[hauleth]

I know, but I do not see a point. If you do not have nanosecond precision there, then adding 1 is IMHO more secure than jumping somewhere wrt. precision of the OS. It is also secure to the same degree IMHO. This would also remove the cross-compilation problem.

I think it also reduces the potential problem of leaking information about runtime OS (if that is an issue). Theoretically it can leaks how many incoming requests tere are, but IMHO that is no different from "old" implementation, the difference is just with the returned value.

Additionally it gives more potential values to be generated within the same timeframe. I honestly do not see the point in making that value other than 1.

[ryanwinchester]

the previous implementation was a randomly-seeded counter (that resets every millisecond), but this version uses the increased clock precision method (method 3) of section 6.2 of the RFC:

https://www.rfc-editor.org/rfc/rfc9562.html#section-6.2-5.5

I think they are trying to stick within the RFC guidelines for Method 3 and stepping by the same fraction of a ms

[ryanwinchester]

maybe I should just default to always using 12 bits for the increased clock precision because nobody that would be concerned about getting 2 extra bits of randomness is going to run elixir in production on windows/mac ;)

plus it's a tradeoff/micro-optimization of not having to check the OS at runtime and do some extra calcs for only 2 bits

[notactuallytreyanastasio]

@ryanwinchester I have no horse in this race, but that option sounds most reasonable. Not having to care about the OS the BEAM is running on is a feature, as are 2 more bits of randomness.

[grzuy]

Thanks for building this library!

Leaving a comment about something I was curious about 🙏

[grzuy]

Does this mean that the sole purpose of the application start here is left to just initialize the persistent term?

Wouldn't you be able instead to have a default in UUIDv7.Clock on first call to Clock.next_ascending and be able to remove completely the need for a supervisor and an application?

Something like

diff --git a/lib/application.ex b/lib/application.ex
deleted file mode 100644
index 59a603f..0000000
--- a/lib/application.ex
+++ /dev/null
@@ -1,15 +0,0 @@
-defmodule UUIDv7.Application do
-  @moduledoc false
-  use Application
-
-  def start(_type, _args) do
-    # Put the timestamp ref into `:persistent_term` store.
-    # This is used in the `UUIDv7.Clock` module to determine the most
-    # recently-used timestamp.
-    clock_atomics_ref = :atomics.new(1, signed: false)
-    :persistent_term.put(UUIDv7.Clock, clock_atomics_ref)
-
-    opts = [strategy: :one_for_one, name: UUIDv7.Supervisor]
-    Supervisor.start_link([], opts)
-  end
-end
diff --git a/lib/clock.ex b/lib/clock.ex
index dbc23d7..fcfca54 100644
--- a/lib/clock.ex
+++ b/lib/clock.ex
@@ -24,8 +24,7 @@ defmodule UUIDv7.Clock do
   conditions.
   """
   def next_ascending do
-    # Get the atomics ref that we set in `UUIDv7.Application.start/2`.
-    timestamp_ref = :persistent_term.get(__MODULE__)
+    timestamp_ref = :persistent_term.get(__MODULE__, :atomics.new(1, signed: false))
 
     previous_ts = :atomics.get(timestamp_ref, 1)
     min_step_ts = previous_ts + @minimal_step_ns
diff --git a/mix.exs b/mix.exs
index d48ed19..652def1 100644
--- a/mix.exs
+++ b/mix.exs
@@ -24,8 +24,7 @@ defmodule UUIDv7.MixProject do
   # Run "mix help compile.app" to learn about applications.
   def application do
     [
-      extra_applications: [:logger],
-      mod: {UUIDv7.Application, []}
+      extra_applications: [:logger]
     ]
   end

Maybe I am missing something here.. he

[ryanwinchester]

nope, totally correct and great catch.

since I removed the genserver, UUIDv7.Application is now unnecessary

[ryanwinchester]

:persistent_term.get/2 default doesn't set it, so here's the new code

    timestamp_ref =
      with nil <- :persistent_term.get(__MODULE__, nil) do
        timestamp_ref = :atomics.new(1, signed: false)
        :ok = :persistent_term.put(__MODULE__, timestamp_ref)
        timestamp_ref
      end

[grzuy]

Right!
Sounds great.