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@mariadb.com <mailto:bar@mariadb.com>> 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