Re: bzr commit into mysql-6.0-falcon-team branch (lars-erik.bjork:2899) Bug#40130

[Hakan Kuecuekyilmaz]

Lars-Erik,

patch looks good. Please see inline for minor improvements and
questions.

On Di, 2008-11-04 at 13:25 +0000, Lars-Erik.Bjork@stripped wrote:
> #At file:///home/lb200670/devel/mysql/nanna21/
> 
>  2899 lars-erik.bjork@stripped	2008-11-04
>       This is a commit for bug#40130 (Falcon date / time indexes broken)
>       
>       The bug was actually fixed by the patch to bug 40112 (Thanks
>       Kevin:) ). This commit only includes a regression test for the bug.
>       
>       Two files are added, these are:
>       
>       mysql-test/suite/falcon/t/falcon_bug_40130.test
>       mysql-test/suite/falcon/r/falcon_bug_40130.result
> added:
>   mysql-test/suite/falcon/r/falcon_bug_40130.result
>   mysql-test/suite/falcon/t/falcon_bug_40130.test
> 
[cut]

> === added file 'mysql-test/suite/falcon/t/falcon_bug_40130.test'
> --- a/mysql-test/suite/falcon/t/falcon_bug_40130.test	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/falcon/t/falcon_bug_40130.test	2008-11-04 13:24:47 +0000
> @@ -0,0 +1,101 @@
> +--source include/have_falcon.inc
> +
> +#
> +# Bug #40130: Falcon date / time indexes broken
> +#
> +--echo *** Bug #40130 ***
> +
> +# ----------------------------------------------------- #
> +# --- Initialisation                                --- #
> +# ----------------------------------------------------- #
> +let $engine = 'Falcon';
> +eval SET @@storage_engine = $engine;
> +
> +--disable_warnings
> +DROP TABLE IF EXISTS `table10`;
> +--enable_warnings
> +
> +# ----------------------------------------------------- #
> +# --- Test                                          --- #
> +# ----------------------------------------------------- #
> +
> +CREATE TABLE `table10` (`time_key` time, key (`time_key` ));
> +INSERT IGNORE INTO table10 VALUES ('23:43:55');
> +INSERT IGNORE INTO table10 VALUES ('03:18:59');
> +INSERT IGNORE INTO table10 VALUES ('05:05:23');
> +INSERT IGNORE INTO table10 VALUES ('09:20:40');
> +INSERT IGNORE INTO table10 VALUES ('22:32:50');
> +INSERT IGNORE INTO table10 VALUES ('07:41:31');
> +INSERT IGNORE INTO table10 VALUES ('10:52:13');
> +INSERT IGNORE INTO table10 VALUES ('12:40:54');
> +INSERT IGNORE INTO table10 VALUES ('10:33:25');
> +INSERT IGNORE INTO table10 VALUES ('22:11:46');
> +

What is the idea of INSERT IGNORE here?

> +SET AUTOCOMMIT=OFF;
> +
> +START TRANSACTION ; UPDATE `table10` SET `time_key` = '09:11:23' WHERE `time_key`
> < '16:23:56' ; INSERT INTO `table10` VALUES ( '20:25:18' ) ; COMMIT;
> +START TRANSACTION ; UPDATE `table10` SET `time_key` = '10:33:25' WHERE `time_key`
> > '22:11:46' ; INSERT INTO `table10` VALUES ( '17:58:48' ) ; COMMIT;
> +START TRANSACTION ; INSERT INTO `table10` VALUES ( '00:16:10' ) ; UPDATE `table10`
> SET `time_key` = '16:05:35' WHERE `time_key` > '16:57:24' ; COMMIT;

Can you please use one line per statement? That way future merges get
easier.

START TRANSACTION;
UPDATE `table10` SET `time_key` = '09:11:23' WHERE `time_key` <
'16:23:56';
INSERT INTO `table10` VALUES ( '20:25:18' );
COMMIT;

I see your point in easier readability, but I also see maintainability.
Think of a future change, which would involve a merge:

Change A:
- START TRANSACTION ; UPDATE `table10` SET `time_key` = '09:11:23'
+ START TRANSACTION; UPDATE `table10` SET `time_key` = '09:11:23'

Change B:
- START TRANSACTION ; UPDATE `table10` SET `time_key` = '09:11:23'
+ START TRANSACTION ; UPDATE `table10` SET `time_key` = '19:11:23'

Now when we try to merge changes A and B, we would get conflict. Having
every statement on a single line avoids such a situation:

Change A:
- START TRANSACTION ;
+ START TRANSACTION;

Change B:
- UPDATE `table10` SET `time_key` = '09:11:23'
+ UPDATE `table10` SET `time_key` = '19:11:23'

Brian Aker blogged about this a while ago in more detail:
"Coding Habits, What is on my mind while compiles are happening..."
    http://krow.livejournal.com/558188.html

[cut]


> +SELECT * FROM `table10`;

Would a SELECT count(*) FROM table10; do it, too?


> +START TRANSACTION ;UPDATE `table10` SET `time_key` = '00:28:04' WHERE `time_key`
> < '09:58:46' ; UPDATE `table10` SET `time_key` = '04:56:25' WHERE `time_key` >
> '09:26:08' ; COMMIT;
> +SELECT * FROM `table10`;

Same here: I would favor one statement per line.

[cut]

Best,

Hakan



-- 
Hakan Küçükyılmaz, Senior Software Engineer DBTG/MySQL +49 160
98953296
Sun Microsystems GmbH     Sonnenallee 1, DE-85551 Kirchheim-Heimstetten
Geschaeftsfuehrer:  Thomas Schroeder, Wolfang Engels, Dr. Roland Boemer
Vorsitz d. Aufs.rat.: Martin Haering   HRB MUC 161028     49.011, 8.376