Exception thrown within StoredProcedure that has transaction can cause a lock (#136)

Activities

wojciech.kapcia@tigase.net opened 2 years ago

Execution concurrently same stored procedure that has transaction inside leads to lock.

Adding exit handler to stored procedures that have the transactions solves the issue:

-- DO NOT REMOVE, required for properly handle exceptions within transactions!
DECLARE exit handler for sqlexception
    BEGIN
        -- ERROR
        ROLLBACK;
        RESIGNAL;
    END;

@andrzej.wojcik please check remaining components (muc, MA, UA, MIX) for stored procedures that are still used and use transactions and add same fix (and please include the comment to not remove the handler).

It would be ideal to have a test for this case (possibly unit-test, but it would be problematic)

To be backported to Tigase Server 8.2.2

Some context: https://stackoverflow.com/a/60230093/211453:

When you open a transaction, and a query inside it fails, the transaction keeps open, it does not commit nor rollback the changes.

So BE CAREFUL, any table/row that was locked with a previous query like SELECT ... FOR SHARE/UPDATE, UPDATE, INSERT or any other locking-query, keeps locked until that session is killed (and executes a rollback), or until a following query commits it explicitly (COMMIT) or implicitly, thus making the partial changes permanent (which might happen hours later, while the transaction was in a waiting state).

That's why the solution involves declaring handlers to immediately ROLLBACK when an error happens.

MySQL has a timeout for locks: innodb_lock_wait_timeout, which by default is 50s

Andrzej Wójcik (Tigase) commented 2 years ago

Fixes applied. https://github.com/tigase/tigase-pubsub/pull/21

Andrzej Wójcik (Tigase) commented 2 years ago

I've updated -dist project to include new versions of MA & UA (server, pubsub and muc were using SNAPSHOTS already).

wojciech.kapcia@tigase.net commented 2 years ago

Merged, let's wait for the TTS-NG results and backport it if evenrything is OK (though, this one would be good candidate to test repositories with TestContainers).

Curiously enough, we had those handlers (without RESIGNAL; though), but they were removed in #issue #126.

wojciech.kapcia@tigase.net commented 2 years ago

The tests passed (https://tc.tigase.net/buildConfiguration/TigaseTtsNgTests_MySQL/173170). Could you backport it to tigase-server-8.2.x (it had issue #5.0.0)

Andrzej Wójcik (Tigase) commented 2 years ago

Handlers without resignal were catching exceptions and returning to the application like the statement (stored procedure) was executed correctly and that caused invalid state in the application level cache.

Andrzej Wójcik (Tigase) commented 2 years ago

Should I backport only fixes for PubSub or all the applicable fixes for version 8.2.x?

Andrzej Wójcik (Tigase) commented 2 years ago

I've created a new branch in tigase-pubsub 5.0.x-bugfix that contains ported changes to MySQL schema. I've added empty SQL files for other databases as I think they are required. I think this is all that is required to backport those changes, but please review them as I've not tested them (just copied the whole file from 5.2.0 and stripped unnecessary procedures from 5.2.0 as there were not other changes related to those procedures, beside adding handlers, since 5.0.0).

Is there anything else to be done? I'm wondering if we will need to release 5.1.x bugfix as well.. as 5.0.1 would be superior to 5.1.0 in this area.. but simple merge from 5.0.x will not be sufficient as those changes would not be loaded - files should be renamed to 5.1.1 if I recall correctly.

wojciech.kapcia@tigase.net commented 2 years ago

We can simplify branch naming, i.e. tigase-issue #5.0.x (just like in server, i.e. https://github.com/tigase/tigase-server-distribution/branches tigase-server-8.2.x, tigase-server-8.3.x, etc )
it seems that TigPubSubWriteItem() misses the handler declaration?
As per chat discussion, having it in the server in procedure related to updating user password may be a "good to have" as it's used extensively in this particular case.

Andrzej Wójcik (Tigase) commented 2 years ago

I've applied mentioned changes, renamed branch, fixed procedure and backported changes for tigase-server

wojciech.kapcia@tigase.net commented 2 years ago

Thank you.

wojciech.kapcia@tigase.net commented 2 years ago

Fix resolved the issue

wojciech.kapcia@tigase.net added "Related" Customers/carnow-inc#148 12 months ago

wojciech.kapcia@tigase.net batch edited 6 months ago

Name	Previous Value	Current Value
Iterations	empty	tigase-server-8.4.0

Type	Bug
Priority	Normal
Assignee	wojciech.kapcia@tigase.net
Version	tigase-server-8.4.0, tigase-server-8.2.2

Iterations

tigase-server-8.4.0 Closed

Issue Votes (0)

Watchers (2)

Reference

tigase/_server/tigase-pubsub#136