Re: [Maria-developers] MDEV-136 Non-blocking "set read_only"
Hi, Holyfoot! On May 09, holyfoot@askmonty.org wrote:
------------------------------------------------------------ revno: 3518 revision-id: holyfoot@askmonty.org-20120509185945-i1yx6d38phgoqzes parent: sanja@montyprogram.com-20120507181437-1zl1v1uxje7v6994 committer: Alexey Botchkov <holyfoot@askmonty.org> branch nick: mdev-136 timestamp: Wed 2012-05-09 23:59:45 +0500 message: MDEV-136 Non-blocking "set read_only". Handle the 'set read_only=1' in lighter way, than the FLUSH TABLES READ LOCK; For the transactional engines we don't wait for operations on that tables to finish.
See my comments below.
=== modified file 'mysql-test/r/read_only.result' --- a/mysql-test/r/read_only.result 2009-03-06 14:56:17 +0000 +++ b/mysql-test/r/read_only.result 2012-05-09 18:59:45 +0000 @@ -59,7 +59,7 @@ connection con1; select @@global.read_only; @@global.read_only -0 +1
This is prone to race conditions. Please, fix the test to remove "send" here and below. (assuming the new result is correct) But is it correct ? Why set read_only is not blocked by a write locked myisam table?
unlock tables ; select @@global.read_only; @@global.read_only @@ -80,7 +80,7 @@ connection con1; select @@global.read_only; @@global.read_only -0 +1 unlock tables ; select @@global.read_only; @@global.read_only
Where are the tests for the changed functionality?
=== modified file 'sql/sql_base.cc' --- a/sql/sql_base.cc 2012-04-19 06:16:30 +0000 +++ b/sql/sql_base.cc 2012-05-09 18:59:45 +0000 @@ -933,7 +938,8 @@ for (uint idx=0 ; idx < open_cache.records ; idx++) { TABLE *table=(TABLE*) hash_element(&open_cache,idx); - if (table->in_use) + if (table->in_use && + (!set_readonly_mode || !table->file->has_transactions()))
I wonder how this could work. The line below sets a flag *on a thread*. The task description tells "not wait for transactional tables", while your change means "not set a flag if all tables used in a thread are transactional". That is, if a thread uses both transactional and non-transactional tables, your change does nothing. Back to my first question - where are the tests for this new task?
table->in_use->some_tables_deleted= 1; } }
Regards, Sergei
Hi!
"Sergei" == Sergei Golubchik <serg@askmonty.org> writes:
<cut>
=== modified file 'sql/sql_base.cc' --- a/sql/sql_base.cc 2012-04-19 06:16:30 +0000 +++ b/sql/sql_base.cc 2012-05-09 18:59:45 +0000 @@ -933,7 +938,8 @@ for (uint idx=0 ; idx < open_cache.records ; idx++) { TABLE *table=(TABLE*) hash_element(&open_cache,idx); - if (table->in_use) + if (table->in_use && + (!set_readonly_mode || !table->file->has_transactions()))
Sergei> I wonder how this could work. The line below sets a flag *on a thread*. Sergei> The task description tells "not wait for transactional tables", while your Sergei> change means "not set a flag if all tables used in a thread are Sergei> transactional". That is, if a thread uses both transactional and Sergei> non-transactional tables, your change does nothing. I think this is actually correct. If a statement uses both transactional and non-transactional tables, the non transactional tables should be flushed as part of set readonly=1 The flag: table->in_use->some_tables_deleted is only use as a check if thr_lock() should fail. If you have a single non transactional table and the thread is doing a lock tables, we should close the table (and thus all the tables) and reopen it. However there is a couple of other things that needs to be done. If we are not going to increment the refresh version when doing set readonly, we should reset the refresh version for any non transactional open table to 0 to force it to be reopened. Holyfoot, how did you test this feature? Regards, Monty
16.05.2012 12:48, Michael Widenius написал:
Holyfoot, how did you test this feature?
I started a long-running query in one session, then ran the 'SET GLOBAL read_lock=1' in another. After the change the 'SET read_lock' doesn't wait for the INNODB to complete the query. Regards HF
Sergei, 11.05.2012 19:44, Sergei Golubchik wrote:
=== modified file 'mysql-test/r/read_only.result' --- a/mysql-test/r/read_only.result 2009-03-06 14:56:17 +0000 +++ b/mysql-test/r/read_only.result 2012-05-09 18:59:45 +0000 @@ -59,7 +59,7 @@ connection con1; select @@global.read_only; @@global.read_only -0 +1 This is prone to race conditions. Please, fix the test to remove "send" here and below. (assuming the new result is correct)
But is it correct ? Why set read_only is not blocked by a write locked myisam table?
It's not blocked as we ceased incrementing the refresh_version in this case: + /* No need to close the open tables if we just set the readonly state */ + if (!set_readonly_mode) + refresh_version++; // Force close of open tables + Right, this test with the 'send set read_only' should be either removed or updated.
Where are the tests for the changed functionality?
No specific tests yet. I was to lazy to add the new test while i was not sure i did the correct thing. Hoped to get the quick answer.
=== modified file 'sql/sql_base.cc' --- a/sql/sql_base.cc 2012-04-19 06:16:30 +0000 +++ b/sql/sql_base.cc 2012-05-09 18:59:45 +0000 @@ -933,7 +938,8 @@ for (uint idx=0 ; idx< open_cache.records ; idx++) { TABLE *table=(TABLE*) hash_element(&open_cache,idx); - if (table->in_use) + if (table->in_use&& + (!set_readonly_mode || !table->file->has_transactions())) I wonder how this could work. The line below sets a flag *on a thread*. The task description tells "not wait for transactional tables", while your change means "not set a flag if all tables used in a thread are transactional". That is, if a thread uses both transactional and non-transactional tables, your change does nothing.
That's right, it' won't work with the mixed types of tables. But as the customer didn't ask for that, maybe the simplest solution will do here.
Back to my first question - where are the tests for this new task?
Will develop some ASAP. HF
Hi, Alexey! On May 16, Alexey Botchkov wrote:
Sergei,
11.05.2012 19:44, Sergei Golubchik wrote:
=== modified file 'mysql-test/r/read_only.result' --- a/mysql-test/r/read_only.result 2009-03-06 14:56:17 +0000 +++ b/mysql-test/r/read_only.result 2012-05-09 18:59:45 +0000 @@ -59,7 +59,7 @@ connection con1; select @@global.read_only; @@global.read_only -0 +1 This is prone to race conditions. Please, fix the test to remove "send" here and below. (assuming the new result is correct)
But is it correct ? Why set read_only is not blocked by a write locked myisam table?
It's not blocked as we ceased incrementing the refresh_version in this case:
Okay. But is it correct? Can you try the following: 1st client: LOCK TABLE t1 WRITE; 2nd client: SET READ_ONLY=1; 1st client: INSERT t1 VALUES (1);
=== modified file 'sql/sql_base.cc' --- a/sql/sql_base.cc 2012-04-19 06:16:30 +0000 +++ b/sql/sql_base.cc 2012-05-09 18:59:45 +0000 @@ -933,7 +938,8 @@ for (uint idx=0 ; idx< open_cache.records ; idx++) { TABLE *table=(TABLE*) hash_element(&open_cache,idx); - if (table->in_use) + if (table->in_use&& + (!set_readonly_mode || !table->file->has_transactions())) I wonder how this could work. The line below sets a flag *on a thread*. The task description tells "not wait for transactional tables", while your change means "not set a flag if all tables used in a thread are transactional". That is, if a thread uses both transactional and non-transactional tables, your change does nothing.
That's right, it' won't work with the mixed types of tables. But as the customer didn't ask for that, maybe the simplest solution will do here.
I'm not sure about it. It's basically a gotcha. "SET READ_ONLY will not flush transactional tables, but only if the statement uses no non-transactional tables. Otherwise it'll flush all tables, transactional or not". One cannot always know if non-transactional tables are involved. Think of log tables, tables used in triggers and stored routines. Regards, Sergei
Sergei, 16.05.2012 15:21, Sergei Golubchik wrote:
Okay. But is it correct? Can you try the following: 1st client: LOCK TABLE t1 WRITE; done
2nd client: SET READ_ONLY=1; hangs, as it's blocked by LOCK TABLE
1st client: INSERT t1 VALUES (1); insert succeeded.
1st client UNLOCK TABLES; 2nd completes the SET READ_ONLY
I'm not sure about it. It's basically a gotcha. "SET READ_ONLY will not flush transactional tables, but only if the statement uses no non-transactional tables.
Otherwise it'll flush all tables, transactional or not". One cannot always know if non-transactional tables are involved. Think of > log tables, tables used in triggers and stored routines.
Well i don't have an opinion of my own on that. Unfortunately i don't know that area well enough. Ok i started investigating and hope to come out with something better tomorrow. Regards HF
Sergei, Here is the new patch: http://lists.askmonty.org/pipermail/commits/2012-May/003324.html I tested things in various ways, and it seems working decent.
1st client: LOCK TABLE t1 WRITE; done
2nd client: SET READ_ONLY=1; if the t1 is MyISAM, it hangs until t1 unlocked if the t1 is InnoDB, it completes immediately.
1st client: INSERT t1 VALUES (1);
if the t1 is MyISAM, the INSERT succeedes at once while the SET READ_ONLY is hanging in the 2nd client. if the t1 is InnoDB, the INSERT fails ERROR 1290 (HY000): The MySQL server is running with the --read-only option so it cannot execute this statement
That's right, it' won't work with the mixed types of tables. But as the customer didn't ask for that, maybe the simplest solution will do here. I'm not sure about it. It's basically a gotcha. "SET READ_ONLY will not flush transactional tables, but only if the statement uses no non-transactional tables. Otherwise it'll flush all tables, transactional or not". One cannot always know if non-transactional tables are involved. Think of log tables, tables used in triggers and stored routines.
Temporary tables shouldn't be a problem here i think. Otherwise, I wasn't able to come out with something better. Regards HF
participants (3)
-
Alexey Botchkov -
Michael Widenius -
Sergei Golubchik