Andrei Elkin <andrei.elkin@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/287511a703f7dff479396334...
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.