lazychaser / laravel-nestedset

Effective tree structures in Laravel 4-8
3.64k stars 472 forks source link

This package is not concurrency safe #503

Open codercms opened 3 years ago

codercms commented 3 years ago

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 commented 2 years ago

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 commented 2 years ago

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

lazychaser commented 2 years ago

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 commented 1 year ago

Does someone have a patch for this ?

decadence commented 1 year ago

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();
    }
}