valgrind warning from innodb_fake_changes patch

Testcase after porting this to the facebook patch -->

--source include/have_innodb_plugin.inc

--disable_warnings
DROP TABLE IF EXISTS t3;
--enable_warnings

CREATE TABLE t3 (a INT primary key, b text) ENGINE=InnoDB;
INSERT INTO t3 VALUES (1,'');

SET autocommit=1;
SET innodb_fake_changes=1;

--error ER_ERROR_DURING_COMMIT
UPDATE t3 set b=lpad('b',11000,'c') where a=1;

Valgrind stack trace for the warning after porting this to 5.1.52 + the facebook patch

==20223== Conditional jump or move depends on uninitialised value(s)
==20223== at 0x8E25B8: mach_write_to_3 (mach0data.ic:129)
==20223== by 0x8E2BA2: mach_write_to_7 (mach0data.ic:359)
==20223== by 0x9550C0: trx_write_roll_ptr (trx0undo.ic:124)
==20223== by 0x931747: row_upd_index_entry_sys_field (row0upd.c:397)
==20223== by 0x969D1B: btr_cur_pessimistic_update (btr0cur.c:2279)
==20223== by 0x934101: row_upd_clust_rec (row0upd.c:1761)
==20223== by 0x9348CF: row_upd_clust_step (row0upd.c:1972)
==20223== by 0x934B03: row_upd (row0upd.c:2050)
==20223== by 0x934DFE: row_upd_step (row0upd.c:2183)
==20223== by 0x91F7BB: row_update_for_mysql (row0mysql.c:1405)
==20223== by 0x8B5591: ha_innobase::update_row(unsigned char const*, unsigned char*) (ha_innodb.cc:5943)
==20223== by 0x7B01C9: handler::ha_update_row(unsigned char const*, unsigned char*) (handler.cc:5053)
==20223== by 0x714C6F: mysql_update(THD*, TABLE_LIST*, List<Item>&, List<Item>&, Item*, unsigned int, st_order*, unsigned long, enum_duplicates, bool) (sql_update.cc:656)
==20223== by 0x658748: mysql_execute_command(THD*, unsigned long long*, unsigned long long*) (sql_parse.cc:3473)
==20223== by 0x661C36: mysql_parse(THD*, char*, unsigned int, char const**, unsigned long long*, char*) (sql_parse.cc:6584)
==20223== by 0x652CBF: dispatch_command(enum_server_command, THD*, char*, unsigned int) (sql_parse.cc:1374)

Confirmed:

Run as:

./mysql-test-run.pl --valgrind --valgrind-option="--suppressions=$PWD/valgrind.supp" --valgrind-option='--show-reachable=yes' --valgrind-option='--gen-suppressions=all' --vardir=$HOME/mysql t/fake.test

where fake.test is

================================================

--source include/have_innodb.inc

--disable_warnings
DROP TABLE IF EXISTS t3;
--enable_warnings

CREATE TABLE t3 (a INT primary key, b text) ENGINE=InnoDB;
INSERT INTO t3 VALUES (1,'');

======================================================

Output:

*==12797== Conditional jump or move depends on uninitialised value(s)
 ==12797== at 0x87D672: row_upd_index_entry_sys_field (mach0data.ic:129)
 ==12797== by 0x8E6228: btr_cur_pessimistic_update (btr0cur.c:2445)
 ==12797== by 0x87AA52: row_upd_clust_rec (row0upd.c:2027)
 ==12797== by 0x883A16: row_upd_clust_step (row0upd.c:2275)
 ==12797== by 0x8844C0: row_upd_step (row0upd.c:2356)
 ==12797== by 0x85F7A8: row_update_for_mysql (row0mysql.c:1503)
 ==12797== by 0x83CD4F: ha_innobase::update_row(unsigned char const*, unsigned char*) (ha_innodb.cc:6237)
 ==12797== by 0x6B5CA3: handler::ha_update_row(unsigned char const*, unsigned char*) (handler.cc:5361)
 ==12797== by 0x613DE7: mysql_update(THD*, TABLE_LIST*, List<Item>&, List<Item>&, Item*, unsigned int, st_order*, unsigned long long, enum_duplicates, bool, unsigned long long*, unsigned long long*) (sql_update.cc:722)
 ==12797== by 0x59785B: mysql_execute_command(THD*) (sql_parse.cc:2940)
 ==12797== by 0x59C7E9: mysql_parse(THD*, char*, unsigned int, Parser_state*) (sql_parse.cc:5811)
 ==12797== by 0x59D289: dispatch_command(enum_server_command, THD*, char*, unsigned int) (sql_parse.cc:1061)
 ==12797== by 0x59E75E: do_command(THD*) (sql_parse.cc:788)
 ==12797== by 0x648060: do_handle_one_connection(THD*) (sql_connect.cc:1484)
 ==12797== by 0x648156: handle_one_connection (sql_connect.cc:1391)
 ==12797== by 0xA05AA1: pfs_spawn_thread (pfs.cc:1015)

Ignore the ssl related valgrind warnings, filing a separate bug for it. (for suppressing it)

I checked the latest bzr code and looks like adding '!trx->fake_changes' to the if condition below should fix it. However, the second row_upd_index_entry_sys_field may be required, so adding another if condition based on trx->fake_changes for first row_upd_index_entry_sys_field may be better.

 if (!(flags & BTR_KEEP_SYS_FLAG)) {
  row_upd_index_entry_sys_field(new_entry, index, DATA_ROLL_PTR,
           roll_ptr);
  row_upd_index_entry_sys_field(new_entry, index, DATA_TRX_ID,
           trx->id);
 }

Indeed the "&& !trx->fake_changes" is the correct fix. I briefly experimented with returning from the function even earlier in the case of fake changes, but the current code already returns as early as possible while having all outputs set.

@Raghu, the second row_upd_index_entry_sys_field is not required with the fake changes neither.

Percona now uses JIRA for bug reports so this bug report is migrated to: https://jira.percona.com/browse/PS-1223