[Tagging] Reduce the number of queries used in content tagging REST API

[pomegranited]

"As a content author, I need content tagging APIs to respond quickly and efficiently, so that I don't waste time waiting for the UIs to load."

As part of openedx/edx-platform#34004 we had tests that verified that the number of query counts made by the content taxonomy REST APIs were not increased by adding user permissions to the response. This revealed that the content tagging REST APIs make too many queries when compiling their responses:

• 16 queries to list the taxonomies enabled for an org (compared to 5 queries from the equivalent oel_tagging library endpoint.)
• 7 queries to retrieve object tags for an object (compared to 1 query for oel_tagging).
• 13 queries to search for tags under a parent (compared to 5 queries for oel_tagging)

A likely cause for these discrepancies is the org-based rules logic, and permission checks being run too often. But there may also be query sets that would benefit from an added select_related.

See also this comment about cross-org tagging and taxonomy admins -- address this issue too.

Acceptance criteria

• Query counts for content tagging REST APIs reduced (ideally O(1) like their oel_tagging equivalents).
• Tests which validate the above.

[pomegranited]

FYI @bradenmacdonald

[rpenido]

7 queries to retrieve object tags for an object (compared to 1 query for oel_tagging).

I tested this with different users ("content_creatorA", "instructorA", "library_staffA", etc..) and the number goes to 19-25. Also, it seems that is a cache between the tests because this number varies between tests (the first user gets 25 calls an the next ones 19)

[pomegranited]

@rpenido @bradenmacdonald I was able to make some small improvements on the "taxonomy list" endpoint today, but there's still way too many queries happening:

E   Captured queries were:
E   1. SELECT "django_site"."id", "django_site"."domain", "django_site"."name" FROM "django_site" WHERE "django_site"."domain" LIKE 'testserver' ESCAPE '\' LIMIT 21
E   2. SELECT "student_userstanding"."id", "student_userstanding"."user_id", "student_userstanding"."account_status", "student_userstanding"."changed_by_id", "student_userstanding"."standing_last_changed_at" FROM "student_userstanding" WHERE "student_userstanding"."user_id" IS NULL LIMIT 21
E   3. SELECT "theming_sitetheme"."id", "theming_sitetheme"."site_id", "theming_sitetheme"."theme_dir_name" FROM "theming_sitetheme" WHERE "theming_sitetheme"."site_id" = 1 ORDER BY "theming_sitetheme"."id" ASC LIMIT 1
E   4. SAVEPOINT "s140245987477312_x30"
E   5. SELECT COUNT(*) AS "__count" FROM "oel_tagging_taxonomy" WHERE ("oel_tagging_taxonomy"."enabled" AND EXISTS(SELECT 1 AS "a" FROM "content_tagging_taxonomyorg" U0 LEFT OUTER JOIN "organizations_organization" U2 ON (U0."org_id" = U2."id") WHERE (U0."rel_type" = 'OWN' AND U0."taxonomy_id" = ("oel_tagging_taxonomy"."id") AND (U0."org_id" IS NULL OR U2."short_name" = 'orgA')) LIMIT 1))
E   6. SELECT "oel_tagging_taxonomy"."id", "oel_tagging_taxonomy"."name", "oel_tagging_taxonomy"."description", "oel_tagging_taxonomy"."enabled", "oel_tagging_taxonomy"."allow_multiple", "oel_tagging_taxonomy"."allow_free_text", "oel_tagging_taxonomy"."visible_to_authors", "oel_tagging_taxonomy"."_taxonomy_class" FROM "oel_tagging_taxonomy" WHERE ("oel_tagging_taxonomy"."enabled" AND EXISTS(SELECT 1 AS "a" FROM "content_tagging_taxonomyorg" U0 LEFT OUTER JOIN "organizations_organization" U2 ON (U0."org_id" = U2."id") WHERE (U0."rel_type" = 'OWN' AND U0."taxonomy_id" = ("oel_tagging_taxonomy"."id") AND (U0."org_id" IS NULL OR U2."short_name" = 'orgA')) LIMIT 1)) ORDER BY "oel_tagging_taxonomy"."name" ASC, "oel_tagging_taxonomy"."id" ASC LIMIT 4
E   7. SELECT "content_tagging_taxonomyorg"."id", "content_tagging_taxonomyorg"."taxonomy_id", "content_tagging_taxonomyorg"."org_id", "content_tagging_taxonomyorg"."rel_type" FROM "content_tagging_taxonomyorg" WHERE "content_tagging_taxonomyorg"."taxonomy_id" IN (3, 5, 7, 11)
E   8. SELECT "oel_tagging_tag"."id", "oel_tagging_tag"."taxonomy_id", "oel_tagging_tag"."parent_id", "oel_tagging_tag"."value", "oel_tagging_tag"."external_id" FROM "oel_tagging_tag" WHERE "oel_tagging_tag"."taxonomy_id" IN (3, 5, 7, 11)
E   9. SELECT COUNT(*) AS "__count" FROM "oel_tagging_tag" WHERE "oel_tagging_tag"."taxonomy_id" = 3
E   10. SELECT "content_tagging_taxonomyorg"."id", "content_tagging_taxonomyorg"."taxonomy_id", "content_tagging_taxonomyorg"."org_id", "content_tagging_taxonomyorg"."rel_type" FROM "content_tagging_taxonomyorg" WHERE "content_tagging_taxonomyorg"."taxonomy_id" = 3
E   11. SELECT 1 AS "a" FROM "content_tagging_taxonomyorg" WHERE ("content_tagging_taxonomyorg"."taxonomy_id" = 3 AND "content_tagging_taxonomyorg"."org_id" IS NULL) LIMIT 1
E   12. SELECT 1 AS "a" FROM "content_tagging_taxonomyorg" WHERE ("content_tagging_taxonomyorg"."taxonomy_id" = 5 AND "content_tagging_taxonomyorg"."org_id" IS NULL) LIMIT 1
E   13. SELECT "organizations_organization"."id", "organizations_organization"."created", "organizations_organization"."modified", "organizations_organization"."name", "organizations_organization"."short_name", "organizations_organization"."description", "organizations_organization"."logo", "organizations_organization"."active" FROM "organizations_organization" WHERE "organizations_organization"."id" = 1 LIMIT 21
E   14. SELECT 1 AS "a" FROM "content_tagging_taxonomyorg" WHERE ("content_tagging_taxonomyorg"."taxonomy_id" = 7 AND "content_tagging_taxonomyorg"."org_id" IS NULL) LIMIT 1
E   15. SELECT "organizations_organization"."id", "organizations_organization"."created", "organizations_organization"."modified", "organizations_organization"."name", "organizations_organization"."short_name", "organizations_organization"."description", "organizations_organization"."logo", "organizations_organization"."active" FROM "organizations_organization" WHERE "organizations_organization"."id" = 1 LIMIT 21
E   16. SELECT "organizations_organization"."id", "organizations_organization"."created", "organizations_organization"."modified", "organizations_organization"."name", "organizations_organization"."short_name", "organizations_organization"."description", "organizations_organization"."logo", "organizations_organization"."active" FROM "organizations_organization" WHERE "organizations_organization"."id" = 2 LIMIT 21
E   17. SELECT 1 AS "a" FROM "content_tagging_taxonomyorg" WHERE ("content_tagging_taxonomyorg"."taxonomy_id" = 11 AND "content_tagging_taxonomyorg"."org_id" IS NULL) LIMIT 1
E   18. RELEASE SAVEPOINT "s140245987477312_x30"
E   19. SELECT "user_api_userpreference"."id", "user_api_userpreference"."user_id", "user_api_userpreference"."key", "user_api_userpreference"."value" FROM "user_api_userpreference" WHERE ("user_api_userpreference"."key" = 'pref-lang' AND "user_api_userpreference"."user_id" = 2) LIMIT 21

• query 8 shows the taxonomy prefetch_related('taxonomyorg_set') that happens in the list view.
• So 10, 11, 12, 14, 17 should be pulling from the prefetched cache, but it isn't.
  (Also confirmed by using django-query-debug : it printed warnings like this: "Accessing uncached reverse ManyToOne field Taxonomy.taxonomyorg"; debugging showed an empty instance prefetch cache.)
• I suspect the issue is with our many-to-many TaxonomyOrg model -- maybe since we can't have a ManyToMany field on the Taxonomy model, the prefetch caching isn't working?

I can work on the prefetch caching, but that could take some time.. Do you have any other ideas I can try?

[rpenido]

I suspect the issue is with our many-to-many TaxonomyOrg model -- maybe since we can't have a ManyToMany field on the Taxonomy model, the prefetch caching isn't working?

I think one culprit is related to our cast() method.

class TaxonomySerializer(UserPermissionsSerializerMixin, serializers.ModelSerializer):
    """
    Serializer for the Taxonomy model.
    """
    # ...
    def to_representation(self, instance):
        """
        Cast the taxonomy before serialize
        """
        instance = instance.cast()
        return super().to_representation(instance)

class Taxonomy(models.Model):
    # ...
    def cast(self):
        """
        Returns the current Taxonomy instance cast into its taxonomy_class.

        If no taxonomy_class is set, or if we're unable to import it, then it just returns self.
        """
        try:
            TaxonomyClass = self.taxonomy_class
            if TaxonomyClass and not isinstance(self, TaxonomyClass):
                return TaxonomyClass().copy(self)
        except ImportError:
            # Log error and continue
            log.exception(
                f"Unable to import taxonomy_class for {self}: {self._taxonomy_class}"
            )

        return self
    # ...
    def copy(self, taxonomy: Taxonomy) -> Taxonomy:
        """
        Copy the fields from the given Taxonomy into the current instance.
        """
        self.id = taxonomy.id
        self.name = taxonomy.name
        self.description = taxonomy.description
        self.enabled = taxonomy.enabled
        self.allow_multiple = taxonomy.allow_multiple
        self.allow_free_text = taxonomy.allow_free_text
        self.visible_to_authors = taxonomy.visible_to_authors
        self._taxonomy_class = taxonomy._taxonomy_class  # pylint: disable=protected-access
        return self

We are losing the cache the way we copy the Taxonomy. In my case, this reduced 2 queries (from the SystemDefinedTaxonomies). In your case, query 9.

11, 12, 14, and 16 seem related to our UserOrgFilterBackend, and the get_relationships methods
Edit: I think it is because of the issue that Braden found with get_all_orgs()

[rpenido]

I can work on the prefetch caching, but that could take some time.. Do you have any other ideas I can try?

We got a good start here. I don't know if we can do this with O(1), but we are not that bad if it's O(N), with the number of taxonomies. I think we don't have O(taxonomy*org), right?

Awesome investigation here @pomegranited! Good work!

I think we should also check this with our most common user profile. Unfortunately, the staff is one of our best-case scenarios.

[bradenmacdonald]

query 8 shows the taxonomy prefetch_related('taxonomyorg_set') that happens in the list view.

I think you mean query 7? 7. SELECT "content_tagging_taxonomyorg". ...

Query 8 actually looks totally unnecessary to me - why is it loading tag details for a "taxonomy list" endpoint? As far as I know, we don't include any actual tag data in the response. 8. SELECT "oel_tagging_tag"."id", "oel_tagging_tag"."taxonomy_id", "oel_tagging_tag"."parent_id", "oel_tagging_tag"."value", "oel_tagging_tag"."external_id" FROM "oel_tagging_tag" WHERE "oel_tagging_tag"."taxonomy_id" IN (3, 5, 7, 11)

I'm also unclear on why queries 8,9,10 are only happening for taxonomy 3 and not the other taxonomies in the response.

11/12/14/17 are weird... they are checking if org_id=None (org=None) but they are not checking rel_type. Everywhere in the code that I could find which filters TaxonomyOrg on org also requires a specific value of rel_type. Checking org_id=None without also checking rel_type is a bug.

[rpenido]

I'm also unclear on why queries 8,9,10 are only happening for taxonomy 3 and not the other taxonomies in the response.

3 is probably a system-defined taxonomy, and we are calling .cast() on it (and losing the prefetched cache).

[rpenido]

11/12/14/17 are weird... they are checking if org_id=None (org=None) but they are not checking rel_type. Everywhere in the code that I could find which filters TaxonomyOrg on org also requires a specific value of rel_type. Checking org_id=None without also checking rel_type is a bug.

Good catch!
The code is here: https://github.com/openedx/edx-platform/blob/ab6793ef29104aa42ca9a7a33ba246c39ad38094/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py#L92-L96

[pomegranited]

We progress! Have reduced the number of queries executed by the "taxonomy list" endpoint from 23 to 11!

@rpenido you called it when you said:

I think one culprit is related to our cast() method.

I'm now copying the prefetch cache when casting, and that dropped 2 queries.

I think you mean query 7? 7. SELECT "content_tagging_taxonomyorg". ...

Oops, yes.

Query 8 actually looks totally unnecessary to me - why is it loading tag details for a "taxonomy list" endpoint? As far as I know, we don't include any actual tag data in the response.

We include the tags_count, and so to support this, we prefetch the tag_set. But now that I write this, there's probably a trickier but better performing way to do this, since that can be a lot of data.

Checking org_id=None without also checking rel_type is a bug.

Agreed, fixed here.

Since we're prefetching the taxonomyorg_set, and there's not likely to be many of these for a given taxonomy, I've updated the code to iterate over them instead of queryset filtering and triggering another query.

The remaining queries look valid to me:

E   Captured queries were:
E   1. SELECT "django_site"."id", "django_site"."domain", "django_site"."name" FROM "django_site" WHERE "django_site"."domain" LIKE 'testserver' ESCAPE '\' LIMIT 21
E   2. SELECT "student_userstanding"."id", "student_userstanding"."user_id", "student_userstanding"."account_status", "student_userstanding"."changed_by_id", "student_userstanding"."standing_last_changed_at" FROM "student_userstanding" WHERE "student_userstanding"."user_id" IS NULL LIMIT 21
E   3. SELECT "theming_sitetheme"."id", "theming_sitetheme"."site_id", "theming_sitetheme"."theme_dir_name" FROM "theming_sitetheme" WHERE "theming_sitetheme"."site_id" = 1 ORDER BY "theming_sitetheme"."id" ASC LIMIT 1
E   4. SAVEPOINT "s139664326948672_x1414"
E   5. SELECT COUNT(*) AS "__count" FROM "oel_tagging_taxonomy" WHERE ("oel_tagging_taxonomy"."enabled" AND EXISTS(SELECT 1 AS "a" FROM "content_tagging_taxonomyorg" U0 LEFT OUTER JOIN "organizations_organization" U2 ON (U0."org_id" = U2."id") WHERE (U0."rel_type" = 'OWN' AND U0."taxonomy_id" = ("oel_tagging_taxonomy"."id") AND (U0."org_id" IS NULL OR U2."short_name" = 'orgA')) LIMIT 1))
E   6. SELECT "oel_tagging_taxonomy"."id", "oel_tagging_taxonomy"."name", "oel_tagging_taxonomy"."description", "oel_tagging_taxonomy"."enabled", "oel_tagging_taxonomy"."allow_multiple", "oel_tagging_taxonomy"."allow_free_text", "oel_tagging_taxonomy"."visible_to_authors", "oel_tagging_taxonomy"."export_id", "oel_tagging_taxonomy"."_taxonomy_class" FROM "oel_tagging_taxonomy" WHERE ("oel_tagging_taxonomy"."enabled" AND EXISTS(SELECT 1 AS "a" FROM "content_tagging_taxonomyorg" U0 LEFT OUTER JOIN "organizations_organization" U2 ON (U0."org_id" = U2."id") WHERE (U0."rel_type" = 'OWN' AND U0."taxonomy_id" = ("oel_tagging_taxonomy"."id") AND (U0."org_id" IS NULL OR U2."short_name" = 'orgA')) LIMIT 1)) ORDER BY "oel_tagging_taxonomy"."name" ASC, "oel_tagging_taxonomy"."id" ASC LIMIT 4
E   7. SELECT "content_tagging_taxonomyorg"."id", "content_tagging_taxonomyorg"."taxonomy_id", "content_tagging_taxonomyorg"."org_id", "content_tagging_taxonomyorg"."rel_type" FROM "content_tagging_taxonomyorg" WHERE "content_tagging_taxonomyorg"."taxonomy_id" IN (3, 5, 7, 11)
E   8. SELECT "organizations_organization"."id", "organizations_organization"."created", "organizations_organization"."modified", "organizations_organization"."name", "organizations_organization"."short_name", "organizations_organization"."description", "organizations_organization"."logo", "organizations_organization"."active" FROM "organizations_organization" WHERE "organizations_organization"."id" IN (2, 1)
E   9. SELECT "oel_tagging_tag"."id", "oel_tagging_tag"."taxonomy_id", "oel_tagging_tag"."parent_id", "oel_tagging_tag"."value", "oel_tagging_tag"."external_id" FROM "oel_tagging_tag" WHERE "oel_tagging_tag"."taxonomy_id" IN (3, 5, 7, 11)
E   10. RELEASE SAVEPOINT "s139664326948672_x1414"
E   11. SELECT "user_api_userpreference"."id", "user_api_userpreference"."user_id", "user_api_userpreference"."key", "user_api_userpreference"."value" FROM "user_api_userpreference" WHERE ("user_api_userpreference"."key" = 'pref-lang' AND "user_api_userpreference"."user_id" = 2) LIMIT 21