- developers - lists.mariadb.org

[Maria-developers] Pushing down aggregation functions
by Berrocal, Eduardo 04 Aug '21

04 Aug '21

Hello!, Continuing in my series of emails about engine condition pushdown... I wonder if it is possible to push down aggregation functions such as SUM(), MIN(), MAX() or AVG() to the storage engine. Currently, and as far as I can see (I may be wrong here), it is not possible. However, this is something that can be beneficial if the storage engine can perform some of these basic computations itself, like suming over a whole column and returning just the result. The storage engine can even be smart enough to use some hardware advantages like accelerators or computational storage. If you guys think this is indeed possible without drastic code changes, it would be amazing to be pointed at where this is. At first, I would only push down functions if they operate over just one column of the same table. Things can be made more complicated in the future, of course. Thanks a lot!, Eduardo.

2 1

Re: [Maria-developers] 203aa38f9b6: MDEV-26102: gcov 9.1 doesn't support intermediate format any more, but json instead - refactoring needed
by Sergei Golubchik 29 Jul '21

29 Jul '21

Hi, Anel! On Jul 28, Anel Husakovic wrote: > revision-id: 203aa38f9b6 (mariadb-10.2.39-96-g203aa38f9b6) > parent(s): afe00bb7cce > author: Anel Husakovic > committer: Anel Husakovic > timestamp: 2021-07-27 16:09:41 +0200 > message: > > MDEV-26102: gcov 9.1 doesn't support intermediate format any more, but json instead - refactoring needed I've renamed the MDEV to have a shorter title, use the new one in your next commit > > Reviewed by: serg(a)mariadb.com that's not right, I haven't reviewed it yet :) > diff --git a/mysql-test/dgcov.pl b/mysql-test/dgcov.pl > index 2c00c64d1ff..e2aadec33e0 100755 > --- a/mysql-test/dgcov.pl > +++ b/mysql-test/dgcov.pl > @@ -25,6 +25,8 @@ use warnings; > use Getopt::Long; > use File::Find; > use Cwd qw/realpath/; > +use IO::Uncompress::Gunzip qw(gunzip $GunzipError); > +use JSON::PP; move this down, dcgov should not try to load these modules for for gcc < 9 > my $opt_verbose=0; > my $opt_generate; > @@ -32,6 +34,7 @@ my $opt_help; > my $opt_purge; > my $opt_only_gcov; > my $opt_skip_gcov; > +my $fname; and this > > my %cov; > my $file_no=0; > @@ -62,12 +65,15 @@ my $res; > my $cmd; > if ($opt_purge) > { > - $cmd= "find . -name '*.da' -o -name '*.gcda' -o -name '*.gcov' -o ". > + $cmd= "find . -name '*.da' -o -name '*.gcda*' -o -name '*.gcov' -o ". > "-name '*.dgcov' | grep -v 'README\.gcov' | xargs rm -f ''"; > logv "Running: $cmd"; > system($cmd)==0 or die "system($cmd): $? $!"; > exit 0; > } > + > +my $gcc_version= `gcc -dumpversion`; > +$gcc_version=~ s/(\d).*$/$1/; you don't need this second line, I suspect > > find(\&gcov_one_file, $root); > find(\&write_coverage, $root) if $opt_generate; > @@ -162,6 +168,7 @@ sub gcov_one_file { > } > > # now, read the generated file > + if ($gcc_version <9){ > for my $gcov_file (<$_*.gcov>) { > open FH, '<', "$gcov_file" or die "open(<$gcov_file): $!"; > my $fname; > @@ -183,6 +189,18 @@ sub gcov_one_file { > $cov{$fname}->{$1}+=$2; > } > close(FH); > + } > + } > + else { put both `use` lines here > + my $gcov_file_json; > + gunzip "$_.gcov.json.gz" => \$gcov_file_json or die "gunzip($_.gcov.json.gz): $GunzipError"; > + my $obj= decode_json $gcov_file_json; > + for my $file (@{$obj->{files}}) { > + $fname= $file->{file}; and write `my $fname` here > + for my $line (@{$file->{lines}}){ > + $cov{$fname}->{$line->{line_number}}= $line->{count}; shouldn't it be += $line->{count} ? The point is to accumulate all counters, see how it's done for non-json intermediate format. > + } > + } > } > } > > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

Re: [Maria-developers] ce289840131: MDEV-24511 null field is created with CREATE..SELECT
by Nikita Malyavin 28 Jul '21

28 Jul '21

Hello, Sergei! Looks fine for me. So if you both prefer it more, let's use this variant. Regards, Nikita On Wed, Jul 28, 2021 at 12:08 PM Sergei Golubchik <serg(a)mariadb.org> wrote: > Hi, Nikita! > > On Jul 27, Nikita Malyavin wrote: > > revision-id: ce289840131 (mariadb-10.3.30-31-gce289840131) > > parent(s): b50ea900636 > > author: Nikita Malyavin > > committer: Nikita Malyavin > > timestamp: 2021-07-22 22:38:31 +0300 > > message: > > > > MDEV-24511 null field is created with CREATE..SELECT > > > > The regression has been initially appeared by MDEV-12588 patch > (441349aa). > > > > The reason CREATE... SELECT NULL does not create null field is that > > it calls handle_select(), where the field is created directly from Item, > > by calling type_handler->type_handler_for_tmp_table(), instead of plain > > type_handler. > > > > In UNION case, we'll already have Item_field's containing UNION'ed tmp > > table fields to the moment of select_create::prepare() call. > > > > There, item's type_handler is used directly in > > st_select_lex_unit::join_union_item_types(), instead of > > type_handler_for_tmp_table() or type_handler_for_union(). > > > > Using those two involve too many changes in CREATE...SELECT...UNION > > behavior, which may be reasonably undesired to be pushed in 10.3 > > > > However, patching Field_null::type_handler() directly seems to be > accurate > > enough. > > > > diff --git a/mysql-test/main/union.result b/mysql-test/main/union.result > > index a892f6c9e40..b19427948a6 100644 > > --- a/mysql-test/main/union.result > > +++ b/mysql-test/main/union.result > > @@ -1609,7 +1609,7 @@ NULL binary(0) YES NULL > > CREATE TABLE t5 SELECT NULL UNION SELECT NULL; > > DESC t5; > > Field Type Null Key Default Extra > > -NULL null YES NULL > > +NULL binary(0) YES NULL > > CREATE TABLE t6 > > SELECT * FROM (SELECT * FROM (SELECT NULL)a) b UNION SELECT a FROM t1; > > DESC t6; > > diff --git a/sql/field.h b/sql/field.h > > index a3385736c0a..2af5346bea9 100644 > > --- a/sql/field.h > > +++ b/sql/field.h > > @@ -2526,7 +2526,7 @@ class Field_null :public Field_str { > > :Field_str(ptr_arg, len_arg, null, 1, > > unireg_check_arg, field_name_arg, collation) > > {} > > - const Type_handler *type_handler() const { return &type_handler_null; > } > > + const Type_handler *type_handler() const { return > &type_handler_string; } > > Information_schema_character_attributes > > information_schema_character_attributes() const > > { > > This looks suspiciously risky to me. The problem was specifically in > UNION, but you change the very basic idea of what type the Field_null > is. > > I cannot clearly see what else might this possibly affect, field's type > is used everywhere. > > It would be safer and more logical to fix specifically field creation > for UNION. > > I've discussed it with Bar, who implemented this whole Type_handler > hierarchy, he suggested to use Type_handler::union_element_finalize() > for that. I've attached the patch that does that. > > union_element_finalize() is for 10.4, so I also backported it to 10.3, > it's very straightforward though. > > What do you think? > > Regards, > Sergei > VP of MariaDB Server Engineering > and security(a)mariadb.org > -- Nikita Malyavin, Software Engineer MariaDB Corporation

1 0

Re: [Maria-developers] ce289840131: MDEV-24511 null field is created with CREATE..SELECT
by Sergei Golubchik 28 Jul '21

28 Jul '21

Hi, Nikita! On Jul 27, Nikita Malyavin wrote: > revision-id: ce289840131 (mariadb-10.3.30-31-gce289840131) > parent(s): b50ea900636 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2021-07-22 22:38:31 +0300 > message: > > MDEV-24511 null field is created with CREATE..SELECT > > The regression has been initially appeared by MDEV-12588 patch (441349aa). > > The reason CREATE... SELECT NULL does not create null field is that > it calls handle_select(), where the field is created directly from Item, > by calling type_handler->type_handler_for_tmp_table(), instead of plain > type_handler. > > In UNION case, we'll already have Item_field's containing UNION'ed tmp > table fields to the moment of select_create::prepare() call. > > There, item's type_handler is used directly in > st_select_lex_unit::join_union_item_types(), instead of > type_handler_for_tmp_table() or type_handler_for_union(). > > Using those two involve too many changes in CREATE...SELECT...UNION > behavior, which may be reasonably undesired to be pushed in 10.3 > > However, patching Field_null::type_handler() directly seems to be accurate > enough. > > diff --git a/mysql-test/main/union.result b/mysql-test/main/union.result > index a892f6c9e40..b19427948a6 100644 > --- a/mysql-test/main/union.result > +++ b/mysql-test/main/union.result > @@ -1609,7 +1609,7 @@ NULL binary(0) YES NULL > CREATE TABLE t5 SELECT NULL UNION SELECT NULL; > DESC t5; > Field Type Null Key Default Extra > -NULL null YES NULL > +NULL binary(0) YES NULL > CREATE TABLE t6 > SELECT * FROM (SELECT * FROM (SELECT NULL)a) b UNION SELECT a FROM t1; > DESC t6; > diff --git a/sql/field.h b/sql/field.h > index a3385736c0a..2af5346bea9 100644 > --- a/sql/field.h > +++ b/sql/field.h > @@ -2526,7 +2526,7 @@ class Field_null :public Field_str { > :Field_str(ptr_arg, len_arg, null, 1, > unireg_check_arg, field_name_arg, collation) > {} > - const Type_handler *type_handler() const { return &type_handler_null; } > + const Type_handler *type_handler() const { return &type_handler_string; } > Information_schema_character_attributes > information_schema_character_attributes() const > { This looks suspiciously risky to me. The problem was specifically in UNION, but you change the very basic idea of what type the Field_null is. I cannot clearly see what else might this possibly affect, field's type is used everywhere. It would be safer and more logical to fix specifically field creation for UNION. I've discussed it with Bar, who implemented this whole Type_handler hierarchy, he suggested to use Type_handler::union_element_finalize() for that. I've attached the patch that does that. union_element_finalize() is for 10.4, so I also backported it to 10.3, it's very straightforward though. What do you think? Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] c34533186ab: MDEV-18734 ASAN heap-use-after-free upon sorting by blob column from partitioned table
by Sergei Golubchik 26 Jul '21

26 Jul '21

Hi, Aleksey! As before, despite the email subject, it's git diff dfa2d0bc13..e533b5fb307 Don't be confused :) On Jul 21, Aleksey Midenkov wrote: > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc > index c53732a2b51..70ed14448ea 100644 > --- a/sql/ha_partition.cc > +++ b/sql/ha_partition.cc > @@ -5130,7 +5135,18 @@ bool ha_partition::init_record_priority_queue() > i= bitmap_get_next_set(&m_part_info->read_partitions, i)) > { > DBUG_PRINT("info", ("init rec-buf for part %u", i)); > - int2store(ptr, i); > + if (table->s->blob_fields) > + { > + for (uint j= 0; j < table->s->blob_fields; ++j, ++objs) > + { > + blob_storage[j]= new (objs) Ordered_blob_storage; > + if (!blob_storage[j]) > + DBUG_RETURN(true); is this possible? > + } > + *((Ordered_blob_storage ***) ptr)= blob_storage; > + blob_storage+= table->s->blob_fields; > + } > + int2store(ptr + sizeof(String **), i); > ptr+= m_priority_queue_rec_len; > } > m_start_key.key= (const uchar*)ptr; > @@ -6291,6 +6333,43 @@ int ha_partition::handle_ordered_index_scan_key_not_found() > } > > > +void ha_partition::swap_blobs(uchar * rec_buf, Ordered_blob_storage ** storage, bool restore) > +{ > + uint *ptr, *end; > + uint blob_n= 0; > + table->move_fields(table->field, rec_buf, table->record[0]); > + for (ptr= table->s->blob_field, end= ptr + table->s->blob_fields; > + ptr != end; ++ptr, ++blob_n) > + { > + DBUG_ASSERT(*ptr < table->s->fields); > + Field_blob *blob= (Field_blob*) table->field[*ptr]; > + DBUG_ASSERT(blob->flags & BLOB_FLAG); > + DBUG_ASSERT(blob->field_index == *ptr); > + if (!bitmap_is_set(table->read_set, *ptr) || blob->is_null()) > + continue; > + > + Ordered_blob_storage &s= *storage[blob_n]; > + > + if (restore) > + { > + if (!s.blob.is_empty()) > + blob->swap(s.blob, s.set_read_value); don't you need here something like else blob->reset(); to make blob empty? > + } > + else > + { > + bool set_read_value; > + String *cached= blob->cached(set_read_value); > + if (cached) > + { > + cached->swap(s.blob); > + s.set_read_value= set_read_value; > + } > + } > + } > + table->move_fields(table->field, table->record[0], rec_buf); On swap_blobs() you always move all fields from rec_buf to table->record[0]? Why? This looks very suspicious. > +} > + > + > /* > Common routine to handle index_next with ordered results > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

Re: [Maria-developers] c4835af7e8c: MDEV-16026: Forbid global system_versioning_asof in non-default time zone
by Sergei Golubchik 26 Jul '21

26 Jul '21

Hi, Nikita! I suggest to squash commits from c4835af7e8c to 80e3007a5a1 into one. It'll be simpler, no intermediate juggling with MYSQL_TIME anymore. This squashed commit is ok to push (make sure to mention both MDEVs in the commit comment, but not necessarily in the first line) On Jul 26, Nikita Malyavin wrote: > revision-id: c4835af7e8c (mariadb-10.3.30-30-gc4835af7e8c) > parent(s): ec3289b6de4 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2021-07-21 00:21:31 +0300 > message: > > MDEV-16026: Forbid global system_versioning_asof in non-default time zone > > * store `system_versioning_asof` in unix time; > * both session and global vars are processed in session timezone; > * setting `default` does not copy global varibale abymore. Instead, it sets > system_time to SYSTEM_TIME_UNSPECIFIED, which means that no 'AS OF' time > is applied and `now()` can be assumed > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] a4a5c2d5963: MDEV-25908: -e does not work for my_print_defaults
by Sergei Golubchik 26 Jul '21

26 Jul '21

Hi, Rucha! On Jul 26, Rucha Deodhar wrote: > revision-id: a4a5c2d5963 (mariadb-10.5.11-52-ga4a5c2d5963) > parent(s): 675b72d0da1 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-07-23 19:51:57 +0530 > message: > > MDEV-25908: -e does not work for my_print_defaults > > Analysis: get_defaults_options() rewrites the value for my_defaults_file, > my_defaults_extra_file and my_defaults_group_suffix to 0. So the config file > can't be read. > Fix: Let handle_options() handle --defaults* option and create a new command > line to pass it to load_defaults(). > > diff --git a/libmariadb b/libmariadb > index 802ce584a26..180c543704d 160000 > --- a/libmariadb > +++ b/libmariadb > @@ -1 +1 @@ > -Subproject commit 802ce584a26fdc0ba67fcf35e277bf3c7440956a > +Subproject commit 180c543704d627a50a52aaf60e24ca14e0ec4686 ^^^^^ This is wrong, please, remove it from your commit Otherwise ok to push Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

[Maria-developers] Accessing read records during join for condition pushdown
by Berrocal, Eduardo 23 Jul '21

23 Jul '21

Hello list (first email here, hopefully sending it to the right address), I have been trying to solve this problem for a while now, and I am stuck. I wonder if what I am trying to do is even possible. I am implementing a new storage engine which handles engine condition pushdown. Now, when MariaDB is working on a JOIN using more than one table (for simplicity, let's assume 2), how can I access the read records for table 1 when working on a condition pushdown for table 2? When joining like this: table1.field1 = table2.field1, I can see 2 Item objects of type FIELD in the condition. However, I can't find where the read records for table1 are so I can effectively filter records in table2 by those values. Thanks a lot for your help, [A close up of a sign Description automatically generated] Eduardo Berrocal García de Carellán Senior Software Engineer Intel Corporation | intel.com<http://intel.com/>

3 9

Re: [Maria-developers] 412e696fd2b: MDEV-25395 server recovery hits replication event checksum error
by Sergei Golubchik 23 Jul '21

23 Jul '21

Hi, Andrei! fdle changes - ok, I've seen them in 21117. cur_log/etc - clear. What are the changes around prev_event_pos for? On Jul 23, Andrei Elkin wrote: > revision-id: 412e696fd2b (mariadb-10.3.26-136-g412e696fd2b) > parent(s): 2fa526e26e8 > author: Andrei Elkin > committer: Andrei Elkin > timestamp: 2021-04-15 17:11:07 +0300 > message: > > MDEV-25395 server recovery hits replication event checksum error > > When @@global.binlog_checksum is changing from none to > crc32 and only the first of two Binlog_checkpoint_log_event gets written to > the crc32 rotated binlog file before the server crashes, the recovery faces a checksum > verification error. > > The reason was that recovery algorithm did not realize > potentially varying binlog checksum at scanning of > a sequence of binlog files. > > That is corrected now. > The master-checksum-verify server recovery aborts now and stops > the server due to the checksum error. > A simulation test confirms the fixes. > > Induced changes: > > rpl_cant_read_event_incident got affected because the test > adopts an external not properly closed binlog file that contains > events which can't pass checksum verification. > A work-around is done to accept that binlog and evade recovery. > > binlog_recover_checksum_error needed refinement as > the master-checksum-verify configured server is made to stop > due to the checksum error. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 1c7d4f9c06f: MDEV-25282 Auto-shutdown on idle when socket-activated
by Sergei Golubchik 23 Jul '21

23 Jul '21

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(a)mariadb.org

1 0