On 12.01.2016 13:09, Michael Widenius wrote:
"Oleksandr" == Oleksandr Byelkin <sanja@montyprogram.com> writes: Oleksandr> Hi! Oleksandr> Here is full diff of the task (will be tested more when georg made it in Oleksandr> the connectors tree)
Oleksandr> Also it can be reviewed by parts in bb-sanja-10.2 branch on github.
Oleksandr> Connectors part is on 10.2-georg in connectors tree.
diff --git a/include/mysql_com.h b/include/mysql_com.h index 57092dc..425559f 100644 --- a/include/mysql_com.h +++ b/include/mysql_com.h @@ -102,7 +102,9 @@ enum enum_server_command COM_STMT_PREPARE, COM_STMT_EXECUTE, COM_STMT_SEND_LONG_DATA, COM_STMT_CLOSE, COM_STMT_RESET, COM_SET_OPTION, COM_STMT_FETCH, COM_DAEMON, /* don't forget to update const char *command_name[] in sql_parse.cc */ - + COM_MDB_GAP_BEG, + COM_MDB_GAP_END=254, + COM_MULTI, /* Must be last */ COM_END };
Change COM_MDB_GAP_END to be 253, to ensure that COM_END is 255. (Better to keep everything in one byte)
Also add an compiler error check that COM_END is 255 (to ensure that one doesn't accidently add a new define without changing COM_DB_GAP_END. OK!
@@ -234,6 +236,8 @@ enum enum_server_command #define MARIADB_CLIENT_FLAGS_MASK 0xffffffff00000000ULL
Haven't seen the above define before and it's not in the current 10.1 code. ah, it was removed wit client code, so I'll remove it if it is not used.
/* Client support progress indicator */ #define MARIADB_CLIENT_PROGRESS (1ULL << 32) +/* support COM_MULTI */ +#define MARIADB_CLIENT_COM_MULTI (1ULL << 33) /* Client support mariaDB extended flags */ #define MARIADB_CLIENT_EXTENDED_FLAGS (1ULL << 63)
@@ -268,7 +272,8 @@ enum enum_server_command MARIADB_CLIENT_PROGRESS | \ CLIENT_PLUGIN_AUTH | \ CLIENT_PLUGIN_AUTH_LENENC_CLIENT_DATA | \ - CLIENT_CONNECT_ATTRS) + CLIENT_CONNECT_ATTRS |\ + MARIADB_CLIENT_COM_MULTI)
You don't have MARIADB_CLIENT_COM_MULTI anywhere else in the patch. Is that intententional ?
If yes, you should add a comment that this is used in the LGPL C driver code so that anyone examening the server code will not get confused.
As you have added flags bigger than 32, you should also change THD::client_capabilities from ulong to ulonglong to reflect this!
This patch doesn't include changes in sql_acl.cc to send the MARIADB_CLIENT_COM_MULTI flag to the client. Where are the necessary changes for this ? expanding in to 64bit was separate task (reviewed and pushed).
Also we are not going to support this new features in old client, so we are signaling our connector only. I'll write a comment about it.
diff --git a/sql/sql_class.cc b/sql/sql_class.cc index b496e4c..54884c0 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -856,6 +856,7 @@ THD::THD(bool is_wsrep_applier) /* statement id */ 0), rli_fake(0), rgi_fake(0), rgi_slave(NULL), protocol_text(this), protocol_binary(this), + last_stmt(NULL), in_sub_stmt(0), log_all_errors(0), binlog_unsafe_warning_flags(0), binlog_table_maps(0),
You don't have to set last_stmt(NULL) in THD::THD() as this function calls init() which sets this too.
ok
@@ -1424,6 +1425,7 @@ void THD::init(void) bzero((char *) &org_status_var, sizeof(org_status_var)); start_bytes_received= 0; last_commit_gtid.seq_no= 0; + last_stmt= NULL; status_in_global= 0; #ifdef WITH_WSREP wsrep_exec_mode= wsrep_applier ? REPL_RECV : LOCAL_STATE;
diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc @@ -884,6 +1115,23 @@ void cleanup_items(Item *item) DBUG_VOID_RETURN; }
+static enum enum_server_command fetch_command(THD *thd, char *packet) +{ + enum enum_server_command + command= (enum enum_server_command) (uchar) packet[0]; + NET *net= &thd->net; + DBUG_ENTER("fetch_command"); +
If command is 256, then the following first test can never be true and you may get a warning from gcc. Another reason to have COM_END as 255.
yes
+ if (command >= COM_END || + (command >= COM_MDB_GAP_BEG && command <= COM_MDB_GAP_END)) + command= COM_END; // Wrong command + + DBUG_PRINT("info",("Command on %s = %d (%s)", + vio_description(net->vio), command, + command_name[command].str)); + DBUG_RETURN(command); +} +
/** + check COM_MULTI packet + + @param thd thread handle + @param packet pointer on the packet of commands + @param packet_length length of this packet + + @retval TRUE - Error + @retval FALSE - OK +*/ + +bool maria_multi_check(THD *thd, char *packet, uint packet_length) +{ + DBUG_ENTER("maria_multi_check"); + while (packet_length) + {
You need to check for packet_length >= 3 and give the my_message(ER_UNKNOWN_COM_ERROR... error if packet_length > 1 and <= 3
+ // length of command + 3 bytes where that length was stored + uint subpacket_length= (uint3korr(packet) + 3); + DBUG_PRINT("info", ("sub-packet length: %d command: %x", + subpacket_length, packet[3])); + + if (subpacket_length == 0 || + subpacket_length > packet_length)
Remove check of sub_packet_length == 0; It can never be 0 as you are adding +3 above.
ok
+ { + my_message(ER_UNKNOWN_COM_ERROR, ER_THD(thd, ER_UNKNOWN_COM_ERROR), + MYF(0)); + DBUG_RETURN(TRUE); + } + + packet+= subpacket_length; + packet_length-= subpacket_length; + }
+ DBUG_RETURN(FALSE); +}
@@ -1901,6 +2200,69 @@ bool dispatch_command(enum enum_server_command command, THD *thd, general_log_print(thd, command, NullS); my_eof(thd); break; + case COM_MULTI: + if (is_com_multi) + { + /* we do not allow deep recursion because it is not safe */ + my_message(ER_UNKNOWN_COM_ERROR, ER_THD(thd, ER_UNKNOWN_COM_ERROR), + MYF(0)); + break; + }
Give another error as the above one is missleading.
OK
+ if (!(thd->client_capabilities & CLIENT_MULTI_RESULTS)) + { + /* The client does not support multiple result sets being sent back */ + my_error(ER_COMMULTI_BADCONTEXT, MYF(0)); + break; + }
How can the above be possible ? We should not allow the client to set COM_MULTI flag if they don't have CLIENT_MULTI_RESULT. Better to remove the above check and document when the client can set the COM_MULTI flag.
I am not of control of client, it can be some home-made client so better to check.
+ if (maria_multi_check(thd, packet, packet_length)) + break; + { + /* We have to store next length because it will be destroyed by '\0' */ + uint next_subpacket_length= uint3korr(packet); + unsigned char *readbuff= net->buff; + unsigned long readbuff_max_packet= net->max_packet; +
Remove the above one extra empty line. Add also some documentation why we are changing net->buff here instead of creating another buffer and using this instead.
Actually it's much better to create a new buffer for packet becasue of:
- The buffer only need to be of size packet_length, not net->max_packet which can be much longer. - Clearer code as you don't have to manipulate net internal structures or restore these. - The disadvantage is that you can't allow COM_CHANGE_USER, but I don't think one should be able to do that anyway as this may ask for more information from the client.
OK
+ if (!(net->buff=(uchar*) my_malloc((size_t) net->max_packet+ + NET_HEADER_SIZE + COMP_HEADER_SIZE +1, + MYF(MY_WME)))) + break; + net->buff_end=net->buff + net->max_packet; + net->write_pos=net->read_pos = net->buff; + + while(packet_length)
space after while
+ { + uint subpacket_length= next_subpacket_length;
Why not do as in other code and make the rest of the code easier: subpacket_length= next_subpacket_length +3 ;
ok
+ if (subpacket_length + 3 < packet_length) + next_subpacket_length= uint3korr(packet + subpacket_length + 3); + /* safety like in do_command() */ + packet[subpacket_length + 3]= '\0'; + + enum enum_server_command subcommand= fetch_command(thd, (packet + 3)); +
I would almost prefer that we check for sub_commend == COM_MULTI here instead of at start of COM_COMMAND: This would be more logical as we may not allow all commands here (like change user).
OK
+ if (is_com_multi) + { + /* we do not allow deep recursion because it is not safe */ + my_message(ER_UNKNOWN_COM_ERROR, ER_THD(thd, ER_UNKNOWN_COM_ERROR), + MYF(0)); + break; + }
+ if (dispatch_command(subcommand, thd, packet + 4, + subpacket_length - 1, TRUE))
Add an assert that is_error() is set if we do a break. (As otherwise you can miss to execute commands the client expects)
OK
+ break; + + DBUG_ASSERT(subpacket_length + 3 <= packet_length); + packet+= (subpacket_length + 3); + packet_length-= (subpacket_length + 3);
+ }
I don't think you need a net_flush here as the packet is flushed by every command the sent a result.
OK
+ net_flush(net); + my_free(net->buff); + net->buff= readbuff; + net->max_packet= readbuff_max_packet; + net->buff_end=net->buff + net->max_packet; + net->write_pos=net->read_pos = net->buff; +
If we use an internal buffer, you can remoev all of the above (except the free)
OK
+ if (!thd->is_error()) + { + thd->get_stmt_da()->reset_diagnostics_area(); + my_ok(thd); + } + } + break;
case COM_SLEEP: case COM_CONNECT: // Impossible here case COM_TIME: // Impossible from client @@ -1940,6 +2302,8 @@ bool dispatch_command(enum enum_server_command command, THD *thd, thd->protocol->end_statement(); query_cache_end_of_result(thd); } + if (drop_more_results) + thd->server_status&= ~SERVER_MORE_RESULTS_EXISTS;
Another way, is to just remember the status of server_status and do:
thd->server_status= ((thd->server_status & ~SERVER_MORE_RESULTS_EXISTS) | (org_server_status & SERVER_MORE_RESULTS_EXISTS));
Easier to understand than drop_more_results
I am not sure that execution can't legally change setver status (if not now then in future)
--- a/sql/sql_prepare.cc +++ b/sql/sql_prepare.cc @@ -324,7 +324,9 @@ find_prepared_statement(THD *thd, ulong id) prepared statements find() will return 0 if there is a named prepared statement with such id. */ - Statement *stmt= thd->stmt_map.find(id); + Statement *stmt= ((id == LAST_STMT_ID) ? + thd->last_stmt : + thd->stmt_map.find(id));
Please add a comment when id can be LAST_STMT_ID (clear to me but not obivous for others)
if (stmt == 0 || stmt->type() != Query_arena::PREPARED_STATEMENT) return NULL;
--- a/storage/perfschema/pfs.cc +++ b/storage/perfschema/pfs.cc @@ -1444,19 +1444,21 @@ static void register_statement_v1(const char *category,
for (; count>0; count--, info++) { - DBUG_ASSERT(info->m_name != NULL); - len= strlen(info->m_name); - full_length= prefix_length + len; - if (likely(full_length <= PFS_MAX_INFO_NAME_LENGTH)) + if(info->m_name != NULL)
Add space after if
Instead of doing an if, and cause a big diff, do instead:
if (!info->m_name) continue;
This will remove all of the following in the diff:
OK
{ - memcpy(formatted_name + prefix_length, info->m_name, len); - info->m_key= register_statement_class(formatted_name, full_length, info->m_flags); - } - else - { - pfs_print_error("register_statement_v1: name too long <%s>\n", - info->m_name); - info->m_key= 0; + len= strlen(info->m_name); + full_length= prefix_length + len; + if (likely(full_length <= PFS_MAX_INFO_NAME_LENGTH)) + { + memcpy(formatted_name + prefix_length, info->m_name, len); + info->m_key= register_statement_class(formatted_name, full_length, info->m_flags); + } + else + { + pfs_print_error("register_statement_v1: name too long <%s>\n", + info->m_name); + info->m_key= 0; + } } } return;
Regards, Monty