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

...2013/5/19 Alexey Kopytov <email address hidden>:
> Review: Needs Information
> Hi Laurynas. Time is a precious resource these days, I only did a
> superficial review, but here goes.

Thank you for your time.

> 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:

I am pushing lp:~laurynas-biveinis/percona-server/5.6-merge-1-fixes
with these items. From commit messages:

> - 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'

Post-review fixes for the lp:percona-server/5.5 rev 397 merge to

- Revert the spurious copyright comment changes, so that they match
  the ones at the current lp:mysql-server/5.6. Do not touch the ones
  that are fixed in the subsequent 5.6.11 merge.

> - 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

This one I did on purpose in fact. WARN_NO_MASTER_INFO seemed like the
most logical thing to return when slave is not running. Reverted.

Post-review fixes for the lp:percona-server/5.5 rev 397 merge to

- Revert the change introduced at the porting where SHOW SLAVE STATUS
  would check whether the slave is running, and returning
  WARN_NO_MASTER_INFO warning if it's not. Re-record
  disabled_replication test to match.

> - debug fprintf()s in sys_vars.h:
> - all UNIV_SRV_PRINT_LATCH_WAITS code has been removed upstream, but
> we are reintroducing it for no apparent reasons

Post-review fixes for the lp:percona-server/5.5 rev 397 merge to

- Remove introduced-by-mistake debugging printf()s in sys_vars.h.

- Fix Percona InnoDB version in univ.i.

- Remove obsolete UNIV_SRV_PRINT_LATCH_WAITS code.

> - 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

I didn't track it down completely, but I did track it to the MEMORY
VARCHAR support. I guess the query optimizer sees the VARCHAR support
in a temp table even without full BLOB temp table support in the
optimizer. The relevant test snippet is

--echo #
--echo # Bug#12799731 - CRASH IN END_READ_RECORD.
--echo #
create table t1(f1 char(255) charset utf8);
insert into t1 values('1'),('2'),('3'),('4'),('5'),('6'),('7'),('8'),('9'),('0');
set @save_heap_size= @@max_heap_table_size;
set @@max_heap_table_size= 1;
flush status;
select count(*) from t1 join (
  select t1.f1 from t1 join t1 as t2 join t1 as t3) tt on t1.f1 = tt.f1;
--echo Should be greater than 1000 as it also includes records dumped from
--echo heap to myisam.
show status like 'Handler_write';
set @@max_heap_table_size= @save_heap_size;
drop table t1;

'Handler_write' > 1000 is after the change too, so seems something
engine-specific to me and I decided not to proceed with the analysis.
Please let me know if I should.

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

This is a bug, a combination of together with
our fix for
Will fix post-merge, 5.5 is affected too.

> - 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
> Was it removed in previous upstream releases, but we decided to
> keep it? Do we actually run it in MTR/Jenkins?

This question is best directed at Oracle :) It seems that their intent
is to keep it empty or even non-existing publicly, but sometimes a
stray commit happens there which is usually removed for the next
release. I am ignoring it (i.e. going along with what bzr merge does)
for this merge.

> 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 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.

Converted to,
and a note in

> - wrong comment alignment in (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 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

Yes, this branch (or is it the merge-3 one) already implemented this
partially for the low hanging fruits. This was not implemented fully
because it became too time-intensive to be a priority at the time to
chase all the refactored defines from 5.5 to 5.6 (apparently I did not
stumble upon DICT_TF_HAS_ATOMIC_BLOBS). Converted to

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

Converted to

> - man pages should be updated from upstream 5.6 tarballs

Converted to

> - 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

Yes, added a note to

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

Converted to

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


> - there seems to be much overlap between
> 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

Good point, will discuss with Vadim.

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

Yes. Also we will integrate TokuDB somehow at some point, which will
require source repo integration as well.

To summarize:
- the "Needs Fixing" issues are being addressed in a follow-up branch.
Then the merge order becomes: merge-1, merge-1-fixes, 5.6.11, merge-3.
As soon as merge-1-fixes is approved, I will push the branches to
trunk in this order as they are being approved;
- the "Needs Information" issues need your OK or additional converting
to bug reports;
- the rest have been converted to bug db.


« Back to merge proposal