maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

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