Error when adding to ManyToManyField

ArthurNRL commented 2 years ago

Subject: ProgrammingError when adding to ManyToManyField

Problem

Attempting to add to a many to many relationship results on a not supported query on Redshift (ON CONFLICT)

Procedure to reproduce the problem

class CustomerUserGroup(AbstractBaseModel):
    id = models.UUIDField(primary_key=True, default=uuid4, editable=False, unique=True, db_column='customerusergroup_id')

class OwnerGroup(AbstractBaseModel):
    id = models.UUIDField(primary_key=True, default=uuid4, editable=False, unique=True, db_column='ownergroup_id')
    customer_user_group = models.ManyToManyField(CustomerUserGroup)

customer_group = CustomerUserGroup.objects.all()[0]
OwnerGroup.objects.all()[0].customer_user_group.add(group)

Same thing happens when trying to add from the admin site

Error logs / results

Traceback (most recent call last):
  File "C:\~\venv\lib\site-packages\django\db\backends\utils.py", line 89, in _execute
    return self.cursor.execute(sql, params)
psycopg2.errors.SyntaxError: syntax error at or near "ON"
LINE 1: ...5c74ff2ce84', 'cf6d2a7cc06846e488905f8ed8844822') ON CONFLIC...
                                                             ^
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
  File "C:\~\AppData\Local\Programs\Python\Python310\lib\threading.py", line 946, in run
    self._target(*self._args, **self._kwargs)
  File "C:\~\venv\lib\site-packages\django\utils\autoreload.py", line 64, in wrapper
    fn(*args, **kwargs)
  File "C:\~\venv\lib\site-packages\django\core\management\commands\runserver.py", line 125, in inner_run
    autoreload.raise_last_exception()
  File "C:\~\venv\lib\site-packages\django\utils\autoreload.py", line 87, in raise_last_exception
    raise _exception[1]
  File "C:\~\venv\lib\site-packages\django\core\management\__init__.py", line 398, in execute
    autoreload.check_errors(django.setup)()
  File "C:\~\venv\lib\site-packages\django\utils\autoreload.py", line 64, in wrapper
    fn(*args, **kwargs)
  File "C:\~\venv\lib\site-packages\django\__init__.py", line 24, in setup
    apps.populate(settings.INSTALLED_APPS)
  File "C:\~\venv\lib\site-packages\django\apps\registry.py", line 124, in populate
    app_config.ready()
  File "C:\~\venv\lib\site-packages\django\contrib\admin\apps.py", line 27, in ready
    self.module.autodiscover()
  File "C:\~\venv\lib\site-packages\django\contrib\admin\__init__.py", line 50, in autodiscover
    autodiscover_modules("admin", register_to=site)
  File "C:\~\venv\lib\site-packages\django\utils\module_loading.py", line 58, in autodiscover_modules
    import_module("%s.%s" % (app_config.name, module_to_search))
  File "C:\~\AppData\Local\Programs\Python\Python310\lib\importlib\__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1050, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1027, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1006, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 688, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 883, in exec_module
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "C:\~\statement\admin\__init__.py", line 3, in <module>
    from .owner_group_admin import CustomOwnerGroupAdmin
  File "C:\~\statement\admin\owner_group_admin.py", line 26, in <module>
    OwnerGroup.objects.all()[0].customer_user_group.add(group)
  File "C:\~\venv\lib\site-packages\django\db\models\fields\related_descriptors.py", line 1048, in add
    self._add_items(
  File "C:\~\venv\lib\site-packages\django\db\models\fields\related_descriptors.py", line 1269, in _add_items
    self.through._default_manager.using(db).bulk_create(
  File "C:\~\venv\lib\site-packages\django\db\models\query.py", line 579, in bulk_create
    returned_columns = self._batched_insert(
  File "C:\~\venv\lib\site-packages\django\db\models\query.py", line 1467, in _batched_insert
    self._insert(
  File "C:\~\venv\lib\site-packages\django\db\models\query.py", line 1434, in _insert
    return query.get_compiler(using=using).execute_sql(returning_fields)
  File "C:\~\venv\lib\site-packages\django\db\models\sql\compiler.py", line 1621, in execute_sql
    cursor.execute(sql, params)
  File "C:\~\venv\lib\site-packages\django\db\backends\utils.py", line 103, in execute
    return super().execute(sql, params)
  File "C:\~\venv\lib\site-packages\django\db\backends\utils.py", line 67, in execute
    return self._execute_with_wrappers(
  File "C:\~\venv\lib\site-packages\django\db\backends\utils.py", line 80, in _execute_with_wrappers
    return executor(sql, params, many, context)
  File "C:\~\venv\lib\site-packages\django\db\backends\utils.py", line 84, in _execute
    with self.db.wrap_database_errors:
  File "C:\~\venv\lib\site-packages\django\db\utils.py", line 91, in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
  File "C:\~\venv\lib\site-packages\django\db\backends\utils.py", line 89, in _execute
    return self.cursor.execute(sql, params)
django.db.utils.ProgrammingError: syntax error at or near "ON"
LINE 1: ...5c74ff2ce84', 'cf6d2a7cc06846e488905f8ed8844822') ON CONFLIC...

Expected results

New record(s) get added to the database

Environment info

OS: Windows 11
Python version: 3.10.2
Django version: 4.0.4
Django-Redshift-Backend version: 3.0.0

ArthurNRL commented 2 years ago

The full query: INSERT INTO "statement_ownergroup_customer_user_group" ("ownergroup_id", "customerusergroup_id") VALUES ('2df4b23614514f93847b45c74ff2ce84', 'cf6d2a7cc06846e488905f8ed8844822') ON CONFLICT DO NOTHING

ArthurNRL commented 2 years ago

Managed to fix it doing, although I'm not database savvy enough to know if this is safe or not

from django_redshift_backend.base import DatabaseOperations as RSDatabaseOperations, DatabaseWrapper as RSDatabaseWrapper

class DatabaseOperations(RSDatabaseOperations):

    def ignore_conflicts_suffix_sql(self, ignore_conflicts=None):
        return ''

class DatabaseWrapper(RSDatabaseWrapper):

    def __init__(self, *args, **kwargs):
        super(DatabaseWrapper, self).__init__(*args, **kwargs)
        self.ops = DatabaseOperations(self)

shimizukawa commented 2 years ago

Thanks for yoru reporting and inspection!

Upon further investigation, it appears that INSERT ON CONFLICT was introduced in Postgres 9.5; https://www.postgresql.org/docs/9.5/sql-insert.html . It also seems to be supported by the postgres backend in Django 2.2; https://github.com/django/django/commit/f1fbef6cd171ddfae41fcc901f1f60ccad039f51 .

Redshift is based on Postgres 8.0.2 and does not support INSERT ON CONFLICT. Therefore, INSERT ON CONFLICT must be disabled in some way.

I think your patch is a good solution because other backends that do not support ON CONFLICT simply return an empty string from ignore_conflicts_suffix_sql. The only other thing I would do is to set DatabaseFeatures.supports_ignore_conflicts = False .

Since I don't have time to fix it now, I will prepare a UnitTest to reproduce the problem, fix it, and confirm that it works in the next few weeks. If you could prepare a PR for me, that would be great ;)

ArthurNRL commented 2 years ago

I can do that for sure!

shimizukawa commented 6 hours ago

I would like to include this request/PR on #167.

jazzband / django-redshift-backend