On Mon, Nov 25, 2024 at 10:15 AM Andrey M. Borodin <x4...@yandex-team.ru> wrote: > > > > > On 25 Nov 2024, at 22:53, Masahiko Sawada <sawada.m...@gmail.com> wrote: > > > > In the following code, we use "defined(__darwin__) || defined(_MSC_VER)": > > > > +#if defined(__darwin__) || defined(_MSC_VER) > > +#define SUBMS_MINIMAL_STEP_BITS 10 > > +#else > > +#define SUBMS_MINIMAL_STEP_BITS 12 > > +#endif > > #define SUBMS_BITS 12 > > -#define SUBMS_MINIMAL_STEP_NS ((NS_PER_MS / (1 << SUBMS_BITS)) + 1) > > +#define SUBMS_MINIMAL_STEP_NS ((NS_PER_MS / (1 << > > SUBMS_MINIMAL_STEP_BITS)) + 1) > > > > on the other hand, we use "defined(__darwin__) || defined(WIN32)" here: > > > > +#if defined(__darwin__) || defined(WIN32) > > + /* > > + * On MacOS real time is truncted to microseconds. Thus, 2 least > > + * significant are dependent on other time-specific bits, thus > > they do not > > + * contribute to uniqueness. To make these bit random we mix in two > > bits > > + * from CSPRNG. > > + * > > + * SUBMS_MINIMAL_STEP is chosen so that we still guarantee > > monotonicity > > + * despite altering these bits. > > + */ > > + uuid->data[7] = uuid->data[7] ^ (uuid->data[8] >> 6); > > +#endif > > > > Is there a reason for using different macros? > > No, that's an oversight. We should mix these 2 bits if an only if > SUBMS_MINIMAL_STEP_BITS=10. > > <tldr> > In your review change_v33.patch you used WIN32, but it did not actually > compile on Windows. > So on Saturday I squashed v33+change_v33.patch, and composed a message that I > think we still should switch to _MSC_VER. And just before sending I received > your message with v36 where you used _MSC_VER :) > > I think this way: > _MSC_VER - native Windows without clock_gettime, we used gettimeofday() and > 10 bits of sub-ms. > MinGW - we use clock_gettime() and 12 bits. > Darwin - we use clock_gettime() and 10 bits. > Anything else - clock_gettime() and 12 bits. > </tldr>
Thank you for the summary. On MinGW, IIUC we can get 100-ns precision timestamps[1], so using 12 bits for calculating the minimal step would make sense. Also, if we implement the Windows port of clock_gettime() in the future, we can remove the part of using gettimeofday() in get_real_time_ns_ascending(). It seems to me that it's over-engineering to implement that part only for the UUID v7. So the current implementation of get_real_time_ns_ascending() makes sense to me. > > > > > In get_real_time_ns_ascending(), we use _MSC_VER so we use > > clock_gettime() on MinGW. > > > >> > >> Sergey Prokhorenko just draw my attention to the new release of MariaDB > >> [0]. They are doing very similar UUID v7 generation as we do [1]. > >> > > > > Thank you for the references. It made me think that we can use the > > function name uuid_v7() rather than uuidv7(). > > I think it's a good idea if we will be kind of SQL-compatible. > Okay, let"s rename it. I've merged patches and renamed functions (also updated the commit message). Please find the attachment. Regards, [1] https://github.com/Alexpux/mingw-w64/blob/d0d7f784833bbb0b2d279310ddc6afb52fe47a46/mingw-w64-libraries/winpthreads/src/clock.c#L119 -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
v38-0001-Add-UUID-version-7-generation-function.patch
Description: Binary data