Code review comment for lp:~hrvojem/percona-server/rn-5.1.65-14.0

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

   In Features of RN, swap the changed page tracking and its I_S
   table, as the latter depends on the former.
   Instead "real incremental backups" (are the current incremental
   backups less real?) we have to be more specific and say "faster
   incremental backups that use this information to avoid full data
   "This table contains a list of pages marked as ``modified`` by log
   tracking thread." - is an implementation detail. Would be nicer
   something like "a list of modified pages from the modified page
   bitmap files produced by the log tracking thread."
   Bug 1039384 does not apply for 5.1. Its commit only contains
   expanded testcases.
   For bug 686392: s/implemente/implemented, s/rename/renames.
   For bug 686534: the mutex is periodically released in order not to
   block server while the dump is in progress.

   For changed page tracking docs, it's not a "reliability" but rather
   "management" feature IMHO, so should be placed accordingly.
   Instead of "InnoDB/XtraDB" we can just say "XtraDB", unless the
   former is consistent with other docs.

   In general I find the current description focusing too much on
   implementation specifics and not on the users. For introduction,
   "XtraDB now tracks the pages that have changes written to them
   according to the redo log. This information is written out in a
   special changed page bitmap file. This information can be used to
   speed up incremental backups using xb, by removing the need to scan
   whole data files to find the changed pages. This change tracking
   ois done by a new XtraDB worker thread that reads and parses log
   records between checkpoints."

   The current "implementation details" I think would only confuse,
   and use undefined terms like "last page flag". I'd rather omit
   this section for now and wait for confused users first that need
   this info.

   In I_S tables section, "This table contains a list of modified
   pages from the bitmap file data. As these files are generated by
   the log tracking thread parsing the log whenever the checkpoint is
   made, it is not real-time data"

   The start_lsn, end_lsn columns have wrong meanings. They actually
   mean "the change to this page happened at least once between these
   two LSNs". They are also equal to checkpoint LSNs.

review: Needs Fixing

« Back to merge proposal