Re: [Tarantool-patches] [PATCH v5 4/8] box, datetime: messagepack support for datetime

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

18.08.2021 02:42, Safin Timur пишет:
> Thanks Sergey for your feedback, below you'll see few comments and 
> incremental patch...
>
> On 17.08.2021 15:16, Serge Petrenko wrote:
>>
>>
>> 16.08.2021 02:59, Timur Safin via Tarantool-patches пишет:
>>> Serialize datetime_t as newly introduced MP_EXT type.
>>> It saves 1 required integer field and upto 2 optional
>>> unsigned fields in very compact fashion.
>>> - secs is required field;
>>> - but nsec, offset are both optional;
>>>
>>> * json, yaml serialization formats, lua output mode
>>>    supported;
>>> * exported symbols for datetime messagepack size calculations
>>>    so they are available for usage on Lua side.
>>>
>>> Part of #5941
>>> Part of #5946
>>> ---
>>>   extra/exports                     |   5 +-
>>>   src/box/field_def.c               |  35 +++---
>>>   src/box/field_def.h               |   1 +
>>>   src/box/lua/serialize_lua.c       |   7 +-
>>>   src/box/msgpack.c                 |   7 +-
>>>   src/box/tuple_compare.cc          |  20 ++++
>>>   src/lib/core/CMakeLists.txt       |   4 +-
>>>   src/lib/core/datetime.c           |   9 ++
>>>   src/lib/core/datetime.h           |  11 ++
>>>   src/lib/core/mp_datetime.c        | 189 
>>> ++++++++++++++++++++++++++++++
>>>   src/lib/core/mp_datetime.h        |  89 ++++++++++++++
>>>   src/lib/core/mp_extension_types.h |   1 +
>>>   src/lib/mpstream/mpstream.c       |  11 ++
>>>   src/lib/mpstream/mpstream.h       |   4 +
>>>   src/lua/msgpack.c                 |  12 ++
>>>   src/lua/msgpackffi.lua            |  18 +++
>>>   src/lua/serializer.c              |   4 +
>>>   src/lua/serializer.h              |   2 +
>>>   src/lua/utils.c                   |   1 -
>>>   test/unit/datetime.c              | 125 +++++++++++++++++++-
>>>   test/unit/datetime.result         | 115 +++++++++++++++++-
>>>   third_party/lua-cjson/lua_cjson.c |   8 ++
>>>   third_party/lua-yaml/lyaml.cc     |   6 +-
>>>   23 files changed, 661 insertions(+), 23 deletions(-)
>>>   create mode 100644 src/lib/core/mp_datetime.c
>>>   create mode 100644 src/lib/core/mp_datetime.h
>>>
>>> diff --git a/extra/exports b/extra/exports
>>> index 2437e175c..c34a5c2b5 100644
>>> --- a/extra/exports
>>> +++ b/extra/exports
>>> @@ -151,9 +151,10 @@ csv_setopt
>>>   datetime_asctime
>>>   datetime_ctime
>>>   datetime_now
>>> +datetime_pack
>>>   datetime_strftime
>>>   datetime_to_string
>>> -decimal_unpack
>>> +datetime_unpack
>>
>>
>> decimal_unpack should stay there.
>
> It's there, but 2 lines below :)
>
> That was me copy-pasted decimal_unpack a few patches before, but has 
> not changed it to datetime_unpack. I've corrected it in this patch.
>
> I've now corrected the original appearance, now with correct name.

Ok, thanks!

>
>>
>>
>>>   decimal_from_string
>>>   decimal_unpack
>
>      ^^^ it was here
>
>>>   tnt_dt_dow
>>> @@ -397,6 +398,7 @@ mp_decode_uint
>>>   mp_encode_array
>>>   mp_encode_bin
>>>   mp_encode_bool
>>> +mp_encode_datetime
>>>   mp_encode_decimal
>>>   mp_encode_double
>>>   mp_encode_float
>>> @@ -413,6 +415,7 @@ mp_next
>>>   mp_next_slowpath
>>>   mp_parser_hint
>>>   mp_sizeof_array
>>> +mp_sizeof_datetime
>>>   mp_sizeof_decimal
>>>   mp_sizeof_str
>>>   mp_sizeof_uuid
>>> diff --git a/src/box/field_def.c b/src/box/field_def.c
>>> index 51acb8025..2682a42ee 100644
>>> --- a/src/box/field_def.c
>>> +++ b/src/box/field_def.c
>>> @@ -72,6 +72,7 @@ const uint32_t field_mp_type[] = {
>>>       /* [FIELD_TYPE_UUID]     =  */ 0, /* only MP_UUID is supported */
>>>       /* [FIELD_TYPE_ARRAY]    =  */ 1U << MP_ARRAY,
>>>       /* [FIELD_TYPE_MAP]      =  */ (1U << MP_MAP),
>>> +    /* [FIELD_TYPE_DATETIME] =  */ 0, /* only MP_DATETIME is 
>>> supported */
>>>   };
>>>   const uint32_t field_ext_type[] = {
>>> @@ -83,11 +84,13 @@ const uint32_t field_ext_type[] = {
>>>       /* [FIELD_TYPE_INTEGER]   = */ 0,
>>>       /* [FIELD_TYPE_BOOLEAN]   = */ 0,
>>>       /* [FIELD_TYPE_VARBINARY] = */ 0,
>>> -    /* [FIELD_TYPE_SCALAR]    = */ (1U << MP_DECIMAL) | (1U << 
>>> MP_UUID),
>>> +    /* [FIELD_TYPE_SCALAR]    = */ (1U << MP_DECIMAL) | (1U << 
>>> MP_UUID) |
>>> +        (1U << MP_DATETIME),
>>>       /* [FIELD_TYPE_DECIMAL]   = */ 1U << MP_DECIMAL,
>>>       /* [FIELD_TYPE_UUID]      = */ 1U << MP_UUID,
>>>       /* [FIELD_TYPE_ARRAY]     = */ 0,
>>>       /* [FIELD_TYPE_MAP]       = */ 0,
>>> +    /* [FIELD_TYPE_DATETIME]  = */ 1U << MP_DATETIME,
>>>   };
>>>   const char *field_type_strs[] = {
>>> @@ -104,6 +107,7 @@ const char *field_type_strs[] = {
>>>       /* [FIELD_TYPE_UUID]     = */ "uuid",
>>>       /* [FIELD_TYPE_ARRAY]    = */ "array",
>>>       /* [FIELD_TYPE_MAP]      = */ "map",
>>> +    /* [FIELD_TYPE_DATETIME] = */ "datetime",
>>>   };
>>>   const char *on_conflict_action_strs[] = {
>>> @@ -128,20 +132,21 @@ field_type_by_name_wrapper(const char *str, 
>>> uint32_t len)
>>>    * values can be stored in the j type.
>>>    */
>>>   static const bool field_type_compatibility[] = {
>>> -       /*   ANY   UNSIGNED  STRING   NUMBER  DOUBLE  INTEGER 
>>> BOOLEAN VARBINARY SCALAR  DECIMAL   UUID    ARRAY    MAP  */
>>> -/*   ANY    */ true,   false,   false,   false,   false, false,   
>>> false,   false,  false,  false,  false,   false, false,
>>> -/* UNSIGNED */ true,   true,    false,   true,    false, true,    
>>> false,   false,  true,   false,  false,   false, false,
>>> -/*  STRING  */ true,   false,   true,    false,   false, false,   
>>> false,   false,  true,   false,  false,   false, false,
>>> -/*  NUMBER  */ true,   false,   false,   true,    false, false,   
>>> false,   false,  true,   false,  false,   false, false,
>>> -/*  DOUBLE  */ true,   false,   false,   true,    true, false,   
>>> false,   false,  true,   false,  false,   false, false,
>>> -/*  INTEGER */ true,   false,   false,   true,    false, true,    
>>> false,   false,  true,   false,  false,   false, false,
>>> -/*  BOOLEAN */ true,   false,   false,   false,   false, false,   
>>> true,    false,  true,   false,  false,   false, false,
>>> -/* VARBINARY*/ true,   false,   false,   false,   false, false,   
>>> false,   true,   true,   false,  false,   false, false,
>>> -/*  SCALAR  */ true,   false,   false,   false,   false, false,   
>>> false,   false,  true,   false,  false,   false, false,
>>> -/*  DECIMAL */ true,   false,   false,   true,    false, false,   
>>> false,   false,  true,   true,   false,   false, false,
>>> -/*   UUID   */ true,   false,   false,   false,   false, false,   
>>> false,   false,  false,  false,  true,    false, false,
>>> -/*   ARRAY  */ true,   false,   false,   false,   false, false,   
>>> false,   false,  false,  false,  false,   true, false,
>>> -/*    MAP   */ true,   false,   false,   false,   false, false,   
>>> false,   false,  false,  false,  false,   false, true,
>>> +       /*   ANY   UNSIGNED  STRING   NUMBER  DOUBLE  INTEGER 
>>> BOOLEAN VARBINARY SCALAR  DECIMAL   UUID    ARRAY    MAP DATETIME */
>>> +/*   ANY    */ true,   false,   false,   false,   false, false,   
>>> false,   false,  false,  false,  false,   false, false,   false,
>>> +/* UNSIGNED */ true,   true,    false,   true,    false, true,    
>>> false,   false,  true,   false,  false,   false, false,   false,
>>> +/*  STRING  */ true,   false,   true,    false,   false, false,   
>>> false,   false,  true,   false,  false,   false, false,   false,
>>> +/*  NUMBER  */ true,   false,   false,   true,    false, false,   
>>> false,   false,  true,   false,  false,   false, false,   false,
>>> +/*  DOUBLE  */ true,   false,   false,   true,    true, false,   
>>> false,   false,  true,   false,  false,   false, false,   false,
>>> +/*  INTEGER */ true,   false,   false,   true,    false, true,    
>>> false,   false,  true,   false,  false,   false, false,   false,
>>> +/*  BOOLEAN */ true,   false,   false,   false,   false, false,   
>>> true,    false,  true,   false,  false,   false, false,   false,
>>> +/* VARBINARY*/ true,   false,   false,   false,   false, false,   
>>> false,   true,   true,   false,  false,   false, false,   false,
>>> +/*  SCALAR  */ true,   false,   false,   false,   false, false,   
>>> false,   false,  true,   false,  false,   false, false,   false,
>>> +/*  DECIMAL */ true,   false,   false,   true,    false, false,   
>>> false,   false,  true,   true,   false,   false, false,   false,
>>> +/*   UUID   */ true,   false,   false,   false,   false, false,   
>>> false,   false,  false,  false,  true,    false, false,   false,
>>> +/*   ARRAY  */ true,   false,   false,   false,   false, false,   
>>> false,   false,  false,  false,  false,   true, false,   false,
>>> +/*    MAP   */ true,   false,   false,   false,   false, false,   
>>> false,   false,  false,  false,  false,   false, true,    false,
>>> +/* DATETIME */ true,   false,   false,   false,   false, false,   
>>> false,   false,  true,   false,  false,   false, false,   true,
>>>   };
>>>   bool
>>> diff --git a/src/box/field_def.h b/src/box/field_def.h
>>> index c5cfe5e86..120b2a93d 100644
>>> --- a/src/box/field_def.h
>>> +++ b/src/box/field_def.h
>>> @@ -63,6 +63,7 @@ enum field_type {
>>>       FIELD_TYPE_UUID,
>>>       FIELD_TYPE_ARRAY,
>>>       FIELD_TYPE_MAP,
>>> +    FIELD_TYPE_DATETIME,
>>>       field_type_MAX
>>>   };
>>
>>
>> Please, define FIELD_TYPE_DATETIME higher.
>> Right after FIELD_TYPE_UUID.
>>
>> This way you won't need to rework field type allowed in index check
>> in the next commit.
>
> That's very straighforward and easy, my bad that I've overcomplicated it!
>
> But I'll move the change to the next patch, as it'scorrectly has 
> pointed out by Vova, should be part of indices support,
>

Ok.

>
>>
>>
>>> diff --git a/src/box/lua/serialize_lua.c b/src/box/lua/serialize_lua.c
>>> index 1f791980f..51855011b 100644
>>> --- a/src/box/lua/serialize_lua.c
>>> +++ b/src/box/lua/serialize_lua.c
>>> @@ -768,7 +768,7 @@ static int
>>>   dump_node(struct lua_dumper *d, struct node *nd, int indent)
>>>   {
>>>       struct luaL_field *field = &nd->field;
>>> -    char buf[FPCONV_G_FMT_BUFSIZE];
>>> +    char buf[FPCONV_G_FMT_BUFSIZE + 8];
>>
>>
>> Why "+8"?
>
> Well, because current FPCONV_G_FMT_BUFSIZE (32) was not enough for 
> full ISO-8601 literal with nanoseconds :)
>
> Probably I should introduce some newer constant...
>
> [Or, as Vova has suggested - just to use MAX from those 2 values, my 
> length and FPCONV_G_FMT_BUFSIZE.]
>
> --------------------------------------------
> diff --git a/src/box/lua/serialize_lua.c b/src/box/lua/serialize_lua.c
> index 51855011b..eef3a4995 100644
> --- a/src/box/lua/serialize_lua.c
> +++ b/src/box/lua/serialize_lua.c
> @@ -768,7 +768,7 @@ static int
>  dump_node(struct lua_dumper *d, struct node *nd, int indent)
>  {
>      struct luaL_field *field = &nd->field;
> -    char buf[FPCONV_G_FMT_BUFSIZE + 8];
> +    char buf[MAX(FPCONV_G_FMT_BUFSIZE, DT_TO_STRING_BUFSIZE)];
>      int ltype = lua_type(d->L, -1);
>      const char *str = NULL;
>      size_t len = 0;
> diff --git a/src/lib/core/datetime.h b/src/lib/core/datetime.h
> index 497cd9f14..b8d179600 100644
> --- a/src/lib/core/datetime.h
> +++ b/src/lib/core/datetime.h
> @@ -87,6 +87,11 @@ struct datetime_interval {
>  int
>  datetime_compare(const struct datetime *lhs, const struct datetime 
> *rhs);
>
> +/**
> + * Required size of datetime_to_string string buffer
> + */
> +#define DT_TO_STRING_BUFSIZE   48
> +
>  /**
>   * Convert datetime to string using default format
>   * @param date source datetime value
> --------------------------------------------
>

Looks good.

>
>>
>>
>>>       int ltype = lua_type(d->L, -1);
>>>       const char *str = NULL;
>>>       size_t len = 0;
>>
>>
>> <stripped>
>>
>>
>>> diff --git a/src/lib/core/mp_datetime.c b/src/lib/core/mp_datetime.c
>>> new file mode 100644
>>> index 000000000..d0a3e562c
>>> --- /dev/null
>>> +++ b/src/lib/core/mp_datetime.c
>>> @@ -0,0 +1,189 @@
>>> +/*
>>> + * Copyright 2021, Tarantool AUTHORS, please see AUTHORS file.
>>> + *
>>> + * Redistribution and use in source and binary forms, with or
>>> + * without modification, are permitted provided that the following
>>> + * conditions are met:
>>> + *
>>> + * 1. Redistributions of source code must retain the above
>>> + *    copyright notice, this list of conditions and the
>>> + *    following disclaimer.
>>> + *
>>> + * 2. Redistributions in binary form must reproduce the above
>>> + *    copyright notice, this list of conditions and the following
>>> + *    disclaimer in the documentation and/or other materials
>>> + *    provided with the distribution.
>>> + *
>>> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
>>> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
>>> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
>>> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
>>> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
>>> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
>>> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
>>> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
>>> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
>>> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>>> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
>>> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>>> + * SUCH DAMAGE.
>>> + */
>>> +
>>
>> Same about the license.
>> Please, replace that with
>>
>> /*
>>   * SPDX-License-Identifier: BSD-2-Clause
>>   *
>>   * Copyright 2021, Tarantool AUTHORS, please see AUTHORS file.
>>   */
>>
>> And do the same for all new files.
>
> Updated.
>
>>
>>> +#include "mp_datetime.h"
>>> +#include "msgpuck.h"
>>> +#include "mp_extension_types.h"
>>> +
>>> +/*
>>> +  Datetime MessagePack serialization schema is MP_EXT (0xC7 for 1 
>>> byte length)
>>> +  extension, which creates container of 1 to 3 integers.
>>> +
>>> + 
>>> +----+---+-----------+====~~~~~~~====+-----~~~~~~~~-------+....~~~~~~~....+ 
>>>
>>> +  |0xC7| 4 |len (uint8)| seconds (int) | nanoseconds (uint) | 
>>> offset (uint) |
>>> + 
>>> +----+---+-----------+====~~~~~~~====+-----~~~~~~~~-------+....~~~~~~~....+ 
>>>
>>
>> The order should be 0xC7, len(uint8), 4, seconds, ...
>> according to
>> https://github.com/msgpack/msgpack/blob/master/spec.md#ext-format-family
>
> Indeed, that was my misconception, thanks for correction!
> [Updated picture in the patch and in the discussion - 
> https://github.com/tarantool/tarantool/discussions/6244#discussioncomment-1043990]
>
>>
>>> +
>>> +  MessagePack extension MP_EXT (0xC7), after 1-byte length, contains:
>>> +
>>> +  - signed integer seconds part (required). Depending on the value of
>>> +    seconds it may be from 1 to 8 bytes positive or negative 
>>> integer number;
>>> +
>>> +  - [optional] fraction time in nanoseconds as unsigned integer.
>>> +    If this value is 0 then it's not saved (unless there is offset 
>>> field,
>>> +    as below);
>>> +
>>> +  - [optional] timzeone offset in minutes as unsigned integer.
>>> +    If this field is 0 then it's not saved.
>>> + */
>>> +
>>> +static inline uint32_t
>>> +mp_sizeof_Xint(int64_t n)
>>> +{
>>> +    return n < 0 ? mp_sizeof_int(n) : mp_sizeof_uint(n);
>>> +}
>>> +
>>> +static inline char *
>>> +mp_encode_Xint(char *data, int64_t v)
>>> +{
>>> +    return v < 0 ? mp_encode_int(data, v) : mp_encode_uint(data, v);
>>> +}
>>> +
>>> +static inline int64_t
>>> +mp_decode_Xint(const char **data)
>>> +{
>>> +    switch (mp_typeof(**data)) {
>>> +    case MP_UINT:
>>> +        return (int64_t)mp_decode_uint(data);
>>> +    case MP_INT:
>>> +        return mp_decode_int(data);
>>> +    default:
>>> +        mp_unreachable();
>>> +    }
>>> +    return 0;
>>> +}
>>
>> I believe mp_decode_Xint and mp_encode_Xint
>> belong to a more generic file, but I couldn't find an
>> appropriate one. Up to you.
>
> Yup, it was planned to be placed to more generic place once it would 
> become useful at least the 2nd time. And this time is actually 2nd 
> (1st was in SQL AST parser branch here 
> https://github.com/tarantool/tarantool/commit/55a4182ebfbed1a3c916fb7e326f8f7861776a7f#diff-e3f5bdfa58bcaed35b89f22e94be7ad472a6b37d656a129722ea0d5609503c6aR132-R143). 
> But that patchset has not yet landed to the master, so once again code 
> usage is 1st time and worth only local application. When I'll return 
> to distributed-sql AST parser I'll reshake them and put elsewhere.
>
>
>>
>>> +
>>> +static inline uint32_t
>>> +mp_sizeof_datetime_raw(const struct datetime *date)
>>> +{
>>> +    uint32_t sz = mp_sizeof_Xint(date->secs);
>>> +
>>> +    // even if nanosecs == 0 we need to output anything
>>> +    // if we have non-null tz offset
>>
>>
>> Please, stick with our comment format:
>
> Oh, yup, that slipt thru. Corrected.
>
>>
>> /*
>>   * Even if nanosecs == 0 we need to output anything
>>   * if we have non-null tz offset
>> */
>>
>>
>>> +    if (date->nsec != 0 || date->offset != 0)
>>> +        sz += mp_sizeof_Xint(date->nsec);
>>> +    if (date->offset)
>>> +        sz += mp_sizeof_Xint(date->offset);
>>> +    return sz;
>>> +}
>>> +
>>> +uint32_t
>>> +mp_sizeof_datetime(const struct datetime *date)
>>> +{
>>> +    return mp_sizeof_ext(mp_sizeof_datetime_raw(date));
>>> +}
>>> +
>>> +struct datetime *
>>> +datetime_unpack(const char **data, uint32_t len, struct datetime 
>>> *date)
>>> +{
>>> +    const char * svp = *data;
>>> +
>>> +    memset(date, 0, sizeof(*date));
>>> +
>>> +    date->secs = mp_decode_Xint(data);
>>
>>
>> Please, leave a comment about date->secs possible range here.
>> Why is it ok to store a decoded int64_t in a double.
>
> Yes, that's reasonable complain. I'll document dt supported range in 
> the datetime.h header, and to declare legal bounds there, so we could 
> use them later in asserts.
>
> Please see incremental patch for this step below...
>
>>
>>
>>> +
>>> +    len -= *data - svp;
>>> +    if (len <= 0)
>>> +        return date;
>>> +
>>> +    svp = *data;
>>> +    date->nsec = mp_decode_Xint(data);
>>> +    len -= *data - svp;
>>> +
>>> +    if (len <= 0)
>>> +        return date;
>>> +
>>> +    date->offset = mp_decode_Xint(data);
>>> +
>>> +    return date;
>>> +}
>>> +
>>> +struct datetime *
>>> +mp_decode_datetime(const char **data, struct datetime *date)
>>> +{
>>> +    if (mp_typeof(**data) != MP_EXT)
>>> +        return NULL;
>>> +
>>> +    int8_t type;
>>> +    uint32_t len = mp_decode_extl(data, &type);
>>> +
>>> +    if (type != MP_DATETIME || len == 0) {
>>> +        return NULL;
>>
>>
>> Please, revert data to savepoint when decoding fails.
>> If mp_decode_extl or datetime_unpack fail, you mustn't
>> modify data.
>>
>
> Didn't think about this case - will make sure data points to the 
> original location if fails.
>
>>
>>> +    }
>>> +    return datetime_unpack(data, len, date);
>>> +}
>>> +
>>> +char *
>>> +datetime_pack(char *data, const struct datetime *date)
>>> +{
>>> +    data = mp_encode_Xint(data, date->secs);
>>> +    if (date->nsec != 0 || date->offset != 0)
>>> +        data = mp_encode_Xint(data, date->nsec);
>>> +    if (date->offset)
>>> +        data = mp_encode_Xint(data, date->offset);
>>> +
>>> +    return data;
>>> +}
>>
>>
>> <stripped>
>>
>>
>>> diff --git a/src/lua/serializer.h b/src/lua/serializer.h
>>> index 0a0501a74..e7a240e0a 100644
>>> --- a/src/lua/serializer.h
>>> +++ b/src/lua/serializer.h
>>> @@ -52,6 +52,7 @@ extern "C" {
>>>   #include <lauxlib.h>
>>>   #include "trigger.h"
>>> +#include "lib/core/datetime.h"
>>>   #include "lib/core/decimal.h" /* decimal_t */
>>>   #include "lib/core/mp_extension_types.h"
>>>   #include "lua/error.h"
>>> @@ -223,6 +224,7 @@ struct luaL_field {
>>>           uint32_t size;
>>>           decimal_t *decval;
>>>           struct tt_uuid *uuidval;
>>> +        struct datetime *dateval;
>>>       };
>>>       enum mp_type type;
>>>       /* subtypes of MP_EXT */
>>> diff --git a/src/lua/utils.c b/src/lua/utils.c
>>> index 2c89326f3..771f6f278 100644
>>> --- a/src/lua/utils.c
>>> +++ b/src/lua/utils.c
>>> @@ -254,7 +254,6 @@ luaL_setcdatagc(struct lua_State *L, int idx)
>>>       lua_pop(L, 1);
>>>   }
>>> -
>>
>>
>> Extraneous change. Please, remove.
>
> Removed from the patch. Thanks!
>
>>
>>
>>>   /**
>>>    * A helper to register a single type metatable.
>>>    */
>>> diff --git a/test/unit/datetime.c b/test/unit/datetime.c
>>> index 1ae76003b..a72ac2253 100644
>>> --- a/test/unit/datetime.c
>>> +++ b/test/unit/datetime.c
>>> @@ -6,6 +6,9 @@
>>>   #include "unit.h"
>>>   #include "datetime.h"
>>> +#include "mp_datetime.h"
>>> +#include "msgpuck.h"
>>> +#include "mp_extension_types.h"
>>>   static const char sample[] = "2012-12-24T15:30Z";
>>> @@ -247,12 +250,132 @@ tostring_datetime_test(void)
>>>       check_plan();
>>>   }
>>>
>>
>>
>> <stripped>
>>
>
>
> -----------------------------------------------------
> diff --git a/src/lib/core/datetime.h b/src/lib/core/datetime.h
> index f98f7010d..df3c1c83d 100644
> --- a/src/lib/core/datetime.h
> +++ b/src/lib/core/datetime.h
> @@ -5,6 +5,7 @@
>   * Copyright 2021, Tarantool AUTHORS, please see AUTHORS file.
>   */
>
> +#include <limits.h>
>  #include <stdint.h>
>  #include <stdbool.h>
>  #include "c-dt/dt.h"
> @@ -30,6 +31,26 @@ extern "C"
>  #define DT_EPOCH_1970_OFFSET  719163
>  #endif
>
> +/**
> + * c-dt library uses int as type for dt value, which
> + * represents the number of days since Rata Die date.
> + * This implies limits to the number of seconds we
> + * could safely store in our structures and then safely
> + * pass to c-dt functions.
> + *
> + * So supported ranges will be
> + * - for seconds [-185604722870400 .. 185480451417600]
> + * - for dates   [-5879610-06-22T00:00Z .. 5879611-07-11T00:00Z]
> + */
> +#define MAX_DT_DAY_VALUE (int64_t)INT_MAX
> +#define MIN_DT_DAY_VALUE (int64_t)INT_MIN
> +#define SECS_EPOCH_1970_OFFSET     \
> +    ((int64_t)DT_EPOCH_1970_OFFSET * SECS_PER_DAY)
> +#define MAX_EPOCH_SECS_VALUE    \
> +    (MAX_DT_DAY_VALUE * SECS_PER_DAY - SECS_EPOCH_1970_OFFSET)
> +#define MIN_EPOCH_SECS_VALUE    \
> +    (MIN_DT_DAY_VALUE * SECS_PER_DAY - SECS_EPOCH_1970_OFFSET)
> +
>  /**
>   * datetime structure keeps number of seconds since
>   * Unix Epoch.
> diff --git a/src/lib/core/mp_datetime.c b/src/lib/core/mp_datetime.c
> index 7e475d5f1..963752c23 100644
> --- a/src/lib/core/mp_datetime.c
> +++ b/src/lib/core/mp_datetime.c
> @@ -1,34 +1,12 @@
>  /*
> - * Copyright 2021, Tarantool AUTHORS, please see AUTHORS file.
> - *
> - * Redistribution and use in source and binary forms, with or
> - * without modification, are permitted provided that the following
> - * conditions are met:
> - *
> - * 1. Redistributions of source code must retain the above
> - *    copyright notice, this list of conditions and the
> - *    following disclaimer.
> - *
> - * 2. Redistributions in binary form must reproduce the above
> - *    copyright notice, this list of conditions and the following
> - *    disclaimer in the documentation and/or other materials
> - *    provided with the distribution.
> + * SPDX-License-Identifier: BSD-2-Clause
>   *
> - * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
> - * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> - * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> - * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> - * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> - * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> - * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> - * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> - * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> - * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> - * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> - * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> - * SUCH DAMAGE.
> + * Copyright 2021, Tarantool AUTHORS, please see AUTHORS file.
>   */
>
> +#include <limits.h>
> +#include <assert.h>
> +
>  #include "mp_datetime.h"
>  #include "msgpuck.h"
>  #include "mp_extension_types.h"
> @@ -37,9 +15,9 @@
>    Datetime MessagePack serialization schema is MP_EXT (0xC7 for 1 
> byte length)
>    extension, which creates container of 1 to 3 integers.
>
> - 
> +----+---+-----------+====~~~~~~~====+-----~~~~~~~~-------+....~~~~~~~....+
> -  |0xC7| 4 |len (uint8)| seconds (int) | nanoseconds (uint) | offset 
> (uint) |
> - 
> +----+---+-----------+====~~~~~~~====+-----~~~~~~~~-------+....~~~~~~~....+
> + 
> +----+-----------+---+====~~~~~~~====+-----~~~~~~~~-------+....~~~~~~~....+
> +  |0xC7|len (uint8)| 4 | seconds (int) | nanoseconds (uint) | offset 
> (int)  |
> + 
> +----+-----------+---+====~~~~~~~====+-----~~~~~~~~-------+....~~~~~~~....+
>
>    MessagePack extension MP_EXT (0xC7), after 1-byte length, contains:
>
> @@ -50,7 +28,7 @@
>      If this value is 0 then it's not saved (unless there is offset 
> field,
>      as below);
>
> -  - [optional] timzeone offset in minutes as unsigned integer.
> +  - [optional] timezone offset in minutes as signed integer.
>      If this field is 0 then it's not saved.
>   */
>
> @@ -80,17 +58,34 @@ mp_decode_Xint(const char **data)
>      return 0;
>  }
>
> +#define check_secs(secs)                                \
> +    assert((int64_t)(secs) <= MAX_EPOCH_SECS_VALUE);\
> +    assert((int64_t)(secs) >= MIN_EPOCH_SECS_VALUE);
> +
> +#define check_nanosecs(nsec)      assert((nsec) < 1000000000);
> +
> +#define check_tz_offset(offset)       \
> +    assert((offset) <= (12 * 60));\
> +    assert((offset) >= (-12 * 60));
> +
>  static inline uint32_t
>  mp_sizeof_datetime_raw(const struct datetime *date)
>  {
> +    check_secs(date->secs);
>      uint32_t sz = mp_sizeof_Xint(date->secs);
>
> -    // even if nanosecs == 0 we need to output anything
> -    // if we have non-null tz offset
> -    if (date->nsec != 0 || date->offset != 0)
> +    /*
> +     * even if nanosecs == 0 we need to output something
> +     * if we have a non-null tz offset
> +     */
> +    if (date->nsec != 0 || date->offset != 0) {
> +        check_nanosecs(date->nsec);
>          sz += mp_sizeof_Xint(date->nsec);
> -    if (date->offset)
> +    }
> +    if (date->offset) {
> +        check_tz_offset(date->offset);
>          sz += mp_sizeof_Xint(date->offset);
> +    }
>      return sz;
>  }
>
> @@ -103,24 +98,30 @@ mp_sizeof_datetime(const struct datetime *date)
>  struct datetime *
>  datetime_unpack(const char **data, uint32_t len, struct datetime *date)
>  {
> -    const char * svp = *data;
> +    const char *svp = *data;
>
>      memset(date, 0, sizeof(*date));
>
> -    date->secs = mp_decode_Xint(data);
> +    int64_t seconds = mp_decode_Xint(data);
> +    check_secs(seconds);
> +    date->secs = seconds;
>
>      len -= *data - svp;
>      if (len <= 0)
>          return date;
>
>      svp = *data;
> -    date->nsec = mp_decode_Xint(data);
> +    uint64_t nanoseconds = mp_decode_uint(data);
> +    check_nanosecs(nanoseconds);
> +    date->nsec = nanoseconds;
>      len -= *data - svp;
>
>      if (len <= 0)
>          return date;
>
> -    date->offset = mp_decode_Xint(data);
> +    int64_t offset = mp_decode_Xint(data);
> +    check_tz_offset(offset);
> +    date->offset = offset;
>
>      return date;
>  }
> @@ -131,10 +132,12 @@ mp_decode_datetime(const char **data, struct 
> datetime *date)
>      if (mp_typeof(**data) != MP_EXT)
>          return NULL;
>
> +    const char *svp = *data;
>      int8_t type;
>      uint32_t len = mp_decode_extl(data, &type);
>
>      if (type != MP_DATETIME || len == 0) {
> +        *data = svp;
>          return NULL;
>      }
>      return datetime_unpack(data, len, date);
> @@ -145,7 +148,7 @@ datetime_pack(char *data, const struct datetime 
> *date)
>  {
>      data = mp_encode_Xint(data, date->secs);
>      if (date->nsec != 0 || date->offset != 0)
> -        data = mp_encode_Xint(data, date->nsec);
> +        data = mp_encode_uint(data, date->nsec);
>      if (date->offset)
>          data = mp_encode_Xint(data, date->offset);
>
> @@ -165,7 +168,9 @@ mp_encode_datetime(char *data, const struct 
> datetime *date)
>  int
>  mp_snprint_datetime(char *buf, int size, const char **data, uint32_t 
> len)
>  {
> -    struct datetime date = {0, 0, 0};
> +    struct datetime date = {
> +        .secs = 0, .nsec = 0, .offset = 0
> +    };
>
>      if (datetime_unpack(data, len, &date) == NULL)
>          return -1;
> @@ -176,7 +181,9 @@ mp_snprint_datetime(char *buf, int size, const 
> char **data, uint32_t len)
>  int
>  mp_fprint_datetime(FILE *file, const char **data, uint32_t len)
>  {
> -    struct datetime date = {0, 0, 0};
> +    struct datetime date = {
> +        .secs = 0, .nsec = 0, .offset = 0
> +    };
>
>      if (datetime_unpack(data, len, &date) == NULL)
>          return -1;
> diff --git a/src/lib/core/mp_datetime.h b/src/lib/core/mp_datetime.h
> index 9a4d2720c..92e94a243 100644
> --- a/src/lib/core/mp_datetime.h
> +++ b/src/lib/core/mp_datetime.h
> @@ -1,33 +1,8 @@
>  #pragma once
>  /*
> - * Copyright 2021, Tarantool AUTHORS, please see AUTHORS file.
> - *
> - * Redistribution and use in source and binary forms, with or
> - * without modification, are permitted provided that the following
> - * conditions are met:
> - *
> - * 1. Redistributions of source code must retain the above
> - *    copyright notice, this list of conditions and the
> - *    following disclaimer.
> + * SPDX-License-Identifier: BSD-2-Clause
>   *
> - * 2. Redistributions in binary form must reproduce the above
> - *    copyright notice, this list of conditions and the following
> - *    disclaimer in the documentation and/or other materials
> - *    provided with the distribution.
> - *
> - * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
> - * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> - * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> - * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> - * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> - * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> - * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> - * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> - * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> - * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> - * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> - * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> - * SUCH DAMAGE.
> + * Copyright 2021, Tarantool AUTHORS, please see AUTHORS file.
>   */
>
>  #include <stdio.h>
>
> -----------------------------------------------------
>
> Thanks,
> Timur

Thanks for the changes!

-- 
Serge Petrenko