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

Closed

Activities

wojciech.kapcia@tigase.net opened 1 year 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 1 year ago

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

Andrzej Wójcik (Tigase) commented 1 year 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 1 year 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 1 year 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 1 year 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 1 year ago

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

Andrzej Wójcik (Tigase) commented 1 year 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 1 year 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 1 year ago

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

wojciech.kapcia@tigase.net commented 1 year ago

Thank you.

wojciech.kapcia@tigase.net commented 1 year ago

Fix resolved the issue

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

wojciech.kapcia@tigase.net batch edited 4 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

tigase/_server/tigase-message-archiving#79 Closed

Exception thrown within StoredProcedure that has transaction can cause a lock
tigase/_server/tigase-unified-archive#14 You are not authorized to access this issue
tigase/_server/tigase-muc#154 Closed

Exception thrown within StoredProcedure that has transaction can cause a lock
Customers/carnow-inc#140 You are not authorized to access this issue
tigase-private/systems-maintenance/servers#384 You are not authorized to access this issue
tigase/_server/server-core#1347 Closed

Exception thrown within StoredProcedure that has transaction can cause a lock
tigase/_server/server-core#1348 Closed

Release Tigase Server 8.2.2
#126 Closed

Deadlocks in MySQL schema
Customers/carnow-inc#148 You are not authorized to access this issue

Issue Votes (0)

Watchers (2)

Reference

tigase/_server/tigase-pubsub#136