maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Daniel!

The approach is fine, see some comments below

On Jul 23, Daniel Black wrote:
> revision-id: 1c7d4f9c06f (mariadb-10.6.1-78-g1c7d4f9c06f)
> parent(s): 71964c76fe0
> author: Daniel Black
> committer: Daniel Black
> timestamp: 2021-06-16 17:15:59 +1000
> message:
> 
> MDEV-25282 Auto-shutdown on idle when socket-activated
> 
> Adds max_idle_execution system variable with a UINT_MAX
> default value that corresponds to the time in seconds
> under which the mariadbd executable will run in an idle state
> with no connections.
> 
> Under systemd socket activiation this variable will get a 10 minute
> default value. This will enable a service to be activated on
> connection and fall back to a shutdown state after 10 minutes of
> no queries. The systemd socket activation can restart the service
> on the next connection transparently.
> 
> A global variable of server_last_activity is updated on
> accepted connections (where max_idle_execution < UINT_MAX)
> and when the connection count (standard or extra) is down
> to <= 1 to keep the number of updates on a single variable
> low (an not create another performance MDEV-21212 problem).
> 
> When the main accept loop times out on the max_idle_execution
> seconds, and then the server_last_activity is checked along
> with if current connection count (standard + extra) is 0
> (in case a recently started connection hasn't finished
> a query).
> 
> To make this neater, in main accept loop moved code to
> handle_new_socket_connection that encompases accepting a connection
> and the timeout mechanism has been separated too.
> 
> Changed when looping though possible connections, loop until
> the end of the connection list and hereby assume two connection can
> occur on the same poll/select call and both will be accepted.
> 
> diff --git a/mysql-test/main/max_idle_execution.test b/mysql-test/main/max_idle_execution.test
> new file mode 100644
> --- /dev/null
> +++ b/mysql-test/main/max_idle_execution.test
> @@ -0,0 +1,38 @@
> +--source include/not_embedded.inc
> +
> +--let $_server_id= `SELECT @@server_id`
> +--let $_expect_file_name= $MYSQLTEST_VARDIR/tmp/mysqld.$_server_id.expect
> +--exec echo "wait" > $_expect_file_name

there's no reason to exec echo, as you can write directly

--write_file $_expect_file_name
wait
EOF

> +disable_reconnect;
> +disconnect default;
> +
> +--echo 'allow server to time out in 10 seconds'
> +
> +--sleep 10

better wait_until_disconnected.inc
here and everywhere below

> +
> +--echo 'should have timed out'
> +# mtr weirdness
> +--replace_regex /[\/0-9]+.*//
> +--error 0,ER_SERVER_SHUTDOWN,ER_CONNECTION_KILLED,2002,2006,201
> +--connect fail_me,localhost,root

you won't need all that ^^^

> +
> +--exec echo "restart" > $_expect_file_name
> +--sleep 5
> +--connect con0,localhost,root,,test
> +
> +--source include/wait_until_connected_again.inc
> +SELECT VARIABLE_VALUE AS THREAD_CONNECTED FROM INFORMATION_SCHEMA.GLOBAL_STATUS WHERE VARIABLE_NAME='THREADS_CONNECTED';
> +SELECT 'we are back';
> +SELECT VARIABLE_VALUE < 10 AS UPTIME_LESS_THAN_10_SECONDS FROM INFORMATION_SCHEMA.GLOBAL_STATUS WHERE VARIABLE_NAME='UPTIME';
> +
> +--sleep 13
> +SELECT 'still here because the connection is open, but disconnecting now';
> +disconnect con0;
> +--sleep 5
> +
> +--connect con2,127.0.0.1,root,,test,$MASTER_EXTRA_PORT,
> +SELECT "extra connection just created: still here, only 5 seconds since last query";
> +
> +--sleep 13 
> +connect  default,localhost,root,,test,,;
> +SELECT 'active extra connection kept the server up';

This will be a fairly slow test. 46 seconds of waits.
May be you can cut away some? E.g. second test, whether a connection
keeps the server up, can reasonably be considered a subset of the third
test, whether an extra connection keeps the server up. So you can remove
the second test.

> diff --git a/mysql-test/main/mysqld--help.result b/mysql-test/main/mysqld--help.result
> --- a/mysql-test/main/mysqld--help.result
> +++ b/mysql-test/main/mysqld--help.result
> @@ -576,6 +576,10 @@ The following specify which files/extra groups are read (specified before remain
>   --max-error-count=# Max number of errors/warnings to store for a statement
>   --max-heap-table-size=# 
>   Don't allow creation of heap tables bigger than this
> + --max-idle-execution=# 
> + If no new connections or running queries within this time
> + (in seconds) shutdown the server (Automatically
> + configured unless set explicitly)
>   --max-join-size=#   Joins that are probably going to read more than
>   max_join_size records return an error
>   --max-length-for-sort-data=# 
> diff --git a/sql/handle_connections_win.cc b/sql/handle_connections_win.cc
> --- a/sql/handle_connections_win.cc
> +++ b/sql/handle_connections_win.cc
> @@ -593,7 +593,7 @@ void network_init_win()
>    }
>  }
>  
> -void handle_connections_win()
> +void handle_connections_win(Atomic_counter<uint> *connection_count)

why do you pass it as an argument? first, you always invoke
handle_connections_win with the same value of the argument, second
other global variables like server_last_activity are used directly.

>  {
>    int n_waits;
>  
> @@ -631,12 +631,26 @@ void handle_connections_win()
>    {
>      DBUG_ASSERT(wait_events.size() <= MAXIMUM_WAIT_OBJECTS);
>      DWORD idx = WaitForMultipleObjects((DWORD)wait_events.size(),
> -                                       wait_events.data(), FALSE, INFINITE);
> +                                       wait_events.data(), FALSE,
> +                                       max_idle_execution < UINT_MAX ?
> +                                         max_idle_execution * 1000 : INFINITE);
> -    DBUG_ASSERT((int)idx >= 0 && (int)idx < (int)wait_events.size());
> +    DBUG_ASSERT(idx == WAIT_TIMEOUT ||
> +                  ((int)idx >= 0 && (int)idx < (int)wait_events.size()));

did you test it on Windows?

>  
>      if (idx == SHUTDOWN_IDX)
>        break;
>  
> +    if (idx == WAIT_TIMEOUT)
> +    {
> +      if (*connection_count == 0 &&
> +          microsecond_interval_timer() > (server_last_activity + max_idle_execution * 1000000))
> +      {
> +        sql_print_information("max_idle_execution time reached starting shutdown");
> +        break;
> +      }
> +      continue;
> +    }
> +
>      all_listeners[idx - LISTENER_START_IDX]->completion_callback();
>    }
>  
> diff --git a/sql/mysqld.cc b/sql/mysqld.cc
> --- a/sql/mysqld.cc
> +++ b/sql/mysqld.cc
> @@ -474,7 +474,8 @@ ulong malloc_calls;
>  ulong specialflag=0;
>  ulong binlog_cache_use= 0, binlog_cache_disk_use= 0;
>  ulong binlog_stmt_cache_use= 0, binlog_stmt_cache_disk_use= 0;
> -ulong max_connections, max_connect_errors;
> +ulong max_connections, max_connect_errors, max_idle_execution;
> +ulonglong server_last_activity;
>  uint max_password_errors;
>  ulong extra_max_connections;
>  uint max_digest_length= 0;
> @@ -4093,6 +4094,13 @@ static int init_common_variables()
>      SYSVAR_AUTOSIZE(back_log, MY_MIN(900, (50 + max_connections / 5)));
>    }
> 
> +  /*
> +    max_idle_execution, defaults to 10mins under systemd socket activation,
> +    otherwise 136 years or so.
> +  */
> +  if (IS_SYSVAR_AUTOSIZE(&max_idle_execution))
> +    SYSVAR_AUTOSIZE(max_idle_execution, sd_listen_fds(0) ? 6000 : UINT_MAX);

is that really 10 minutes?

> +
>    unireg_init(opt_specialflag); /* Set up extern variabels */
>    if (!(my_default_lc_messages=
>          my_locale_by_name(lc_messages)))
> @@ -6064,17 +6072,60 @@ static void set_non_blocking_if_supported(MYSQL_SOCKET sock)
>  }
>  
>  
> +static void handle_socket_timeout()
> +{

please add a comment, explaining why a race condition is impossible
here, that is, why no other thread can increment connection_count or
extra_connection_count just after you saw them being zeros.

> +  if (connection_count == 0 && extra_connection_count == 0 &&
> +      microsecond_interval_timer() > (server_last_activity + max_idle_execution * 1000000))

* 1000000ULL ?

> +  {
> +    sql_print_information("max_idle_execution time reached starting shutdown");
> +    abort_loop= 1;
> +  }
> +}
> +
> +
> +static void handle_new_socket_connection(MYSQL_SOCKET sock)
> +{
> +  struct sockaddr_storage cAddr;
> +  uint error_count= 0;
> +
> +  for (uint retry= 0; retry < MAX_ACCEPT_RETRY; retry++)
> +  {
> +    size_socket length= sizeof(struct sockaddr_storage);
> +    MYSQL_SOCKET new_sock;
> +
> +    new_sock= mysql_socket_accept(key_socket_client_connection, sock,
> +                                  (struct sockaddr *)(&cAddr),
> +                                  &length);
> +    if (mysql_socket_getfd(new_sock) != INVALID_SOCKET)
> +      handle_accepted_socket(new_sock, sock);
> +    else if (socket_errno != SOCKET_EINTR && socket_errno != SOCKET_EAGAIN)
> +    {
> +      /*
> +        accept(2) failed on the listening port.
> +        There is not much details to report about the client,
> +        increment the server global status variable.
> +      */
> +      statistic_increment(connection_errors_accept, &LOCK_status);
> +      if ((error_count++ & 255) == 0) // This can happen often
> +        sql_perror("Error in accept");
> +      if (socket_errno == SOCKET_ENFILE || socket_errno == SOCKET_EMFILE)
> +        sleep(1); // Give other threads some time
> +      break;
> +    }
> +  }
> +}
> +
> +
>  void handle_connections_sockets()
>  {
> -  MYSQL_SOCKET sock= mysql_socket_invalid();
> +  MYSQL_SOCKET sock;
> -  uint error_count=0;
> -  struct sockaddr_storage cAddr;
>    int retval;
>  #ifdef HAVE_POLL
>    // for ip_sock, unix_sock and extra_ip_sock
>    Dynamic_array<struct pollfd> fds(PSI_INSTRUMENT_MEM);
>  #else
>    fd_set readFDs,clientFDs;
> +  struct timespec timeout;
>  #endif
>  
>    DBUG_ENTER("handle_connections_sockets");
> @@ -6099,6 +6150,7 @@ void handle_connections_sockets()
>    }
>  #endif
>  
> +  server_last_activity= microsecond_interval_timer();
>    sd_notify(0, "READY=1\n"
>              "STATUS=Taking your SQL requests now...\n");
>  
> @@ -6106,10 +6158,12 @@ void handle_connections_sockets()
>    while (!abort_loop)
>    {
>  #ifdef HAVE_POLL
> -    retval= poll(fds.get_pos(0), fds.size(), -1);
> +    /* poll timeout in milliseconds */
> +    retval= poll(fds.get_pos(0), fds.size(), max_idle_execution * 1000);

max value of max_idle_execution is UINT_MAX. The third argument of
poll() is int. Although it seems to "work", that is, you get a negative
number which poll() interprets as infinite, I seriously suspect it's a UB.

>  #else
> +    timeout= { max_idle_execution, 0};
>      readFDs=clientFDs;
> -    retval= select(FD_SETSIZE, &readFDs, NULL, NULL, NULL);
> +    retval= select(FD_SETSIZE, &readFDs, NULL, NULL, &timeout);
>  #endif
>  
>      if (retval < 0)
> @@ -6132,50 +6186,27 @@ void handle_connections_sockets()
>        break;
>  
>      /* Is this a new connection request ? */
> -#ifdef HAVE_POLL
> -    for (size_t i= 0; i < fds.size(); ++i)
> +    sock= mysql_socket_invalid();
> +    for (size_t i= 0; i < listen_sockets.size(); i++)
>      {
> +#ifdef HAVE_POLL
>        if (fds.at(i).revents & POLLIN)
> -      {
> -        sock= listen_sockets.at(i);
> -        break;
> -      }
> -    }
>  #else  // HAVE_POLL
> -    for (size_t i=0; i < listen_sockets.size(); i++)
> -    {
>        if (FD_ISSET(mysql_socket_getfd(listen_sockets.at(i)), &readFDs))
> +#endif // HAVE_POLL
>        {
>          sock= listen_sockets.at(i);
> -        break;
> +        handle_new_socket_connection(sock);
>        }
>      }
> -#endif // HAVE_POLL
> -
> -    for (uint retry=0; retry < MAX_ACCEPT_RETRY; retry++)
> +    /* timeout */
> +    if (mysql_socket_getfd(sock) == INVALID_SOCKET)
>      {
> -      size_socket length= sizeof(struct sockaddr_storage);
> -      MYSQL_SOCKET new_sock;
> -
> -      new_sock= mysql_socket_accept(key_socket_client_connection, sock,
> -                                    (struct sockaddr *)(&cAddr),
> -                                    &length);
> -      if (mysql_socket_getfd(new_sock) != INVALID_SOCKET)
> -        handle_accepted_socket(new_sock, sock);
> -      else if (socket_errno != SOCKET_EINTR && socket_errno != SOCKET_EAGAIN)
> -      {
> -        /*
> -          accept(2) failed on the listening port.
> -          There is not much details to report about the client,
> -          increment the server global status variable.
> -        */
> -        statistic_increment(connection_errors_accept, &LOCK_status);
> -        if ((error_count++ & 255) == 0) // This can happen often
> -          sql_perror("Error in accept");
> -        if (socket_errno == SOCKET_ENFILE || socket_errno == SOCKET_EMFILE)
> -          sleep(1); // Give other threads some time
> -        break;
> -      }
> +      handle_socket_timeout();
> +    }
> +    else if (max_idle_execution <  UINT_MAX)
> +    {
> +      server_last_activity= microsecond_interval_timer();
>      }
>    }
>    sd_notify(0, "STOPPING=1\n"
> diff --git a/sql/sql_class.h b/sql/sql_class.h
> --- a/sql/sql_class.h
> +++ b/sql/sql_class.h
> @@ -3949,6 +3950,9 @@ class THD: public THD_count, /* this must be first */
>      set_time_for_next_stage();
>      if (utime_after_query >= utime_after_lock + variables.long_query_time)
>        server_status|= SERVER_QUERY_WAS_SLOW;
> +    /* If we're tracking idle execution, and we're down to the last connection */
> +    if (max_idle_execution <  UINT_MAX && *scheduler->connection_count <= 1)
> +      server_last_activity= utime_after_query;

1. you update it concurrenty. Must be atomic, right?

2. do you really want to shutdown the server if there's an idle
connection? I'd think it'd be less surprising to shutdown only when
there are no connections at all. If there is a runaway idle connection,
it'll die after interactive_timeout or wait_timeout. And then you
shutdown the server after max_idle_execution.

So instead of shutting down the server when there are connections,
I'd suggest to autoset interactive_timeout and wait_timeout to be in
line with max_idle_execution. Like, 10 mins, or, may be

  if (IS_SYSVAR_AUTOSIZE(net_wait_timeout) && net_wait_timeout > max_idle_execution)
    SYSVAR_AUTOSIZE(net_wait_timeout, max_idle_execution);

>    }
>    inline ulonglong found_rows(void)
>    {
> diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc
> --- a/sql/sys_vars.cc
> +++ b/sql/sys_vars.cc
> @@ -1724,6 +1724,14 @@ Sys_max_connect_errors(
>         VALID_RANGE(1, UINT_MAX), DEFAULT(MAX_CONNECT_ERRORS),
>         BLOCK_SIZE(1));
>  
> +static Sys_var_ulong Sys_max_idle_execution(

ulong is non portable.
why not make the variable uint, if max value is UINT_MAX anyway?

> +       "max_idle_execution",
> +       "If no new connections or running queries within this time (in seconds) "
> +       "shutdown the server",

may be something like "Default to 10 minutes in systemd socket
activation setup" ? I know, it means asking for troubles :(

> +       AUTO_SET GLOBAL_VAR(max_idle_execution), CMD_LINE(REQUIRED_ARG),
> +       VALID_RANGE(10, UINT_MAX), DEFAULT(UINT_MAX),
> +       BLOCK_SIZE(1));
> +
>  static Sys_var_on_access_global<Sys_var_uint,
>                                  PRIV_SET_SYSTEM_GLOBAL_VAR_MAX_PASSWORD_ERRORS>
>  Sys_max_password_errors(
> 
Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx