From 94582264e98ae6b164701a2631e105d9886b8eae Mon Sep 17 00:00:00 2001 From: Adam Charnock Date: Tue, 28 May 2024 22:58:38 +0100 Subject: [PATCH 1/4] Work on performance improvement --- hordak/migrations/0041_auto_20240528_2107.py | 146 +++++++++++++++++++ hordak/tests/models/test_core.py | 32 +++- hordak/tests/utils.py | 9 +- 3 files changed, 182 insertions(+), 5 deletions(-) create mode 100644 hordak/migrations/0041_auto_20240528_2107.py diff --git a/hordak/migrations/0041_auto_20240528_2107.py b/hordak/migrations/0041_auto_20240528_2107.py new file mode 100644 index 00000000..3e1a5901 --- /dev/null +++ b/hordak/migrations/0041_auto_20240528_2107.py @@ -0,0 +1,146 @@ +# 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) + pass + + 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"), + ] + + operations = [ + migrations.RunPython(forward, reverse), + ] diff --git a/hordak/tests/models/test_core.py b/hordak/tests/models/test_core.py index f89121f1..b78957e2 100644 --- a/hordak/tests/models/test_core.py +++ b/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 diff --git a/hordak/tests/utils.py b/hordak/tests/utils.py index e13adca5..7dd8f482 100644 --- a/hordak/tests/utils.py +++ b/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, From 819a30e41499e827b85a6aa1959b1d460e516919 Mon Sep 17 00:00:00 2001 From: Adam Charnock Date: Wed, 29 May 2024 10:14:46 +0100 Subject: [PATCH 2/4] Work on mysql performance improvement --- hordak/migrations/0041_auto_20240528_2107.py | 31 +++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/hordak/migrations/0041_auto_20240528_2107.py b/hordak/migrations/0041_auto_20240528_2107.py index 3e1a5901..2c47801c 100644 --- a/hordak/migrations/0041_auto_20240528_2107.py +++ b/hordak/migrations/0041_auto_20240528_2107.py @@ -66,7 +66,35 @@ def forward(apps, schema_editor): elif schema_editor.connection.vendor == "mysql": # Performance improvement not implemented in mysql (a port is welcome) - pass + # 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() + 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 + ) + ; + END + """ + ) else: raise NotImplementedError( @@ -140,6 +168,7 @@ class Migration(migrations.Migration): dependencies = [ ("hordak", "0040_alter_account_name"), ] + atomic = False operations = [ migrations.RunPython(forward, reverse), From 7f61e86c7b9c65dc83be79c7b37febbb16bca708 Mon Sep 17 00:00:00 2001 From: Adam Charnock Date: Wed, 29 May 2024 11:03:04 +0100 Subject: [PATCH 3/4] mysql now has full performance improvements for update_full_account_codes() --- hordak/migrations/0006_auto_20161209_0108.py | 2 +- .../0032_check_account_type_big_int.py | 2 +- ...0039_recreate_update_full_account_codes.py | 2 +- hordak/migrations/0041_auto_20240528_2107.py | 6 ++- hordak/models/core.py | 46 ++++++++++--------- 5 files changed, 32 insertions(+), 26 deletions(-) diff --git a/hordak/migrations/0006_auto_20161209_0108.py b/hordak/migrations/0006_auto_20161209_0108.py index 584d5639..adecb99b 100644 --- a/hordak/migrations/0006_auto_20161209_0108.py +++ b/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) diff --git a/hordak/migrations/0032_check_account_type_big_int.py b/hordak/migrations/0032_check_account_type_big_int.py index 4046895d..39209517 100644 --- a/hordak/migrations/0032_check_account_type_big_int.py +++ b/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 diff --git a/hordak/migrations/0039_recreate_update_full_account_codes.py b/hordak/migrations/0039_recreate_update_full_account_codes.py index 9df682af..8bcb9290 100644 --- a/hordak/migrations/0039_recreate_update_full_account_codes.py +++ b/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( diff --git a/hordak/migrations/0041_auto_20240528_2107.py b/hordak/migrations/0041_auto_20240528_2107.py index 2c47801c..ab6217dd 100644 --- a/hordak/migrations/0041_auto_20240528_2107.py +++ b/hordak/migrations/0041_auto_20240528_2107.py @@ -41,7 +41,7 @@ def forward(apps, schema_editor): 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 + -- 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 ; @@ -70,7 +70,7 @@ def forward(apps, schema_editor): schema_editor.execute("DROP PROCEDURE update_full_account_codes") schema_editor.execute( """ - CREATE PROCEDURE update_full_account_codes() + CREATE PROCEDURE update_full_account_codes(changed_lft int, changed_rght int, changed_tree int) BEGIN UPDATE hordak_account AS a @@ -91,6 +91,8 @@ def forward(apps, schema_editor): 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 """ diff --git a/hordak/models/core.py b/hordak/models/core.py index 8a49f250..c3805d63 100644 --- a/hordak/models/core.py +++ b/hordak/models/core.py @@ -20,7 +20,6 @@ - ``StatementLine`` - Represents a statement line. ``StatementLine.create_transaction()`` may be called to create a transaction for the statement line. """ - from django.db import connection, models from django.db import transaction from django.db import transaction as db_transaction @@ -69,16 +68,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 +178,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 +539,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 +589,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 +807,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) From 528879bf4536c2a42679807a5811990a458d2e7b Mon Sep 17 00:00:00 2001 From: Adam Charnock Date: Wed, 29 May 2024 11:16:34 +0100 Subject: [PATCH 4/4] Code formatting fix --- hordak/models/core.py | 1 + 1 file changed, 1 insertion(+) diff --git a/hordak/models/core.py b/hordak/models/core.py index c3805d63..95e6900b 100644 --- a/hordak/models/core.py +++ b/hordak/models/core.py @@ -20,6 +20,7 @@ - ``StatementLine`` - Represents a statement line. ``StatementLine.create_transaction()`` may be called to create a transaction for the statement line. """ + from django.db import connection, models from django.db import transaction from django.db import transaction as db_transaction