Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

This package is not concurrency safe #503

Open
codercms opened this issue Mar 26, 2021 · 5 comments
Open

This package is not concurrency safe #503

codercms opened this issue Mar 26, 2021 · 5 comments

Comments

@codercms
Copy link

codercms commented Mar 26, 2021

Because this package relies on calculations on the code side, the tree will always be in a broken state when simultaneously inserts are performed (duplicates by _lft will occur).
I think this behavior should be documented.

To ensure concurrency safety this package should perform all calculations on the database side with row locking during update.
For concurrency safe inserts, the following queries should be used (when parent_id is set):

BEGIN;

--
-- lock rows for update
--
WITH parent AS ( -- select parent _lft here
    SELECT _lft
    FROM goals
    WHERE
        id = :parent_id
        AND scope_attr1 = :scope_attr1 -- AND other scope attributes
)
SELECT 1
FROM goals
WHERE
    scope_attr1 = :scope_attr1  -- AND other scope attributes
    AND (_rgt > (SELECT _lft FROM parent) OR _lft > (SELECT _lft FROM parent))
ORDER BY id -- you have to ORDER BY by sequential value (if your PK is uuid you can order by created_at + id, or make a surrogate key)
FOR UPDATE;

--
-- make tree gap
--
WITH parent AS (
    SELECT _lft
    FROM goals
    WHERE
        id = :parent_id
        AND scope_attr1 = :scope_attr1 -- AND other scope attributes
)
UPDATE goals
SET
    _lft = CASE
        WHEN _lft > (SELECT _lft FROM parent)
        THEN _lft + 2
        ELSE _lft
    END,
    _rgt = CASE
        WHEN _rgt > (SELECT _lft FROM parent)
        THEN _rgt + 2
        ELSE _rgt
    END
WHERE
    scope_attr1 = :scope_attr1  -- AND other scope attributes
    AND (_rgt > (SELECT _lft FROM parent) OR _lft > (SELECT _lft FROM parent))

--
-- insert row
--
INSERT INTO goals (
    -- your list of columns here,
    parent_id,
    _lft,
    _rgt
)
SELECT -- by using the INSERT ... SELECT statement we can get actual data of the parent node
     -- your list of values to insert here
    :parent_id,
    _lft + 1, -- node _lft = _lft from parent + 1
    _lft + 2  -- node _rgt = _lft from parent + 2
FROM goals
WHERE
    id = :parent_id
    AND scope_attr1 = :scope_attr1  -- AND other scope attributes
RETURNING *;

COMMIT;

If parent_id is not set, the following queries should be used:

BEGIN;

WITH max_rgt AS (
    SELECT COALESCE(MAX(_rgt), 0) AS _rgt
    FROM goals
    WHERE scope_attr1 = :scope_attr1  -- AND other scope attributes
)
INSERT INTO goals (
    -- your list of columns here,
    parent_id,
    _lft,
    _rgt
)
SELECT
     -- your list of values to insert here
    NULL,
    (SELECT _rgt + 1 FROM max_rgt) AS _lft,
    (SELECT _rgt + 2 FROM max_rgt) AS _rgt
RETURNING *;

COMMIT;

Refs:

@domihagen
Copy link

Would be great if this is getting done. We currently have a lot of Deadlock issues:

SQLSTATE[HY000]: General error: 1205 Lock wait timeout exceeded; try restarting transaction (SQL: update `pages` set `_lft` = case when `_lft` between 41 and 42 then `_lft`-7 when `_lft` between 34 and 42 then `_lft`+2 else `_lft` end, `_rgt` = case when `_rgt` between 41 and 42 then `_rgt`-7 when `_rgt` between 34 and 42 then `_rgt`+2 else `_rgt` end where (`_lft` between 34 and 42 or `_rgt` between 34 and 42))

@codercms
Copy link
Author

@lazychaser what do you think about this issue? This issue affects all users.
Do you have any plans for implementation?

@lazychaser
Copy link
Owner

Please use other locking mechanics like RedLock when performing updates on nodes to avoid dead locks.
This package will only be updated to the latest versions of Laravel, no more features are added.

@Nuranto
Copy link

Nuranto commented Jun 18, 2023

Does someone have a patch for this ?

@decadence
Copy link

decadence commented Sep 1, 2023

Can this be workaround? At least for broken trees, not deadlocks.

Laravel Command that runs every minute or more often (latest Laravel supports Sub-minute Scheduling)

class FixTree extends Command
{
    public function handle()
    {
        if(!Category::isBroken()) {
            return;   
        }

        Category::fixTree();
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants