Re: [Maria-developers] [Commits] 00ed367: MDEV-4262 - P_S discovery
Hi, Sergey! On Jul 07, svoj@mariadb.org wrote:
revision-id: 00ed36700addfbce91940223cce4ccd6ee5823a1 parent(s): f37f19a6c790100cc2ae5f483ba4db34a517f0ae committer: Sergey Vojtovich branch nick: 10.1 timestamp: 2014-07-07 14:32:13 +0400 message:
MDEV-4262 - P_S discovery
Discover P_S tables automatically.
Most of this patch is code clean-up: - removed tests and code responsible for P_S tables correctness verification - always return error from ha_perfschema::create() - install/upgrade scripts won't create P_S tables anymore
Great work, thanks! The patch is pretty much "ok to push", but, first I want to understand why you deleted table_file_summary.cc file. See other minor comments below.
diff --git a/mysql-test/suite/perfschema/include/upgrade_check.inc b/mysql-test/suite/perfschema/include/upgrade_check.inc index 52d4cfd..1532de8 100644 --- a/mysql-test/suite/perfschema/include/upgrade_check.inc +++ b/mysql-test/suite/perfschema/include/upgrade_check.inc @@ -3,7 +3,6 @@ #
--source include/count_sessions.inc ---error 1 --exec $MYSQL_UPGRADE --skip-verbose --force > $out_file 2> $err_file --source include/wait_until_count_sessions.inc
The rest of this upgrade_check.inc file does (according to the comment) "Verify that mysql_upgrade complained about the performance_schema" perhaps you should change the comment to say "does not complain" ?
diff --git a/scripts/mysql_performance_tables.sql b/scripts/mysql_performance_tables.sql index d495578..3061603 100644 --- a/scripts/mysql_performance_tables.sql +++ b/scripts/mysql_performance_tables.sql @@ -53,1427 +53,3 @@ SET @str = IF(@broken_pfs = 0, @cmd, 'SET @dummy = 0'); PREPARE stmt FROM @str; EXECUTE stmt; DROP PREPARE stmt;
Why did you keep the above? I'd think that a simple DROP DATABASE IF EXISTS performance_schema should be enough to replace the whole file. And it could be moved back to mysql_system_tables.sql, so this file could be completely removed.
- -# -# From this point, only create the performance schema tables -# if the server is built with performance schema -# - -set @have_pfs= (select count(engine) from information_schema.engines where engine='PERFORMANCE_SCHEMA' and support != 'NO'); - -# -# TABLE COND_INSTANCES -# ... diff --git a/storage/perfschema/pfs_engine_table.cc b/storage/perfschema/pfs_engine_table.cc index 958a2bd..6f0a363 100644 --- a/storage/perfschema/pfs_engine_table.cc +++ b/storage/perfschema/pfs_engine_table.cc @@ -1440,5 +1319,17 @@ end: DBUG_RETURN(false); }
+int pfs_discover_table_names(handlerton *hton, LEX_STRING *db, + MY_DIR *dir,
hm, no "unused argument" compiler warnings? okay...
+ handlerton::discovered_list *result) +{ + if (compare_table_names(db->str, PERFORMANCE_SCHEMA_str.str)) + return 0; + for (size_t i= 0; i < array_elements(all_shares) - 1; i++) + result->add_table(all_shares[i]->m_name.str, + all_shares[i]->m_name.length); + return 0; +} + /** @} */
diff --git a/storage/perfschema/table_file_summary.cc b/storage/perfschema/table_file_summary.cc deleted file mode 100644 index 104fa0f..0000000 --- a/storage/perfschema/table_file_summary.cc +++ /dev/null @@ -1,372 +0,0 @@
Why did you remove this file?
-/* Copyright (c) 2008, 2010, Oracle and/or its affiliates. All rights reserved. - - This program is free software; you can redistribute it and/or modify - it under the terms of the GNU General Public License as published by - the Free Software Foundation; version 2 of the License.
Regards, Sergei
Hi Sergei, On Tue, Jul 22, 2014 at 10:56:31AM +0200, Sergei Golubchik wrote:
Hi, Sergey!
On Jul 07, svoj@mariadb.org wrote:
revision-id: 00ed36700addfbce91940223cce4ccd6ee5823a1 parent(s): f37f19a6c790100cc2ae5f483ba4db34a517f0ae committer: Sergey Vojtovich branch nick: 10.1 timestamp: 2014-07-07 14:32:13 +0400 message:
MDEV-4262 - P_S discovery
Discover P_S tables automatically.
Most of this patch is code clean-up: - removed tests and code responsible for P_S tables correctness verification - always return error from ha_perfschema::create() - install/upgrade scripts won't create P_S tables anymore
Great work, thanks! Thanks!
The patch is pretty much "ok to push", but, first I want to understand why you deleted table_file_summary.cc file.
See other minor comments below.
diff --git a/mysql-test/suite/perfschema/include/upgrade_check.inc b/mysql-test/suite/perfschema/include/upgrade_check.inc index 52d4cfd..1532de8 100644 --- a/mysql-test/suite/perfschema/include/upgrade_check.inc +++ b/mysql-test/suite/perfschema/include/upgrade_check.inc @@ -3,7 +3,6 @@ #
--source include/count_sessions.inc ---error 1 --exec $MYSQL_UPGRADE --skip-verbose --force > $out_file 2> $err_file --source include/wait_until_count_sessions.inc
The rest of this upgrade_check.inc file does (according to the comment) "Verify that mysql_upgrade complained about the performance_schema" perhaps you should change the comment to say "does not complain" ?
Right, I'll fix it.
diff --git a/scripts/mysql_performance_tables.sql b/scripts/mysql_performance_tables.sql index d495578..3061603 100644 --- a/scripts/mysql_performance_tables.sql +++ b/scripts/mysql_performance_tables.sql @@ -53,1427 +53,3 @@ SET @str = IF(@broken_pfs = 0, @cmd, 'SET @dummy = 0'); PREPARE stmt FROM @str; EXECUTE stmt; DROP PREPARE stmt;
Why did you keep the above? I'd think that a simple
DROP DATABASE IF EXISTS performance_schema
should be enough to replace the whole file. And it could be moved back to mysql_system_tables.sql, so this file could be completely removed.
I thought we can't discover database name and it needs to be created manually. But if you say it is possible: I'll try it. There are also additional checks that would prevent dropping P_S database if there are non-PFS tables/views/events/routines. Should we abandon these checks too?
- -# -# From this point, only create the performance schema tables -# if the server is built with performance schema -# - -set @have_pfs= (select count(engine) from information_schema.engines where engine='PERFORMANCE_SCHEMA' and support != 'NO'); - -# -# TABLE COND_INSTANCES -# ... diff --git a/storage/perfschema/pfs_engine_table.cc b/storage/perfschema/pfs_engine_table.cc index 958a2bd..6f0a363 100644 --- a/storage/perfschema/pfs_engine_table.cc +++ b/storage/perfschema/pfs_engine_table.cc @@ -1440,5 +1319,17 @@ end: DBUG_RETURN(false); }
+int pfs_discover_table_names(handlerton *hton, LEX_STRING *db, + MY_DIR *dir,
hm, no "unused argument" compiler warnings? okay...
Strange indeed. It was compiled like: cmake -DCMAKE_BUILD_TYPE=Debug -DMYSQL_MAINTAINER_MODE=ON && make It adds -Wno-unused-parameter gcc flag. Same flag seem to be added in release builds. Did we decide not to hunt for unused arguments?
+ handlerton::discovered_list *result) +{ + if (compare_table_names(db->str, PERFORMANCE_SCHEMA_str.str)) + return 0; + for (size_t i= 0; i < array_elements(all_shares) - 1; i++) + result->add_table(all_shares[i]->m_name.str, + all_shares[i]->m_name.length); + return 0; +} + /** @} */
diff --git a/storage/perfschema/table_file_summary.cc b/storage/perfschema/table_file_summary.cc deleted file mode 100644 index 104fa0f..0000000 --- a/storage/perfschema/table_file_summary.cc +++ /dev/null @@ -1,372 +0,0 @@
Why did you remove this file?
It had file_summary_by_event_name and file_summary_by_instance which were moved to table_file_summary_by_event_name.cc and table_file_summary_by_instance.cc. Otherwise it wasn't even listed in CMakeLists.txt. Thanks, Sergey
Hi, Sergey! On Jul 22, Sergey Vojtovich wrote:
diff --git a/scripts/mysql_performance_tables.sql b/scripts/mysql_performance_tables.sql index d495578..3061603 100644 --- a/scripts/mysql_performance_tables.sql +++ b/scripts/mysql_performance_tables.sql @@ -53,1427 +53,3 @@ SET @str = IF(@broken_pfs = 0, @cmd, 'SET @dummy = 0'); PREPARE stmt FROM @str; EXECUTE stmt; DROP PREPARE stmt;
Why did you keep the above? I'd think that a simple
DROP DATABASE IF EXISTS performance_schema
should be enough to replace the whole file. And it could be moved back to mysql_system_tables.sql, so this file could be completely removed.
I thought we can't discover database name and it needs to be created manually. But if you say it is possible: I'll try it. There are also additional checks that would prevent dropping P_S database if there are non-PFS tables/views/events/routines. Should we abandon these checks too?
I'm not sure. P_S doesn't allow anyone to create table in the P_S database. But yes, one can copy files manually. So, the options are 1. db is not needed, don't preserve files: DROP DATABASE IF EXISTS performance_schema 2. db is needed, don't preserve files: DROP DATABASE IF EXISTS performance_schema CREATE DATABASE performance_schema 3. preserve files: CREATE DATABASE IF NOT EXISTS performance_schema Either way, there's no need to do any IF's and checks. I tend to agree, it's better to preserve user's files (and other objects) in P_S.
diff --git a/storage/perfschema/pfs_engine_table.cc b/storage/perfschema/pfs_engine_table.cc index 958a2bd..6f0a363 100644 --- a/storage/perfschema/pfs_engine_table.cc +++ b/storage/perfschema/pfs_engine_table.cc @@ -1440,5 +1319,17 @@ end: DBUG_RETURN(false); }
+int pfs_discover_table_names(handlerton *hton, LEX_STRING *db, + MY_DIR *dir,
hm, no "unused argument" compiler warnings? okay... Strange indeed. It was compiled like: cmake -DCMAKE_BUILD_TYPE=Debug -DMYSQL_MAINTAINER_MODE=ON && make
It adds -Wno-unused-parameter gcc flag. Same flag seem to be added in release builds. Did we decide not to hunt for unused arguments?
Seems like this was done in 5.5 by Oracle in 2010
diff --git a/storage/perfschema/table_file_summary.cc b/storage/perfschema/table_file_summary.cc deleted file mode 100644 index 104fa0f..0000000 --- a/storage/perfschema/table_file_summary.cc +++ /dev/null @@ -1,372 +0,0 @@
Why did you remove this file? It had file_summary_by_event_name and file_summary_by_instance which were moved to table_file_summary_by_event_name.cc and table_file_summary_by_instance.cc.
Otherwise it wasn't even listed in CMakeLists.txt.
Yes, I've noticed that you only removed pfs_check.cc from CMakeLists.txt, that was also puzzling. Okay then. Regards, Sergei
Hi Sergei, On Tue, Jul 22, 2014 at 12:37:25PM +0200, Sergei Golubchik wrote:
On Jul 22, Sergey Vojtovich wrote:
diff --git a/scripts/mysql_performance_tables.sql b/scripts/mysql_performance_tables.sql index d495578..3061603 100644 --- a/scripts/mysql_performance_tables.sql +++ b/scripts/mysql_performance_tables.sql @@ -53,1427 +53,3 @@ SET @str = IF(@broken_pfs = 0, @cmd, 'SET @dummy = 0'); PREPARE stmt FROM @str; EXECUTE stmt; DROP PREPARE stmt;
Why did you keep the above? I'd think that a simple
DROP DATABASE IF EXISTS performance_schema
should be enough to replace the whole file. And it could be moved back to mysql_system_tables.sql, so this file could be completely removed.
I thought we can't discover database name and it needs to be created manually. But if you say it is possible: I'll try it. There are also additional checks that would prevent dropping P_S database if there are non-PFS tables/views/events/routines. Should we abandon these checks too?
I'm not sure. P_S doesn't allow anyone to create table in the P_S database. But yes, one can copy files manually. So, the options are
1. db is not needed, don't preserve files:
DROP DATABASE IF EXISTS performance_schema
2. db is needed, don't preserve files:
DROP DATABASE IF EXISTS performance_schema CREATE DATABASE performance_schema
3. preserve files:
CREATE DATABASE IF NOT EXISTS performance_schema
Either way, there's no need to do any IF's and checks.
I tend to agree, it's better to preserve user's files (and other objects) in P_S. Then what about:
4. db is not needed, but preserve files: - nothing - 5. db is not needed, preserve files, but drop database if there are no user files: IF-s and then DROP DATABASE IF EXISTS performance_schema Thanks, Sergey
Hi, Sergey! On Jul 22, Sergey Vojtovich wrote:
There are also additional checks that would prevent dropping P_S database if there are non-PFS tables/views/events/routines. Should we abandon these checks too?
I'm not sure. P_S doesn't allow anyone to create table in the P_S database. But yes, one can copy files manually. So, the options are
1. db is not needed, don't preserve files: DROP DATABASE IF EXISTS performance_schema 2. db is needed, don't preserve files: DROP DATABASE IF EXISTS performance_schema CREATE DATABASE performance_schema 3. preserve files: CREATE DATABASE IF NOT EXISTS performance_schema
Either way, there's no need to do any IF's and checks.
I tend to agree, it's better to preserve user's files (and other objects) in P_S. Then what about:
4. db is not needed, but preserve files:
- nothing -
That's fine too.
5. db is not needed, preserve files, but drop database if there are no user files:
IF-s and then DROP DATABASE IF EXISTS performance_schema
I wouldn't bother - too complicated. But it's up to you. Regards, Sergei
participants (2)
-
Sergei Golubchik -
Sergey Vojtovich