Merge lp:~sergei.glushchenko/percona-server/ST30462-bug925343-5.5 into lp:percona-server/5.5

[Sergei Glushchenko]

Bug 925343: mysql client aborts connection on terminal resize.
Fix for upstream bug #26780 "patch to add auto vertical output
option to the cli" introduced SIGWINCH handling by mysql cli.
This leads to EINTR to be returned by read call on socket.
On most systems there is a possibility for read call
to be automatically restarted if SA_RESTART flag is set for
the signal. Hovewer this is not the case, as read timeout on
socket has been set. Linux manual page for signal(7) tells that
there is no chance for read to be restarted if timeout has
been set on socket. It doesn't matter whether SA_RESTART has
been used for signal or not.
So the only portable solution would be to block signal for the
time when client communicates with server. This however would
require a lot of carefull code reading to spot all the point
of such communication in source code and wrap them with
block/unblock signal.
That is why option to completely disable SIGWINCH handling
has been choosen. The only purpose of SIGWINCH handler was to
determine new width of terminal window. The only place where
this width is used is to print output from the server.
Handling of SIGWINCH has been replaced with explicit invocation
of ioctl to get terminal width just before print of result set
is started.

http://jenkins.percona.com/view/PS%205.5/job/percona-server-5.5-param/717/

[Stewart Smith (stewart)]

review: Approve

[Alexey Kopytov (akopytov)]

Sergei,

I don't understand this fix. It is correct that a socket I/O operation is not automatically restarted if socket timeouts are used and when the operation is interrupted by a signal handler. However, we don't use SA_RESTART, so the network code does not rely on that behavior. Otherwise the server might crash on signals as well.

Furthermore, the fix removes the mysql.cc signal handler, but the comments suggest that handler will be called from the _readline_ handler. It is also described in the readline docs:

"Readline contains an internal signal handler that is installed for a number of signals (SIGINT, SIGQUIT, SIGTERM, SIGALRM, SIGTSTP, SIGTTIN, and SIGTTOU). When one of these signals is received, the signal handler will reset the terminal attributes to those that were in effect before readline() was called, reset the signal handling to what it was before readline() was called, and resend the signal to the calling application.
"

So:

- what was really the reason of the crash. Revision comments say nothing on that.
- even if we remove mysql's own signal handler for SIGWINCH, there are other signal handlers installed by both mysql.cc and readline. Following the explanation form the revision comments, those may also result in a crash?

review: Needs Information

[Sergei Glushchenko (sergei.glushchenko)]

Alexey,

Here is a short summary.

1. There is no crash, but just an aborted connection with server.
2. Bug can be reproduced very easy. Just
   - start mysql
   - send select sleep(100);
   - resize terminal window (if you are in Gnome and using Gnome Terminal or other terminal emulator with resizable window) or just execute `killall -WINCH mysql`
   - query execution will be aborted and you will see 'ERROR 2013 (HY000): Lost connection to MySQL server during query'
3. Bug cannot be reproduced on every UNIX system. Even not on every Linux system. I can repeat it on Ubuntu Linux 11.04 (64bit). Cannot repeat on CentOS 6.3 (32bit). As strace shows, on CentOS socket is configured the same way as on Ubuntu Linux, but instead of EINTR I can see ERESTARTSYS and automatic restart of read.
4. Readline does take care of cleaning up all signal headers after readline() call finished it's job.
5. net_serv.cc doesn't restart interrupted reads. It did on 5.1, there was an option to produce thread-safe client and it was turned on by default (--disable-thread-safe-client). But 5.5 and 5.6 as well don't turn on this option and don't define THREAD_SAFE_CLIENT. (Possibly it shoul be reported as upstream bug, as there is dead code in client or incorrect behaviour)
6. mysql itself handles INT and QUIT.
Behavior of mysql in this case is very similar:
mysql> select sleep(20);
^CCtrl-C -- sending "KILL QUERY 21" to server ...
Ctrl-C -- query aborted.
ERROR 2013 (HY000): Lost connection to MySQL server during query
Eventually the connection with the server is closed, and gdb shows exactly the same code path as for SIGWINCH.

Thanks,
Sergei

[Alexey Kopytov (akopytov)]

After an IRC discussion I agree that getting rid of the signal handler is the most simple way to fix the problem. However, there is still one issue with the patch:

previously that ioctl() to get the terminal width was only called on SIGWINCH. Now we are going to call it unconditionally on every command, even if the option that requires it (i.e. auto_vertical_output) is disabled. Consider a case when mysql is used to import a large dump file.

What about doing the following instead?

--- client/mysql.cc 2012-12-12 17:01:03 +0000
+++ client/mysql.cc 2013-04-04 09:11:44 +0000
@@ -3074,7 +3074,7 @@
    print_table_data_html(result);
  else if (opt_xml)
    print_table_data_xml(result);
- else if (vertical || (auto_vertical_output && (terminal_width < get_result_width(result))))
+ else if (vertical || (auto_vertical_output && (get_terminal_width() < get_result_width(result))))
    print_table_data_vertically(result);
  else if (opt_silent && verbose <= 2 && !output_tables)
    print_tab_data(result);

review: Needs Fixing

[Sergei Glushchenko (sergei.glushchenko)]

Alexey,

Thanks for the nice remark. I've updated the branch and tested it locally. I started Jenkins build just now with the hope that it will not show any surprises.

http://jenkins.percona.com/view/PS%205.5/job/percona-server-5.5-param/720/

Same goes to 5.6 version of MP.

Thanks,
Sergei

[Alexey Kopytov (akopytov)]

review: Approve

=== modified file 'Percona-Server/client/mysql.cc'
--- Percona-Server/client/mysql.cc	2013-03-22 03:29:56 +0000
+++ Percona-Server/client/mysql.cc	2013-04-04 09:36:24 +0000
@@ -193,7 +193,6 @@
 static uint prompt_counter;
 static char delimiter[16]= DEFAULT_DELIMITER;
 static uint delimiter_length= 1;
-unsigned short terminal_width= 80;
 
 #ifdef HAVE_SMEM
 static char *shared_memory_base_name=0;
@@ -1073,9 +1072,7 @@
 static void nice_time(double sec,char *buff,bool part_second);
 extern "C" sig_handler mysql_end(int sig);
 extern "C" sig_handler handle_sigint(int sig);
-#if defined(HAVE_TERMIOS_H) && defined(GWINSZ_IN_SYS_IOCTL)
-static sig_handler window_resize(int sig);
-#endif
+static unsigned short get_terminal_width();
 
 
 int main(int argc,char *argv[])
@@ -1175,13 +1172,6 @@
     signal(SIGINT, handle_sigint);              // Catch SIGINT to clean up
   signal(SIGQUIT, mysql_end);			// Catch SIGQUIT to clean up
 
-#if defined(HAVE_TERMIOS_H) && defined(GWINSZ_IN_SYS_IOCTL)
-  /* Readline will call this if it installs a handler */
-  signal(SIGWINCH, window_resize);
-  /* call the SIGWINCH handler to get the default term width */
-  window_resize(0);
-#endif
-
   put_info("Welcome to the MySQL monitor.  Commands end with ; or \\g.",
 	   INFO_INFO);
   sprintf((char*) glob_buffer.ptr(),
@@ -1354,15 +1344,16 @@
 }
 
 
+unsigned short get_terminal_width()
+{
 #if defined(HAVE_TERMIOS_H) && defined(GWINSZ_IN_SYS_IOCTL)
-sig_handler window_resize(int sig)
-{
   struct winsize window_size;
 
   if (ioctl(fileno(stdin), TIOCGWINSZ, &window_size) == 0)
-    terminal_width= window_size.ws_col;
+    return window_size.ws_col;
+#endif
+  return 80;
 }
-#endif
 
 static struct my_option my_long_options[] =
 {
@@ -3134,7 +3125,7 @@
 	  print_table_data_html(result);
 	else if (opt_xml)
 	  print_table_data_xml(result);
-  else if (vertical || (auto_vertical_output && (terminal_width < get_result_width(result))))
+  else if (vertical || (auto_vertical_output && (get_terminal_width() < get_result_width(result))))
 	  print_table_data_vertically(result);
 	else if (opt_silent && verbose <= 2 && !output_tables)
 	  print_tab_data(result);