Code review comment for lp:~laurynas-biveinis/percona-server/5.6-merge-1

Hi Laurynas. Time is a precious resource these days, I only did a
superficial review, but here goes. I've broken comments/questions into
multiple sections for convenience.

Needs fixing:
=============

These do not necessarily block the MP and can be fixed
either in this MP or subsequent ones, as you like. I would just like to
have these fixed before the code lands in trunk:

   - many spurious copyright changes in the upstream code. Most of them
     look like merge oversights in upstream, but I don't think we should
     fix their copyrights for them :) Something like this should give
     you the list:

: bzr diff -c318 | egrep '^-.*Oracle'

   - as changes in disabled_replication.result show, we deviate from
     upstream behavior when SHOW SLAVE STATUS is run on a non-slave
     server. Upstream sends an empty row with no warnings, PS 5.6 sends
     no column metadata and a warning. Which looks like a very minor
     difference, but 1) some monitoring scripts may stop working if
     their authors are cautious about warnings and 2) there's really no
     reason to change that in PS 5.6, it looks more like an incorrect
     merge of show_slave_status_nolock

   - debug fprintf()s in sys_vars.h:

@@ -192,6 +193,9 @@
       if (SIGNED)
       {
         longlong max_val= *max_var_ptr();
+ fprintf(stderr, "v = %lld, max_val = %lld\n", v, max_val);
+ fprintf(stderr, "save_result = %lld\n",
+ (longlong)(var->save_result.ulonglong_value));

    (and a couple of more occurrences later)

   - rows_read have been removed from PROCESSLIST, but not from the slow
     query log. Which means
     https://blueprints.launchpad.net/percona-server/+spec/obsolete-rows-read-in-56
     is only partially implemented

   - incorrect PERCONA_INNODB_VERSION:

--- Percona-Server/storage/innobase/include/univ.i 2012-12-04 08:24:59 +0000
+++ Percona-Server/storage/innobase/include/univ.i 2013-05-06 15:43:51 +0000
@@ -46,6 +46,11 @@
 #define INNODB_VERSION_MINOR 2
 #define INNODB_VERSION_BUGFIX MYSQL_VERSION_PATCH

+#ifndef PERCONA_INNODB_VERSION
+#define PERCONA_INNODB_VERSION 29.3
+#endif
+

     And it is the same in 5.6-merge-3.

   - all UNIV_SRV_PRINT_LATCH_WAITS code has been removed upstream, but
     we are reintroducing it for no apparent reasons

Needs information:
==================

   - can you explain the following change?

--- Percona-Server/mysql-test/r/derived.result 2013-02-12 07:47:19 +0000
+++ Percona-Server/mysql-test/r/derived.result 2013-05-06 15:43:51 +0000
@@ -1530,7 +1530,7 @@
 heap to myisam.
 show status like 'Handler_write';
 Variable_name Value
-Handler_write 1021
+Handler_write 1011
 set @@max_heap_table_size= @save_heap_size;
 drop table t1;
 #

   - can you explain changes in
     mysql-test/suite/innodb/r/innodb_wl6347_comp_indx_stat.result ?

   - what's the story with the top-level 'internal' directory? it is
     currently missing in both 5.5 upstream and 5.6 upstream, and the
     only useful test it contains is the one for
     http://bugs.mysql.com/bug.php?id=65745.
     Was it removed in previous upstream releases, but we decided to
     keep it? Do we actually run it in MTR/Jenkins?

Nice to have
============
These have little relevance to the merge per se, but some of them can be
reported as low-priority bugs for PS 5.1/5.5 as well.

   - MAX_KEY <= 128 compile-time assertion in userstat, is thus
    incompatible with 5.7. see http://bugs.mysql.com/54127. This would
    be a valid report against PS 5.7, but OTOH I don't see any reasons
    for that assertion. So could be considered a Wishlist bug for lower
    versions.

   - wrong comment alignment in innochecksum.cc (it is correct in 5.5):

+#define DICT_TF_FORMAT_ZIP 1 /*!< InnoDB plugin for 5.1:
+ compressed tables,
+ new BLOB treatment */

   - I would not even bother to comment on the above, but it looks like
     we can avoid those defines altogether, as innochecksum.cc can now
     include InnoDB headers. So instead of defining/using
     PAGE_ZIP_MIN_SIZE we can just use UNIV_ZIP_SIZE_MIN, and all
     DICT_TF_FORMAT* stuff can be replaced with the
     DICT_TF_HAS_ATOMIC_BLOBS() check from dict0mem.h

   - duplicate LIST_PROCESS_HOST_LEN definition in mysql_com.h. We
     should just move the definition from sql_show.cc to a more
     approriate place (which I think is not mysql_com.h)

   - man pages should be updated from upstream 5.6 tarballs

   - the patch removes the upstream sys_vars.innodb_page_size_basic test
     and replaces it with our test. The upside is that it will make
     future merges from PS 5.5 easier (though file-ids
     differ from both PS 5.5 and upstream 5.6), the downside is that 1)
     our test is a little more basic and 2) upstream merges may result
     in file-id conflicts. I would just keep the upstream test

   - we should remove our own fix for bug #813587. It is a merge
     regression resulting in duplicate code, though it looks harmless.

   - there's an unused ha_innobase::is_corrupt() function

General thoughts:
=================
(no immediate action required):

   - there seems to be much overlap between
     http://www.percona.com/doc/percona-server/5.5/diagnostics/innodb_show_status.html
     and INNODB_METRICS. we need to revisit those stats and either
     obsolete those that are already present in INNODB_METRICS or
     implement missing ones via INNODB_METRICS for consistency. Created
     https://blueprints.launchpad.net/percona-server/+spec/use-innodb-metrics-for-extended-stats

   - out of 63k lines diff, about 10k lines are javascript file diff in
     the doc/ directory. we have to do something about it.