maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

Hi Sergei,

On Tue, Jul 22, 2014 at 10:56:31AM +0200, Sergei Golubchik wrote:
> Hi, Sergey!
> 
> On Jul 07, svoj@xxxxxxxxxxx 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