Andrei Elkin <andrei.el...@mariadb.com> writes: >> So please do a small refactoring of trx_group_commit_leader() and keep >> only ... > > and no need for refactoring really arises.
Well, the refactoring is good, even without the MDEV-32014 patch. The function trx_group_commit_leader() is already too long and complicated, and does too much in a single function. And the first part (obtaining the queue) is cleanly separated from the second part (committing each entry in the queue in-order in binlog and participating engines). It's just nice that this improvement of the existing code also makes the MDEV-32014 patch integrate cleaner. > I noted earlier on this part on the PR page and LiBing seems to have > practically proven (by the most recent commit that you must've missed) that > conducting the rename inside the flush stage of > trx_group_commit_leader() is viable: > > https://github.com/MariaDB/server/pull/3402/commits/287511a703f7dff47939633475edd436a542cf6f#r1741144525 > > By that measure the flushing is expanded onto possibly multiple "implicitly" > rotating log files Hm, I don't like the change 287511a703f7dff47939633475edd436a542cf6f. In fact, that is one of the things I like the most about Libing Song's patch, that it so cleanly separates the commit-by-rotate logic from the complex group commit logic in queue_for_group_commit() and trx_group_commit_leader(), which is already too complex. The original patch without 287511a703f7dff47939633475edd436a542cf6f seems much cleaner. It is better to do as in the original patch, and treat commit-by-rotate like a normal binlog rotate, not try to participate that transaction in binlog group commit. We cannot share the fsync anyway, as the files are different. And it introduces a lot of complexities to suddenly have binlog rotations in the middle of group commit - what about the complex accounting around xid_count and binlog checkpoints? There will be a lot of corner cases eg. if _two_ commit-by-rotate happens in the same group commit, with lots of potential for subtle bugs that will occur only very rarely in production and be almost impossible to track down. I strongly prefer the original patch I reviewed (with my suggested changes to write_transaction_to_binlog_events() and trx_group_commit_leader()), where the commit-by-rotate logic is cleanly separated from the group commit logic and implemented mostly using the existing binlog rotate logic. - Kristian. _______________________________________________ developers mailing list -- developers@lists.mariadb.org To unsubscribe send an email to developers-le...@lists.mariadb.org
