← Back to team overview

maria-developers team mailing list archive

Re: Please review MDEV-17832 Protocol: extensions for Pluggable types and JSON, GEOMETRY

 

Hi Georg,

Thanks for reviewing!

See comments inline:

On 2/26/20 7:58 PM, Georg Richter wrote:
Hi Bar,

2 things I'd like to change:

+typedef struct st_mariadb_metadata_string
+{
+  const char *str;
+  size_t length;
+} MARIADB_FIELD_METADATA_STRING;

We already have MYSQL_LEX_STRING for dynamic columns and MARIADB_STRING for replication API. I think we should just move MARIADB_STRING from mariadb_rpl.h to mysql.h and use it instead.

These structures do not work. They use a "char *" pointer:

struct st_mysql_lex_string
{
  char *str;
  size_t length;
};
typedef struct st_mysql_lex_string MYSQL_LEX_STRING;
typedef struct st_mysql_lex_string LEX_STRING;

typedef struct {
  char *str;
  size_t length;
} MARIADB_STRING;


While I need a "const char *" pointer.

Perhaps we could add a new structure MARIADB_CSTRING as follows:


typedef struct {
  const char *str;
  size_t length;
} MARIADB_CSTRING;


Btw, possibly in many cases we'll be able to change
MARIADB_STRING / MYSQL_LEX_STRING to MARIADB_CSTRING.



+
+
+MARIADB_FIELD_METADATA_STRING
+  mariadb_field_metadata_attr(const MYSQL_FIELD *field,
+                              enum mariadb_field_metadata_attr_t type);

I think we should have a more general function, so that we don't need to add a new function when> We have a similiar function in mariadb_lib.c: mariadb_get_infov():

we need to extend MYSQL_FIELD (but it can be used also for retrieving values from other members of MYSQL_FIELD):

enum mariadb_field_info_type {
   FIELD_INFO_DATA_TYPE_NAME,
   FIELD_INFO_DATA_TYPE_FORMAT
   /* Later I can add
   FIELD_INFO_CATALOG,
   FIELD_INFO_TABLE,
   ..
   */
}

my_bool STDCALL mariadb_get_field_info(const MYSQL_FIELD *field,
                                        enum mariadb_field_info_type,
                                        void *arg, ..)

> We have a similiar function in mariadb_lib.c: mariadb_get_infov():

Sounds fine.

I have one concern though: wouldn't it be confusing that
mariadb_get_field_info() return strings as "char*",
while mariadb_get_field_info() will return strings
as MARIADB_CSTRING?


Also the function needs to be added to the MARIADB_SYMBOLS in libmariadb/CMakeLists.txt

Thanks. Will do.



Otherwise it looks ok!

/Georg

On Wed, Feb 26, 2020 at 3:40 PM Alexander Barkov <bar@xxxxxxxxxxx <mailto:bar@xxxxxxxxxxx>> wrote:

    Hi, Sergei, Georg,

    Please review a fixed version of the patch for MDEV-17832.

    There are two files attached:
    - mdev-17821.v18.diff     (server changes)
    - mdev-17821-cli.v06.diff (libmariadb changes)


    Comparing to the previous version, this version:


    1. Adds a new structure MA_FIELD_EXTENSION

    2. Moves extended data type information from MYSQL_FIELD
         to MYSQL_FIELD::extension in the client-server implementation.

         Note, in case of embedded server, the extended metadata
         is stored directly to MYSQL_FIELD.

    3. Adds a new API function mariadb_field_metadata_attr(),
         to extract metadata from MYSQL_FIELD.


    4. Changes the way how the metadata is packed on the wire
         from "easily human readable" to "easily parse-able", which:
         - makes the things faster
         - allows to transfer arbitrary binary data in the future, if
    needed.

    Every metadata chunk is now encoded as:

    a. chunk type (1 byte)
    b. chunk data length (1 byte)
    c. chunk data (according to #b)

    For now, two chunk types are implemented:
    - data type name (used for GEOMETRY sub-types, and for INET6)
    - format name (for JSON)


    Thanks!



--
Georg Richter, Senior Software Engineer
MariaDB Corporation Ab


References