Merge pull request #9556 from bjester/morango-pg-transactions

Update morango and stop locking sync when db backend is postgres

Makefile

@@ -1,3 +1,5 @@
+SHELL := /bin/bash
+
 # List most target names as 'PHONY' to prevent Make from thinking it will be creating a file of the same name
 .PHONY: help clean clean-assets clean-build clean-pyc clean-docs lint test test-all assets coverage docs release test-namespaced-packages staticdeps staticdeps-cext writeversion setrequirements buildconfig pex i18n-extract-frontend i18n-extract-backend i18n-transfer-context i18n-extract i18n-django-compilemessages i18n-upload i18n-pretranslate i18n-pretranslate-approve-all i18n-download i18n-regenerate-fonts i18n-stats i18n-install-font i18n-download-translations i18n-download-glossary i18n-upload-glossary docker-whl docker-demoserver docker-devserver docker-envlist
 
@@ -29,9 +31,11 @@ help:
 	@echo "lint: check Python style with flake8"
 	@echo "test: run tests quickly with the default Python"
 	@echo "test-all: run tests on every Python version with Tox"
+	@echo "test-with-postgres: run tests quickly with a temporary postgresql backend"
 	@echo "test-namespaced-packages: verify that we haven't fetched anything namespaced into kolibri/dist"
 	@echo "coverage: run tests, recording and printing out Python code coverage"
 	@echo "docs: generate developer documentation"
+	@echo "start-foreground-with-postgres: run Kolibri in foreground mode with a temporary postgresql backend"
 	@echo ""
 	@echo "Internationalization"
 	@echo "--------------------"
@@ -90,6 +94,29 @@ test:
 test-all:
 	tox
 
+%-with-postgres:
+	@echo -e "\e[33mWARNING: for testing purposes only; postgresql database backend is ephemeral\e[0m"
+	@echo -e "\e[36mINFO: run 'docker-compose -v' to remove the database volume\e[0m"
+	export KOLIBRI_DATABASE_ENGINE=postgres; \
+	export KOLIBRI_DATABASE_NAME=default; \
+	export KOLIBRI_DATABASE_USER=postgres; \
+	export KOLIBRI_DATABASE_PASSWORD=postgres; \
+	export KOLIBRI_DATABASE_HOST=127.0.0.1; \
+	export KOLIBRI_DATABASE_PORT=5432; \
+	set -ex; \
+	function _on_interrupt() { docker-compose down; }; \
+	trap _on_interrupt SIGINT SIGTERM SIGKILL ERR; \
+	docker-compose up --detach; \
+	until docker-compose logs --tail=1 postgres | grep -q "database system is ready to accept connections"; do \
+		echo "$(date) - waiting for postgres..."; \
+		sleep 1; \
+	done; \
+	$(MAKE) -e $(subst -with-postgres,,$@); \
+	docker-compose down -v
+
+start-foreground:
+	kolibri start --foreground
+
 assets:
 	yarn install
 	yarn run build

docker-compose.yml

@@ -0,0 +1,16 @@
+version: '3.4'
+
+services:
+  postgres:
+    image: postgres:12
+    ports:
+      - "5432:5432"
+    environment:
+      POSTGRES_USER: postgres
+      POSTGRES_PASSWORD: postgres
+      POSTGRES_DB: default
+    volumes:
+      - kolibri-pg:/var/lib/postgresql/data
+#    command: "postgres -c log_statement=all -c log_line_prefix='%t %d '"
+volumes:
+  kolibri-pg:

kolibri/core/auth/management/commands/sync.py

@@ -23,7 +23,7 @@
 from kolibri.core.logger.utils.data import bytes_for_humans
 from kolibri.core.tasks.exceptions import UserCancelledError
 from kolibri.core.tasks.management.commands.base import AsyncCommand
-from kolibri.core.utils.lock import db_lock
+from kolibri.core.utils.lock import db_lock_sqlite_only
 from kolibri.utils import conf
 
 DATA_PORTAL_SYNCING_BASE_URL = conf.OPTIONS["Urls"]["DATA_PORTAL_SYNCING_BASE_URL"]
@@ -216,7 +216,12 @@ def _lock(self):
             cancellable = self.job.cancellable
             self.job.save_as_cancellable(cancellable=False)
 
-        with db_lock():
+        # Morango v0.6.13+ uses read repeatable isolation for postgres, and handles database
+        # transactions in a special manner that would be problematic if we began a transaction here.
+        # If data has changed during morango's transactions, postgres may throw a serialization
+        # rollback error, which will cause morango to retry it. So it's safe to avoid locking here
+        # when using postgres
+        with db_lock_sqlite_only():
             yield
 
         if self.job:

kolibri/core/auth/test/test_morango_integration.py

@@ -7,6 +7,7 @@
 import uuid
 
 from django.test import TestCase
+from django.test import TransactionTestCase
 from morango.models import InstanceIDModel
 from morango.models import Store
 from morango.models import syncable_models
@@ -44,7 +45,7 @@ def test_partition_and_id_values(self):
             self.assertTrue(partition.startswith(dataset_id))
 
 
-class DateTimeTZFieldTestCase(TestCase):
+class DateTimeTZFieldTestCase(TransactionTestCase):
     def setUp(self):
         self.controller = MorangoProfileController(PROFILE_FACILITY_DATA)
         InstanceIDModel.get_or_create_current_instance()

kolibri/core/utils/lock.py

@@ -30,8 +30,7 @@ def execute(self):
 
 
 @contextmanager
-def db_lock():
-    lock_id = 1
+def db_lock_sqlite_only():
     if connection.vendor == "sqlite":
         while True:
             try:
@@ -44,11 +43,30 @@ def db_lock():
             except OperationalError as e:
                 if "database is locked" not in str(e):
                     raise e
-    elif connection.vendor == "postgresql":
+    else:
+        yield
+
+
+@contextmanager
+def db_lock_postgresql_only():
+    lock_id = 1
+    if connection.vendor == "postgresql":
         with transaction.atomic():
             operation = PostgresLock(key=lock_id)
             operation.execute()
             yield
+    else:
+        yield
+
+
+@contextmanager
+def db_lock():
+    if connection.vendor == "sqlite":
+        with db_lock_sqlite_only():
+            yield
+    elif connection.vendor == "postgresql":
+        with db_lock_postgresql_only():
+            yield
     else:
         raise NotImplementedError(
             "kolibri.core.utils.cache.DatabaseLock not implemented for vendor {vendor}".format(

requirements/base.txt

@@ -20,7 +20,7 @@ kolibri_exercise_perseus_plugin==1.3.5
 jsonfield==2.0.2
 requests-toolbelt==0.8.0
 clint==0.5.1
-morango==0.6.13
+morango==0.6.14
 tzlocal==2.1
 pytz==2020.5
 python-dateutil==2.7.5