Account code regeneration major performance improvement

hordak/migrations/0006_auto_20161209_0108.py

@@ -45,7 +45,7 @@ def create_trigger(apps, schema_editor):
         )
 
     elif schema_editor.connection.vendor == "mysql":
-        # we have to call this procedure in Leg.on_commit, because MySQL does not support deferred triggers
+        # we have to call this procedure in python via mysql_simulate_trigger(), because MySQL does not support deferred triggers
         schema_editor.execute(
             """
             CREATE OR REPLACE PROCEDURE check_leg(_transaction_id INT)

hordak/migrations/0032_check_account_type_big_int.py

@@ -22,7 +22,7 @@ def create_trigger(apps, schema_editor):
         )
 
     elif schema_editor.connection.vendor == "mysql":
-        # we have to call this procedure in Leg.on_commit, because MySQL does not support deferred triggers
+        # we have to call this procedure in python via mysql_simulate_trigger(), because MySQL does not support deferred triggers
         schema_editor.execute(
             """
             CREATE OR REPLACE TRIGGER check_account_type_on_insert

hordak/migrations/0039_recreate_update_full_account_codes.py

@@ -28,7 +28,7 @@ def create_trigger(apps, schema_editor):
         )
 
     elif schema_editor.connection.vendor == "mysql":
-        # we have to call this procedure in Leg.on_commit, because MySQL does not support deferred triggers
+        # we have to call this procedure in python via mysql_simulate_trigger(), because MySQL does not support deferred triggers
         pass
     else:
         raise NotImplementedError(

hordak/migrations/0041_auto_20240528_2107.py

@@ -0,0 +1,177 @@
+# Generated by Django 5.0.6 on 2024-05-28 21:07
+
+from django.db import migrations
+
+
+def forward(apps, schema_editor):
+    if schema_editor.connection.vendor == "postgresql":
+        # Performance improvement, functionality unchanged
+        schema_editor.execute(
+            "DROP TRIGGER update_full_account_codes_trigger ON hordak_account"
+        )
+        schema_editor.execute(
+            """
+            CREATE OR REPLACE FUNCTION update_full_account_codes()
+                RETURNS TRIGGER AS
+            $$
+            BEGIN
+                -- Update all children's full account codes
+
+                -- Only do this if the account code has been changed
+                IF OLD.code != NEW.code OR (OLD.code IS NULL) != (NEW.code IS NULL) THEN
+                    -- Useful for debugging, check the postgres logs to see output
+                    -- RAISE WARNING 'Left %%, Right %%, tree %%, %%|%% -> %%|%%', NEW.lft, NEW.rght, NEW.tree_id, OLD.name, OLD.code, NEW.name, NEW.code;
+
+                    UPDATE
+                        hordak_account AS a
+                    SET
+                        full_code = (
+                            SELECT CASE
+                                -- Aggregate the codes of all parents to make the full account code.
+                                -- However, if this path includes NULL values then the full code must be set to null.
+                                -- Checking for the existence of NULLS in an array is not that easy in postgres
+                                -- (https://stackoverflow.com/q/22695015/764723) so we do a little tick to keep things
+                                -- sane and readable. We simply compare the stringified version of the array
+                                -- against one that has NULL values replaced with '*'. If they match then we are free of any NULL
+                                -- values.
+                                WHEN array_to_string(array_agg(nullif(code, '')), '', '') = array_to_string(array_agg(nullif(code, '')), '', '*')
+                                    THEN string_agg(code, '' order by lft)
+                                ELSE NULL
+                            END
+                            FROM hordak_account AS a2
+                            WHERE a2.lft <= a.lft AND a2.rght >= a.rght AND a.tree_id = a2.tree_id
+                        )
+                    -- Only update this account and the accounts below it in the tree
+                    WHERE a.lft >= NEW.lft AND a.rght <= NEW.rght AND a.tree_id = NEW.tree_id
+                    ;
+
+                END IF;
+
+                RETURN NULL;
+            END;
+            $$
+            LANGUAGE plpgsql;
+        """
+        )
+        # The function above requires 'FOR EACH ROW' on the trigger
+        schema_editor.execute(
+            """
+            CREATE TRIGGER update_full_account_codes_trigger
+            AFTER INSERT OR UPDATE OR DELETE ON hordak_account
+            FOR EACH ROW
+            WHEN (pg_trigger_depth() = 0)
+            EXECUTE PROCEDURE update_full_account_codes();
+        """
+        )
+
+    elif schema_editor.connection.vendor == "mysql":
+        # Performance improvement not implemented in mysql (a port is welcome)
+        # But we do fix an issue that surfaced
+        schema_editor.execute("DROP PROCEDURE update_full_account_codes")
+        schema_editor.execute(
+            """
+            CREATE PROCEDURE update_full_account_codes(changed_lft int, changed_rght int, changed_tree int)
+            BEGIN
+                UPDATE
+                    hordak_account AS a
+                SET
+                    full_code = (
+                        SELECT CASE
+                            -- Aggregate the codes of all parents to make the full account code.
+                            -- However, if this path includes NULL values then the full code must be set to null.
+                            -- Checking for the existence of NULLS in an array is not that easy in mariadb
+                            -- so we do a little tick to keep things
+                            -- sane and readable. We simply compare the stringified version of the array
+                            -- against one that has NULL values replaced with '*'. If they match then we are free of any NULL
+                            -- values.
+                            WHEN GROUP_CONCAT(IFNULL(NULLIF(code, ''), '*')) = GROUP_CONCAT(IFNULL(NULLIF(code, ''), ''))
+                                THEN GROUP_CONCAT(code ORDER BY lft SEPARATOR '')
+                            ELSE NULL
+                        END
+                        FROM hordak_account AS a2
+                        WHERE a2.lft <= a.lft AND a2.rght >= a.rght AND a.tree_id = a2.tree_id
+                    )
+                -- Only update this account and the accounts below it in the tree
+                WHERE a.lft >= changed_lft AND a.rght <= changed_rght AND a.tree_id = changed_tree
+                ;
+            END
+        """
+        )
+
+    else:
+        raise NotImplementedError(
+            "Don't know how to create trigger for %s" % schema_editor.connection.vendor
+        )
+
+
+def reverse(apps, schema_editor):
+    if schema_editor.connection.vendor == "postgresql":
+        # Recreate update_full_account_codes as it was in migration 0027
+        schema_editor.execute(
+            "DROP TRIGGER update_full_account_codes_trigger ON hordak_account"
+        )
+        schema_editor.execute(
+            """
+            CREATE OR REPLACE FUNCTION update_full_account_codes()
+                RETURNS TRIGGER AS
+            $$
+            BEGIN
+                -- Set empty string codes to be NULL
+                UPDATE hordak_account SET code = NULL where code = '';
+
+                -- Set full code to the combination of the parent account's codes
+                UPDATE
+                    hordak_account AS a
+                SET
+                    full_code = (
+                        SELECT string_agg(code, '' order by lft)
+                        FROM hordak_account AS a2
+                        WHERE a2.lft <= a.lft AND a2.rght >= a.rght AND a.tree_id = a2.tree_id AND code IS NOT NULL
+                    )
+                    WHERE tree_id IN (SELECT DISTINCT tree_id FROM hordak_account WHERE code IS NOT NULL);  -- search only account trees without null codes
+
+
+                -- Set full codes to NULL where a parent account includes a NULL code
+                UPDATE
+                    hordak_account AS a
+                SET
+                    full_code = NULL
+                WHERE
+                    (
+                        SELECT COUNT(*)
+                        FROM hordak_account AS a2
+                        WHERE a2.lft <= a.lft AND a2.rght >= a.rght AND a.tree_id = a2.tree_id AND a2.code IS NULL
+                    ) > 0
+                    AND full_code IS NOT NULL;  -- search only account trees without null codes
+                RETURN NULL;
+            END;
+            $$
+            LANGUAGE plpgsql;
+        """
+        )
+        schema_editor.execute(
+            """
+            CREATE TRIGGER update_full_account_codes_trigger
+            AFTER INSERT OR UPDATE OR DELETE ON hordak_account
+            WHEN (pg_trigger_depth() = 0)
+            EXECUTE PROCEDURE update_full_account_codes();
+        """
+        )
+    elif schema_editor.connection.vendor == "mysql":
+        # the triggers will have to be called within django again...
+        schema_editor.execute("DROP PROCEDURE update_full_account_codes")
+    else:
+        raise NotImplementedError(
+            "Don't know how to drop trigger for %s" % schema_editor.connection.vendor
+        )
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("hordak", "0040_alter_account_name"),
+    ]
+    atomic = False
+
+    operations = [
+        migrations.RunPython(forward, reverse),
+    ]

hordak/models/core.py

@@ -69,16 +69,6 @@ def get_by_natural_key(self, uuid):
         return self.get(uuid=uuid)
 
 
-def _enforce_account():
-    with connection.cursor() as curs:
-        # postgresql has this enforced by a trigger, but MySQL/MariaDB does not support deferred constraint
-        # triggers, and does not support triggers updating the table they are triggered from
-        # so we have to do it by calling a procedure here instead
-        # (https://stackoverflow.com/a/15300941/1908381)
-        if connection.vendor == "mysql":
-            curs.callproc("update_full_account_codes")
-
-
 class Account(MPTTModel):
     """Represents an account
 
@@ -189,7 +179,14 @@ def save(self, *args, **kwargs):
                 "currencies",
             ]
         super(Account, self).save(*args, update_fields=update_fields, **kwargs)
-        transaction.on_commit(_enforce_account)
+
+        if connection.vendor == "mysql":
+            # We need updated lft/rght/tree_id values for the mysql_run_manual_trigger() call
+            self.refresh_from_db()
+
+        mysql_simulate_trigger(
+            "update_full_account_codes", self.lft, self.rght, self.tree_id
+        )
 
         do_refresh = False
 
@@ -543,14 +540,6 @@ def credits(self):
 CustomLegManager = LegManager.from_queryset(LegQuerySet)
 
 
-def _enforce_leg(transaction_id: int):
-    with connection.cursor() as curs:
-        # postgresql has this enforced by a trigger, but MySQL/MariaDB does not support deferred constraint
-        # triggers so we have to do it by calling a procedure here instead
-        if connection.vendor == "mysql":
-            curs.callproc("check_leg", [transaction_id])
-
-
 class Leg(models.Model):
     """The leg of a transaction
 
@@ -601,7 +590,7 @@ def save(self, *args, **kwargs):
             raise exceptions.ZeroAmountError()
 
         leg = super(Leg, self).save(*args, **kwargs)
-        transaction.on_commit(lambda: _enforce_leg(transaction_id=self.transaction_id))
+        mysql_simulate_trigger("check_leg", self.transaction_id)
         return leg
 
     def natural_key(self):
@@ -819,3 +808,19 @@ def create_transaction(self, to_account):
 
     class Meta:
         verbose_name = _("statementLine")
+
+
+def mysql_simulate_trigger(proc_name, *args):
+    # MySQL/MariaDB does not support deferred constraint triggers (unlike postgres),
+    # and also does not support triggers updating the table they are triggered from.
+    # So this function allows us to trigger manual function calls on transaction finish.
+    # Enforcing this at the application level is not idea. If this is important to you
+    # then use postgres.
+    # (https://stackoverflow.com/a/15300941/1908381)
+    def _mysql_call_proc():
+        with connection.cursor() as curs:
+            curs.callproc(proc_name, args)
+
+    if connection.vendor == "mysql":
+        with connection.cursor():
+            transaction.on_commit(_mysql_call_proc)

hordak/tests/models/test_core.py

@@ -426,7 +426,7 @@ def test_full_code_error_on_non_unique(self):
         with self.assertRaises(DatabaseError):
             self.account(parent=account1, code="0")
 
-    def test_full_code_changes_on_update(self):
+    def test_full_code_changes_on_update_simple(self):
         account1 = self.account(code="5")
         account2 = self.account(parent=account1, code="0")
         account3 = self.account(parent=account2, code="9")
@@ -445,6 +445,7 @@ def test_full_code_changes_on_update(self):
         self.assertEqual(account3.full_code, "A09")
 
     def test_full_code_changes_on_update_with_null_code(self):
+        account0 = self.account(code="1")
         account1 = self.account(code="5")
         account2 = self.account(parent=account1, code="0")
         account3 = self.account(parent=account2, code="9")
@@ -455,11 +456,13 @@ def test_full_code_changes_on_update_with_null_code(self):
         account2.refresh_from_db()
         account3.refresh_from_db()
 
+        self.assertEqual(account0.full_code, "1")
         self.assertEqual(account1.full_code, None)
         self.assertEqual(account2.full_code, None)
         self.assertEqual(account3.full_code, None)
 
     def test_full_code_changes_on_update_with_empty_string_code(self):
+        account0 = self.account(code="1")
         account1 = self.account(code="5")
         account2 = self.account(parent=account1, code="0")
         account3 = self.account(parent=account2, code="9")
@@ -470,10 +473,37 @@ def test_full_code_changes_on_update_with_empty_string_code(self):
         account2.refresh_from_db()
         account3.refresh_from_db()
 
+        self.assertEqual(account0.full_code, "1")
         self.assertEqual(account1.full_code, None)
         self.assertEqual(account2.full_code, None)
         self.assertEqual(account3.full_code, None)
 
+    def test_full_code_changes_on_create_with_null_code(self):
+        account1 = self.account(code="5")
+        account2 = self.account(parent=account1, code=None)
+        account3 = self.account(parent=account2, code="9")
+
+        account1.refresh_from_db()
+        account2.refresh_from_db()
+        account3.refresh_from_db()
+
+        self.assertEqual(account1.full_code, "5")
+        self.assertEqual(account2.full_code, None)
+        self.assertEqual(account3.full_code, None)
+
+    def test_full_code_changes_on_create_with_empty_string_code(self):
+        account1 = self.account(code="5")
+        account2 = self.account(parent=account1, code="")
+        account3 = self.account(parent=account2, code="9")
+
+        account1.refresh_from_db()
+        account2.refresh_from_db()
+        account3.refresh_from_db()
+
+        self.assertEqual(account1.full_code, "5")
+        self.assertEqual(account2.full_code, None)
+        self.assertEqual(account3.full_code, None)
+
     def test_child_asset_account_can_be_bank_account(self):
         """Regression test for: #Postgres check bank_accounts_are_asset_accounts
         does not work on child bank accounts

hordak/tests/utils.py

@@ -5,6 +5,9 @@
 from hordak.utilities.currency import Balance
 
 
+Empty = object()
+
+
 class DataProvider(object):
     """Utility methods for providing data to test cases"""
 
@@ -36,7 +39,7 @@ def account(
         name=None,
         parent=None,
         type=Account.TYPES.income,
-        code=None,
+        code=Empty,
         currencies=("EUR",),
         **kwargs
     ):
@@ -46,9 +49,7 @@ def account(
             Account
         """
         name = name or "Account {}".format(Account.objects.count() + 1)
-        code = (
-            code if code is not None else Account.objects.filter(parent=parent).count()
-        )
+        code = Account.objects.filter(parent=parent).count() if code is Empty else code
 
         return Account.objects.create(
             name=name,