MDEV-34466 XA prepare don't release unmodified records for some cases

[vlad-lesin]

There is no need to exclude exclusive non-gap locks from the procedure
of locks releasing on XA PREPARE execution in
lock_release_on_prepare_try() after commit
17e59ed3aad481918f1a01d8afbc071c316d5930 (MDEV-33454), because
lock_rec_unlock_unmodified() should check if the record was modified
with the XA, and release the lock if it was not.

lock_release_on_prepare_try(): don't skip X-locks, let
lock_rec_unlock_unmodified() to process them.

lock_sec_rec_some_has_impl(): add template parameter for not acquiring
trx_t::mutex for the case if a caller already holds the mutex, don't
crash if lock's bitmap is clean.

row_vers_impl_x_locked(), row_vers_impl_x_locked_low(): add new argument
to skip trx_t::mutex acquiring.

rw_trx_hash_t::validate_element(): don't acquire trx_t::mutex if the
current thread already holds it.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[dr-m]

The changes look OK, I only have some minor nitpicks. I would suggest to write
commit 17e59ed3aad481918f1a01d8afbc071c316d5930 (MDEV-33454)
in the commit message so that it is a little more convenient to find the change that it is referring to. In the GitHub web user interface the 17e59ed should be displayed as a hyperlink.

Once you have addressed these, @mariadb-DebarunBanerjee should review this.

[mariadb-DebarunBanerjee]

Hi Vladislav,
Thanks for the patch. I have reviewed the code and included my comments. The design level comments are from my general DB/MySQL understanding and I may not be completely correct with MariaDB. I still think it is worth to point them out for your consideration.

As this code is enabling the major part of the base MDEV work, we should consider testing the feature well for impacts.

It could have significant performance impact in Primary while checking for 2ndary index implicit lock especially with concurrent load modifying index records.

Since this early release of locks hardly helps the primary (rather impacts the performance/concurrency), we should evaluate how much it indeed helps the replica with scenarios where it is effective.

For row level replication, the replica should be updating the rows that are modified directly and the locks acquired during range scanning for update in Primary should not be there while replaying. So, I think there are only very specific cases, like unique index insert S locks where the optimization for releasing S lock early is really useful.

For statement level replication, I think releasing S locks early has a functional issue related to transaction ordering. Statement level replication is essentially dependent on Serializability (Note that RR is serializable when only DMLs are considered) and releasing S lock at XA PREPARE impacts the ordering.I would try to repeat it with mtr and update.

[mariadb-DebarunBanerjee]

This seems to be the crux of the patch. To make sure we are on same plane, let me first state the logic, as it stands today, of,releasing locks at XA prepare.

1. Release all record locks in bitmap only if it is GAP only (X + LOCK_GAP). [!rec_granted_exclusive_not_gap]
2. Otherwise release only the supremum record lock if supremum is part of bitmap.

This is the condition in lock_release_on_prepare_try, which actually can release only very few locks excluding all the S locks. The function lock_rec_unlock_unmodified() is completely unreachable from lock_release_on_prepare_try.

The lock_rec_unlock_unmodified is reachable from "lock_release_on_prepare()" where we do call it when [1] & [2] are not satisfied Unfortunately, lock_release_on_prepare_try() would also set all_released =T after releasing only minimal number of locks unless it fails to acquire the lock sys latch.

I have two points to include here.

1. With this context, the patch makes lock_release_on_prepare_try() logic similar to lock_release_on_prepare() and should be correct assuming the base design is ok.

2. Assuming we do intend to release all locks for which we haven't actually modified the record, even after the patch, we would miss the set of records for which the the lock also includes supremum record.

I will add a separate point on the base design which we need to evaluate too as the core part was vastly disabled before this patch.

[dr-m]

My comments are about the MDEV-34690 patch.

I see that there is some rather similar code between lock_release_on_prepare_try() and lock_release_on_prepare() for releasing the record locks, but there seem to be enough differences so that the similar code can’t be refactored to a separate function.

[dr-m]

It could be a better idea to remove have_debug.inc from the test file and filter out all debug-only parameters from the output. In that way, this test could be run on all targets.

[vlad-lesin]

Why do we need to run the test for all targets? It just checks the presence of system variables, have_debug.inc gives more coverage for that. We don't have release-only variables, so we don't need a separate run for release build.