Code review comment for lp:~akopytov/percona-server/bug1131189

Revision history for this message
Laurynas Biveinis (laurynas-biveinis) wrote :

It's a good patch, just couple of relatively small questions and comments:

   - The only thing I'd look into doing differently would be not
     doing linear search at all in trx_reserve_descriptor(), and just
     adding a debug assertion that the array is sorted after the
     insert (which would be just the comparison with the n-1th
     element). Would save a dozen code bytes in a presumably very hot
     code path.

   - I must be missing something here, but why there is padding
     inserted between descr_n_max and descr_n_used in trx_sys? It
     seems that these fields are accessed at close times.

   - File read0read.h adds the include of trx0sys.h. But the diff of
     read0read.h does not show anything new external used there.
     Rather, it is read0read.ic which starts using
     trx_find_descriptor(), thus the trx0sys.h include should be
     moved to read0read.ic.

   - Spurious whitespace changes in diff lines 381, 382, 645,
     1155-1157, 1220.

   - Why does trx_reserve_decriptor() loop: n_used++; while (n_used >
     n_max) n_max *= 2 ? A second iteration of this loop can never
     happen, it looks maybe a bit too defensive for me,
     and this is next to a very hot code path, isn't it? Since there
     is if (n_used > n_max) just above, one unconditional n_max *= 2;
     should be enough.

   - Why does trx_free_prepared() free its view now?

review: Needs Information

« Back to merge proposal