Hi! On Sun, Dec 29, 2024 at 1:52 PM Sergei Golubchik <serg@mariadb.org> wrote:

Hi, Monty,

Only a couple of comments about the test case:

...

commit 448e82f9f0d Author: Michael Widenius <monty@mariadb.org> Date: Fri Dec 27 19:26:55 2024 +0200

diff --git a/mysql-test/main/log_state.test b/mysql-test/main/log_state.test --- a/mysql-test/main/log_state.test +++ b/mysql-test/main/log_state.test @@ -321,6 +321,48 @@ SET GLOBAL general_log_file = @old_general_log_file;

--enable_ps_protocol

+--echo # +--echo # MDEV-35720 Add query_time to statistics +--echo # + +connect (con2,localhost,root,,); +let $s1=`show status like "query_time"`; +select sleep(3) into @a; +let $s2=`show status like "query_time"`; +let $s3=`show global status like "query_time"`; +--disable_query_log +--eval SET @s1= SUBSTRING_INDEX('$s1', '\t', -1); +--eval SET @s2= SUBSTRING_INDEX('$s2', '\t', -1); +--eval SET @s3= SUBSTRING_INDEX('$s3', '\t', -1);

Eh, that's not how one is supposed to do it. I mean, it works, but the easier approach is

let $s1=query_get_value(show status like "query_time", Value);

or purely in SQL

set @s1=(select variable_value from information_schema.global_status where variable_name='query_time');

I use the above substring approach as this was used in some other tests. We should probably fix the other tests to use the simpler approach to not confuse other developers. I still hope that we can soon implement @@status.query_time as it will make things much simpler to use the status variables directly in expressions.

...

+select @s1 >= 0.00 and @s1 <= 2.00 as "should be true"; +--eval select @s2 >= 2.00 and @s2 < 10.00 as "should be true"; +--eval select @s3 >= 3.00 as "should be true";

1. you don't need eval here

This comes from me first having $s1 in the above, but later replaced with @s1.

2. I expect this test, like almost all other timing tests will be horribly unstable in buildbot and will be eventually disabled. No, I don't have a recipe for a stable timing test.

I do not expect the above to be stable as most of the test are >= of someting that is guaranteed to always be true. The only' unsafe' thing ius the < 10.00, but that can easily be increased if that is wrong. This is just there to ensure we do not get any totally ridiculous value for query_time. Thanks for the review, everything is now fixed (except the >= 10 test) Regards, Monty