September 2024 - developers - lists.mariadb.org

Clean up executable test result files:
by Otto Kekäläinen 04 Sep '24

04 Sep '24

Hi! In https://github.com/MariaDB/server/commit/9cb75f333fa2ae816909f95a4e19e6ddac… Alexander made some test result files executable (look at 100644 → 100755 in git diff). I sent twice a PR to fix this: - Oct 2023: https://github.com/MariaDB/server/pull/2779 - Feb 2024: https://github.com/MariaDB/server/pull/3042 Unfortunately, I can see that on the main branch this is executable again: mariadb-server main+ ± find mysql-test -executable -name *.test -or -executable -name *.result mysql-test/main/lowercase_table2.result Could the next person doing merges fix this tiny annoyance once and for all? Thanks :)

1 0

Re: [PATCH] MDEV-32014 Rename binlog cache temporary file to binlog file for large transaction
by Kristian Nielsen 03 Sep '24

03 Sep '24

Andrei Elkin <andrei.elkin(a)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/287511a703f7dff47939633… > > 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.

2 2

Re: [PATCH] MDEV-34857: Implement --slave-abort-blocking-timeout
by Michael Widenius 03 Sep '24

03 Sep '24

Review of MDEV-34857: Implement --slave-abort-blocking-timeout If a slave replicating an event has waited for more than @@slave_abort_blocking_timeout for a conflicting metadata lock held by a non-replication thread, the blocking query is killed to allow replication to proceed and not be blocked indefinitely by a user query. <cut> diff --git a/sql/mdl.cc b/sql/mdl.cc index faccd1c9476..9845718e165 100644 --- a/sql/mdl.cc +++ b/sql/mdl.cc <cut> @@ -2397,14 +2398,39 @@ MDL_context::acquire_lock(MDL_request *mdl_request, double lock_wait_timeout) find_deadlock(); - struct timespec abs_timeout, abs_shortwait; + struct timespec abs_timeout, abs_shortwait, abs_abort_blocking_timeout; + bool abort_blocking_enabled= false; + double abort_blocking_timeout= slave_abort_blocking_timeout; Add a comment here. Something like /* This code implements aborting of queries on the slaves that stops replication from continuing. */ + if (abort_blocking_timeout < lock_wait_timeout && + m_owner->get_thd()->rgi_slave) + { + set_timespec_nsec(abs_abort_blocking_timeout, + (ulonglong)(abort_blocking_timeout * 1000000000ULL)); + abort_blocking_enabled= true; + } set_timespec_nsec(abs_timeout, (ulonglong)(lock_wait_timeout * 1000000000ULL)); - set_timespec(abs_shortwait, 1); wait_status= MDL_wait::EMPTY; - while (cmp_timespec(abs_shortwait, abs_timeout) <= 0) + for (;;) { + bool abort_blocking= false; + set_timespec(abs_shortwait, 1); + if (abort_blocking_enabled && + cmp_timespec(abs_shortwait, abs_abort_blocking_timeout) >= 0) + { + /* + If a slave DDL has waited for --slave-abort-select-timeout, then notify + any blocking SELECT once before continuing to wait until the full + timeout. + */ + abs_shortwait= abs_abort_blocking_timeout; + abort_blocking= true; + abort_blocking_enabled= false; + } + if (cmp_timespec(abs_shortwait, abs_timeout) > 0) + break; Is the above right last compare right? Assuming that cmp_timespec(abs_shortwait, abs_abort_blocking_timeout) > 0 then the above will not break and we will not signal other threads to break even if the previous code expectrs us to abort blocking. (I think the cmp_timespec() return test should be the same in both cases) Shouldn't the code be: for (;;) { bool abort_blocking= false; set_timespec(abs_shortwait, 1); if (abort_blocking_enabled && cmp_timespec(abs_shortwait, abs_abort_blocking_timeout) >= 0) { /* If a slave DDL has waited for --slave-abort-select-timeout, then notify any blocking SELECT once before continuing to wait until the full timeout. */ abort_blocking= true; abort_blocking_enabled= false; } else if (cmp_timespec(abs_shortwait, abs_timeout) > 0) break; Or have I missed something? /* abs_timeout is far away. Wait a short while and notify locks. */ wait_status= m_wait.timed_wait(m_owner, &abs_shortwait, FALSE, mdl_request->key.get_wait_state_name()); @@ -2425,9 +2451,8 @@ MDL_context::acquire_lock(MDL_request *mdl_request, double lock_wait_timeout) mysql_prlock_wrlock(&lock->m_rwlock); if (lock->needs_notification(ticket)) - lock->notify_conflicting_locks(this); + lock->notify_conflicting_locks(this, abort_blocking); mysql_prlock_unlock(&lock->m_rwlock); - set_timespec(abs_shortwait, 1); I like the new code in that we are not setting abs_shortwait two times anymore. (Using a while loop in the is case was sub optimal. Ok to push after the above change or if you have a good argument why the current code is correct

1 0

Re: [PATCH] MDEV-32014 Rename binlog cache temporary file to binlog file for large transaction
by andrei.elkin＠pp.inet.fi 02 Sep '24

02 Sep '24

(Re-sending to the list) Howdy Kristian. > > The patch calls the trx_group_commit_leader() function. That is ackward, as > commit-by-rotate does not actually participate in group commit. But the > problem is with the existing function trx_group_commit_leader(), which does > too much in a single function. 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/287511a703f7dff47939633… By that measure the flushing is expanded onto possibly multiple "implicitly" rotating log files > > So please do a small refactoring of trx_group_commit_leader() and keep > only ... and no need for refactoring really arises. Could you please assess that approach, cheers, Andrei

1 0