Opened 10 months ago

Closed 10 months ago

Last modified 10 months ago

#34779 closed Bug (fixed)

Serializer makes unnecessary joins

Reported by: Juan Alvarez Owned by: Juan Alvarez
Component: Core (Serialization) Version: 4.2
Severity: Release blocker Keywords:
Cc: Martin Svoboda Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When handling M2M fields, the serializer would make unnecessary joins, as it has an empty select_related. Its default behavior is to select all related fields. This can even trigger a recursive addition of related fields, that end up in an OperationalError because of too many joins.

The regression seems to have been introduced in https://code.djangoproject.com/ticket/34620

Change History (9)

comment:1 by Juan Alvarez, 10 months ago

Owner: changed from nobody to Juan Alvarez
Status: newassigned

comment:2 by Juan Alvarez, 10 months ago

Has patch: set
Type: UncategorizedBug

comment:3 by Natalia Bidart, 10 months ago

Resolution: needsinfo
Status: assignedclosed

Hello Juan, thanks for your report.

Do you think you could provide a reproducer? A small django test project, or a set of models exercising the issue, or a failing test case. That way we can triage the issue more accurately.

Natalia.

in reply to:  3 comment:4 by Juan Alvarez, 10 months ago

Replying to Natalia Bidart:

Hello Juan, thanks for your report.

Do you think you could provide a reproducer? A small django test project, or a set of models exercising the issue, or a failing test case. That way we can triage the issue more accurately.

Natalia.

Natalia,

For reference, I did submit a potential fix at https://github.com/django/django/pull/17164

Actually, the existing test did _kind_ of covered the regression. The issue was that since the model had null=True, the empty select_related() did not use the field, because of:

def select_related_descend(field, restricted, requested, select_mask, reverse=False):
    # [...]
    if not restricted and field.null:
        return False

So this change:

  • tests/serializers/models/base.py

    diff --git a/tests/serializers/models/base.py b/tests/serializers/models/base.py
    index e9f548ad3c..c5a4a0f580 100644
    a b class TopicManager(models.Manager):  
    6060
    6161class Topic(models.Model):
    6262    name = models.CharField(max_length=255)
    63     category = models.ForeignKey(Category, models.CASCADE, null=True)
     63    category = models.ForeignKey(Category, models.CASCADE)
    6464    objects = TopicManager()

Makes this test fail:

    def test_serialize_only_pk(self):
        with self.assertNumQueries(7) as ctx:
            serializers.serialize(
                self.serializer_name,
                Article.objects.all(),
                use_natural_foreign_keys=False,
            )

        categories_sql = ctx[1]["sql"]
        self.assertNotIn(connection.ops.quote_name("meta_data_id"), categories_sql)
        meta_data_sql = ctx[2]["sql"]
        self.assertNotIn(connection.ops.quote_name("kind"), meta_data_sql)
        topics_data_sql = ctx[3]["sql"]
        self.assertNotIn(connection.ops.quote_name("category_id"), topics_data_sql)

because there is a join to category_id when Topic.category is not nullable.

If you consider this not sufficient, I can try to come up with a more complete example. But this issue is breaking our application, and the proposed fix does fix it.

comment:5 by Juan Alvarez, 10 months ago

Resolution: needsinfo
Status: closednew

comment:6 by Mariusz Felisiak, 10 months ago

Cc: Martin Svoboda added
Component: UncategorizedCore (Serialization)
Severity: NormalRelease blocker
Status: newassigned
Triage Stage: UnreviewedAccepted

Good catch! Thanks for the report and patch.

comment:7 by Mariusz Felisiak, 10 months ago

Triage Stage: AcceptedReady for checkin

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 10 months ago

Resolution: fixed
Status: assignedclosed

In 517d3bb4:

Fixed #34779 -- Avoided unnecessary selection of non-nullable m2m fields without natural keys during serialization.

By using select_related(None) instead of select_related(), the
unnecessary joins are completely avoided. Note that the current tests
already covers the change, when the field is not null=True.

Regression in f9936deed1ff13b20e18bd9ca2b0750b52706b6c.

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 10 months ago

In 46b2b08e:

[4.2.x] Fixed #34779 -- Avoided unnecessary selection of non-nullable m2m fields without natural keys during serialization.

By using select_related(None) instead of select_related(), the
unnecessary joins are completely avoided. Note that the current tests
already covers the change, when the field is not null=True.

Regression in f9936deed1ff13b20e18bd9ca2b0750b52706b6c.

Backport of 517d3bb4dd17e9c51690c98d747b86a0ed8b2fbf from main

Note: See TracTickets for help on using tickets.
Back to Top