Re: [Maria-developers] [Commits] 7f7cbb1: MDEV-6756: map a linux pid (child pid) to a connection id shown in the output of SHOW PROCESSLIST
Hi, Sanja! There's an issue with cmake detection and with tests. But the code looks fine, thanks! On Sep 14, sanja@mariadb.com wrote:
revision-id: 7f7cbb132fa0a40687e4b25a9c9e163c93ec6d15 (mariadb-10.1.6-12-g7f7cbb1) parent(s): 86a3613d4e981c341e38291c9eeec5dc9f836fae committer: Oleksandr Byelkin timestamp: 2015-09-14 12:47:57 +0200 message:
MDEV-6756: map a linux pid (child pid) to a connection id shown in the output of SHOW PROCESSLIST
Added tid (thread ID) for system where it is present.
ps -eL -o tid,pid,command
shows the thread on Linux
diff --git a/mysql-test/r/information_schema.result b/mysql-test/r/information_schema.result index 2a918ad..ef889c7 100644 --- a/mysql-test/r/information_schema.result +++ b/mysql-test/r/information_schema.result @@ -2066,3 +2066,7 @@ Variable_name Value Opened_tables 3 drop database mysqltest; drop database db1; +set @TID= (SELECT tid FROM information_schema.processlist); +select (SELECT tid FROM information_schema.processlist) = @TID; +(SELECT tid FROM information_schema.processlist) = @TID +1
That seems pretty useless as a test :)
diff --git a/sql/sql_class.cc b/sql/sql_class.cc index bf161b2..0478b33 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -69,6 +69,10 @@ #include "sql_connect.h" #include "my_atomic.h"
+#ifdef HAVE_SYS_SYSCALL_H
Pardon me? Did you see whether your code actually work? You check here whether HAVE_SYS_SYSCALL_H is defined, but you don't have CHECK_INCLUDE_FILES(sys/syscall.h HAVE_SYS_SYSCALL_H) in configure.cmake and HAVE_SYS_SYSCALL_H in config.h.cmake. So, I don't see how your code can possibly work as HAVE_SYS_SYSCALL_H should be never defined in your patch. Please, when fixing it, make sure that your test *fails* without TID support (I suspect you'll have to enable it only on linux). And then you could remove that useless test above.
+#include
+#endif
Regards, Sergei
On 15.09.15 15:21, Sergei Golubchik wrote:
Hi, Sanja!
There's an issue with cmake detection and with tests. But the code looks fine, thanks!
On Sep 14, sanja@mariadb.com wrote:
revision-id: 7f7cbb132fa0a40687e4b25a9c9e163c93ec6d15 (mariadb-10.1.6-12-g7f7cbb1) parent(s): 86a3613d4e981c341e38291c9eeec5dc9f836fae committer: Oleksandr Byelkin timestamp: 2015-09-14 12:47:57 +0200 message:
MDEV-6756: map a linux pid (child pid) to a connection id shown in the output of SHOW PROCESSLIST
Added tid (thread ID) for system where it is present.
ps -eL -o tid,pid,command
shows the thread on Linux
diff --git a/mysql-test/r/information_schema.result b/mysql-test/r/information_schema.result index 2a918ad..ef889c7 100644 --- a/mysql-test/r/information_schema.result +++ b/mysql-test/r/information_schema.result @@ -2066,3 +2066,7 @@ Variable_name Value Opened_tables 3 drop database mysqltest; drop database db1; +set @TID= (SELECT tid FROM information_schema.processlist); +select (SELECT tid FROM information_schema.processlist) = @TID; +(SELECT tid FROM information_schema.processlist) = @TID +1 That seems pretty useless as a test :) What else I can check? I have an idea that in diferent threads the TIDs are different, but it will fail on platforms where it is not supported and so equal to zero.
diff --git a/sql/sql_class.cc b/sql/sql_class.cc index bf161b2..0478b33 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -69,6 +69,10 @@ #include "sql_connect.h" #include "my_atomic.h"
+#ifdef HAVE_SYS_SYSCALL_H Pardon me? Did you see whether your code actually work? You check here whether HAVE_SYS_SYSCALL_H is defined, but you don't have
CHECK_INCLUDE_FILES(sys/syscall.h HAVE_SYS_SYSCALL_H)
in configure.cmake and HAVE_SYS_SYSCALL_H in config.h.cmake. So, I don't see how your code can possibly work as HAVE_SYS_SYSCALL_H should be never defined in your patch.
Please, when fixing it, make sure that your test *fails* without TID support (I suspect you'll have to enable it only on linux).
And then you could remove that useless test above.
I checked and it works, to make it working I added: +#cmakedefine HAVE_SYS_SYSCALL_H 1 to config.h.cmake
+#include
+#endif Regards, Sergei
Hi, Oleksandr! On Sep 15, Oleksandr Byelkin wrote:
diff --git a/mysql-test/r/information_schema.result b/mysql-test/r/information_schema.result index 2a918ad..ef889c7 100644 --- a/mysql-test/r/information_schema.result +++ b/mysql-test/r/information_schema.result @@ -2066,3 +2066,7 @@ Variable_name Value Opened_tables 3 drop database mysqltest; drop database db1; +set @TID= (SELECT tid FROM information_schema.processlist); +select (SELECT tid FROM information_schema.processlist) = @TID; +(SELECT tid FROM information_schema.processlist) = @TID +1 That seems pretty useless as a test :) What else I can check? I have an idea that in diferent threads the TIDs are different, but it will fail on platforms where it is not supported and so equal to zero.
Yes, you can move it to a linux-specific test file. But a test that succeeds with a feature disabled is not of much use.
diff --git a/sql/sql_class.cc b/sql/sql_class.cc index bf161b2..0478b33 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -69,6 +69,10 @@ #include "sql_connect.h" #include "my_atomic.h"
+#ifdef HAVE_SYS_SYSCALL_H Pardon me? Did you see whether your code actually work? You check here whether HAVE_SYS_SYSCALL_H is defined, but you don't have
CHECK_INCLUDE_FILES(sys/syscall.h HAVE_SYS_SYSCALL_H)
in configure.cmake and HAVE_SYS_SYSCALL_H in config.h.cmake. So, I don't see how your code can possibly work as HAVE_SYS_SYSCALL_H should be never defined in your patch.
Please, when fixing it, make sure that your test *fails* without TID support (I suspect you'll have to enable it only on linux).
And then you could remove that useless test above. I checked and it works, to make it working I added:
+#cmakedefine HAVE_SYS_SYSCALL_H 1
to config.h.cmake
Right, but you still need that CHECK_INCLUDE_FILES() line. Currently it works only because tokudb checks for sys/syscall.h. If you do a clean build without tokudb, you code won't work again. Regards, Sergei
participants (2)
-
Oleksandr Byelkin -
Sergei Golubchik