Implement CLIENT_DEPRECATE_EOF for mariadb-connector-c #131

bibstha · 2020-03-30T19:11:58Z

mariadb-connector-c client now sends CLIENT_DEPRECATE_EOF flag to the server.

There are some major changes on how the server responds. Mainly the way packets are sent back from server changes which are documented in https://dev.mysql.com/worklog/task/?id=7766

I will try to add as much comment on the code changes as possible.

Note:

Most of the changes have been studied on the worklog, looking into the packets through wireshark and how mysql client behaves.

bibstha · 2020-03-31T05:46:26Z

include/ma_common.h

@@ -23,6 +23,8 @@
 #include <mysql.h>
 #include <ma_hash.h>

+#define MAX_PACKET_LENGTH (256L*256L*256L-1)


Moved from ma_net.c from below: https://github.com/mariadb-corporation/mariadb-connector-c/pull/131/files#diff-4ad0e2e8b53cd72422b2c0863655c5e5L42 since it is required in mariadb_lib.c

bibstha · 2020-03-31T05:56:49Z

libmariadb/mariadb_lib.c

@@ -188,11 +189,14 @@ void net_get_error(char *buf, size_t buf_len,
 *****************************************************************************/

 ulong
-ma_net_safe_read(MYSQL *mysql)
+ma_net_safe_read(MYSQL *mysql, my_bool *is_data_packet)


Similar to https://github.com/mysql/mysql-server/blob/bd9c260cf7026a54d9bf91ce6782cdb4e6a25c71/sql-common/client.c#L1168

bibstha · 2020-03-31T06:05:02Z

libmariadb/mariadb_lib.c

+*/
+
+int
+unpack_field(const MYSQL *mysql, MA_MEM_ROOT *alloc, my_bool default_value,


This is extracted as is from unpack_fields function below so it can be called directly when reading packet metadata similar to mysql source code here

bibstha · 2020-03-31T06:11:53Z

libmariadb/mariadb_lib.c

+ * and is called by both
+ * mthd_my_read_query_result and mthd_stmt_read_prepare_response
+ */
+MYSQL_FIELD *mthd_my_read_metadata_ex(MYSQL *mysql, MA_MEM_ROOT *alloc,


Similar to mysql client

bibstha · 2020-03-31T06:13:09Z

libmariadb/mariadb_lib.c

+  return result;
+}
+
+MYSQL_FIELD *mthd_my_read_metadata(MYSQL *mysql, ulong field_count, uint field)


The reason mthd_my_read_metadata_ex and mthd_my_read_metadata are two methods: similar to how it is implemented in mysql client. mthd_my_read_metadata_ex takes MA_MEM_ROOT so it can be called from mthd_stmt_read_prepare_response

bibstha · 2020-03-31T06:18:54Z

libmariadb/mariadb_lib.c

@@ -4341,41 +4488,41 @@ struct st_mariadb_api MARIADB_API=
 */
 struct st_mariadb_methods MARIADB_DEFAULT_METHODS = {
  /* open a connection */
-  mthd_my_real_connect,
+  mthd_my_real_connect, // db_connect


Adding the actual names so it's easier to grep.

bibstha · 2020-03-31T06:24:20Z

libmariadb/mariadb_stmt.c

-      p+=2;
-      stmt->upsert_status.server_status= stmt->mysql->server_status= uint2korr(p);
+      /* save status info */
+      if (stmt->mysql->server_capabilities & CLIENT_DEPRECATE_EOF && !is_data_packet)


Ref mysql source

bibstha · 2020-03-31T06:26:37Z

libmariadb/mariadb_stmt.c

    }
-    if (packet_len < 8 && *pos == 254) /* EOF */
+    else


The else part is as is as before. Only the upper if conditional is different.

bibstha · 2020-03-31T06:28:11Z

libmariadb/mariadb_stmt.c

+
+      if (is_data_packet)
+      {
+        // TODO: <metadata><row(s)><OK> not supported right now


Note that I'm still unsure how to reproduce it here. Non of the existing tests capture it yet.

bibstha · 2020-03-31T06:29:30Z

libmariadb/mariadb_stmt.c

@@ -1869,6 +1925,48 @@ int stmt_read_execute_response(MYSQL_STMT *stmt)
  if (!stmt->mysql)
    return(1);

+  if ((mysql->server_capabilities & CLIENT_DEPRECATE_EOF))


bibstha · 2020-04-06T14:27:15Z

@9E0R9 do you have some time to look into this?

9EOR9 · 2020-04-15T04:00:36Z

Thanks for your pull request - I will review it next week.

9EOR9 · 2020-04-20T18:18:14Z

Can you please stash multiple commits into one commit? That makes reviewing much easier. Thans

With CLIENT_DEPRECATE_EOF, there are few changes how the server sends back the packets. The protocol changes are explained in Worklog https://dev.mysql.com/worklog/task/?id=7766

bibstha · 2020-04-20T18:32:50Z

Done, I squashed them all into a single one.

9EOR9 · 2020-04-26T04:21:32Z

From a technical standpoint the code looks more or less ok, but from legal it doesn't:

There is a lot of code (and even comments) just copied from Oracle/MySQL libmysql which is under GPL license and therefore not compatible with LGPL license of MariaDB Connector/C.

bibstha · 2020-04-28T15:03:55Z

@9E0R9 I see, I'll approach the code in a different way than mysql.

ioquatix · 2020-06-30T05:48:06Z

I'm playing around with this feature, what are the benefits of CLIENT_DEPRECATE_EOF?

bibstha · 2020-07-08T15:35:48Z

Hi @ioquatix,
The main benefit for CLIENT_DEPRECATE_EOF is that each mysql response now supports additional information in the form of OK_packets, ie: session tracking information. More info on session tracking info

SessionTracking is not just for pre-defined data, but you can write plugins to send back custom data too. At work, we're using this to send back performance metrics from inside the database.

PS: We also added support for session-tracking to mysql2 gem in brianmario/mysql2#1092 (merged into master, not yet released.)

vaintroub · 2020-07-08T15:49:19Z

The MySQL implementation was done by someone without much experience in MySQL protocol, and is even conceptually broken w.r.t old features. For example, the "stored procedure output parameters" from 5.5, is now broken because it is not possible to determine from the result set whether it is this special "output parameters" result set, from the metadata, until the whole result set is read. Because somebody wanted optimize out the 9 bytes in the result set metadata.

CLIENT_DEPRECATE_EOF decided to encode key-value in its own, unique way, dismissing the 20 years old notion of "result set" in the protocol and misused OK packet for this purpose. It was definitely a low quality design work by newly hired MySQL trainee by Oracle. Someone needs to stop these people from screwing the protocol.

bibstha · 2020-07-08T17:05:43Z

@vaintroub that's interesting insight regarding compatibility.

Given that is how the protocol works now, and most other connectors have it implemented, and without CLIENT_DEPRECATE_EOF, the connector cannot access session tracking information, do you see any other way forward?

vaintroub · 2020-07-08T18:31:18Z

I do not know about the good way forward. Luckily in this situation I'm almost a bystander/observer.

But the snarky remark I told, is the same I told maybe back in 2014, when MySQL trainee developers successfully conquered their first challenge of the protocol butchering.

What now?

We're debating to implement a MariaDB protocol extension, for our server, and our clients which would workaround what this CLIENT_DEPRECATE_EOF broke. We need extension for https://jira.mariadb.org/browse/MDEV-19237 anyway, so maybe we can include the status flags that newbie developers throw away in intermediate EOF (2 of the whooping 9 bytes they spared per result set)

I guess most of the client APIs are doing OK without that extra trailing stuff at the end of OK packet. They did it for decades, and they can work with servers not supporting it. There is no notion of "client change state notification" in any of standard database APIs I know of. This stuff is mostly special needs for proxies, I think

vaintroub · 2020-07-08T19:05:06Z

@bibstha , now what do you need this capability for, exactly? Can you replace it with something more well-formed, e.g a SELECT for a variable you're interested in, after the "COMMIT". In case you're concerned about network roundtrips, semicolon batching is your friend

bibstha · 2020-07-10T22:54:43Z

These projects:

both already support the CLIENT_DEPRECATE_EOF. Wouldn't it simplify therefore to also have support in the c-connector? Maybe a flag to enable disable the support on client side?

The primary use-case for us to have this support is to be able to read session-tracking data. We are very much interested to gather query-metadata (specially performance) and return it in-line (realtime) so we can monitor / build systems that analyze queries behavior. The best way to do this for now is to have it return in the OK_packet as a session tracking variable.

We haven't considered batching queries yet, that's something we can look into. But the larger issue is still how you guys decide to move forward with the protocol support.

There is no notion of "client change state notification" in any of standard database APIs I know of

We're debating to implement a MariaDB protocol extension

IMO having a way to send back metadata of a query is very very useful. If not session_tracking, while you guys discuss protocol extension, please consider someway of sending back the metadata.

9EOR9 · 2022-01-23T20:28:07Z

Closing PR - we will not implement DEPRECATE_EOF due to broken design (see CONC-461)

bibstha force-pushed the 3.1-client_deprecate_eof branch from 359860b to 1d6d824 Compare March 31, 2020 05:35

bibstha commented Mar 31, 2020

View reviewed changes

bibstha mentioned this pull request Apr 6, 2020

V2.3.1 client deprecate eof flag bibstha/mariadb-connector-c#4

Open

bibstha force-pushed the 3.1-client_deprecate_eof branch from 03fd847 to 0c23b1d Compare April 20, 2020 18:30

Implement CLIENT_DEPRECATE_EOF support

1bb5927

With CLIENT_DEPRECATE_EOF, there are few changes how the server sends back the packets. The protocol changes are explained in Worklog https://dev.mysql.com/worklog/task/?id=7766

bibstha force-pushed the 3.1-client_deprecate_eof branch from 0c23b1d to 1bb5927 Compare April 20, 2020 18:32

9EOR9 closed this Jan 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement CLIENT_DEPRECATE_EOF for mariadb-connector-c #131

Implement CLIENT_DEPRECATE_EOF for mariadb-connector-c #131

bibstha commented Mar 30, 2020 •

edited

bibstha Mar 31, 2020

bibstha Mar 31, 2020

bibstha Mar 31, 2020

bibstha Mar 31, 2020

bibstha Mar 31, 2020

bibstha Mar 31, 2020

bibstha Mar 31, 2020

bibstha Mar 31, 2020

bibstha Mar 31, 2020

bibstha Mar 31, 2020

bibstha commented Apr 6, 2020

9EOR9 commented Apr 15, 2020

9EOR9 commented Apr 20, 2020

bibstha commented Apr 20, 2020

9EOR9 commented Apr 26, 2020

bibstha commented Apr 28, 2020

ioquatix commented Jun 30, 2020

bibstha commented Jul 8, 2020

vaintroub commented Jul 8, 2020 •

edited

bibstha commented Jul 8, 2020

vaintroub commented Jul 8, 2020

vaintroub commented Jul 8, 2020

bibstha commented Jul 10, 2020 •

edited

9EOR9 commented Jan 23, 2022

Implement CLIENT_DEPRECATE_EOF for mariadb-connector-c #131

Implement CLIENT_DEPRECATE_EOF for mariadb-connector-c #131

Conversation

bibstha commented Mar 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bibstha commented Apr 6, 2020

9EOR9 commented Apr 15, 2020

9EOR9 commented Apr 20, 2020

bibstha commented Apr 20, 2020

9EOR9 commented Apr 26, 2020

bibstha commented Apr 28, 2020

ioquatix commented Jun 30, 2020

bibstha commented Jul 8, 2020

vaintroub commented Jul 8, 2020 • edited

bibstha commented Jul 8, 2020

vaintroub commented Jul 8, 2020

vaintroub commented Jul 8, 2020

bibstha commented Jul 10, 2020 • edited

9EOR9 commented Jan 23, 2022

bibstha commented Mar 30, 2020 •

edited

vaintroub commented Jul 8, 2020 •

edited

bibstha commented Jul 10, 2020 •

edited