Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Performance optimization: list endpoints slow due to permission checks #1365

Closed
stevenbal opened this issue Apr 18, 2023 · 3 comments · Fixed by #1405 or #1406
Closed

Performance optimization: list endpoints slow due to permission checks #1365

stevenbal opened this issue Apr 18, 2023 · 3 comments · Fixed by #1405 or #1406
Assignees
@stevenbal @annashamray
Labels
standard: zaken 1.6 topic: performance
Milestone
Release 1.9

Comments

@stevenbal
Copy link
Contributor

stevenbal commented Apr 18, 2023 •
edited by annashamray
Loading

Master US - #1344
Taiga issue 454

Performance testing showed that several list endpoints are slowed down if a JWT without superuser permissions:

  • GET openzaak.components.zaken.api.viewsets.RolViewSet 
  • GET openzaak.components.zaken.api.viewsets.ZaakInformatieObjectViewSet 
  • GET openzaak.components.zaken.api.viewsets.StatusViewSet

Notes:

  • determining the count for pagination seems to be the bottleneck here as well, in this case because the query contains an INNER JOIN on the zaken_zaak table, which seems to reduce performance significantly. I tested locally with a simple query: SELECT COUNT(*) FROM "zaken_rol" INNER JOIN "zaken_zaak" ON ("zaken_rol"."zaak_id" = "zaken_zaak"."identificatie_ptr_id"), this query takes around 800ms on my machine

Upd. by Anna
The optimization is done in several steps (and PRs):

  1. use only "pk" in pagination count for all viewsets - PR ⚡ optimize pagination count #1405 - OZ 1.9
  2. optimize authorization filtering (initially developed in PR [DRAFT] ⚡ [#1365] Performance improvements for permissions check #1368, then cherry picked into new PR Feature/1365 auth filter #1406) - OZ 1.9
  3. introduce fuzzy pagination - [DRAFT] ⚡ [#1365] Performance improvements for permissions check #1368 - planned for OZ 1.11
@stevenbal stevenbal self-assigned this Apr 18, 2023
@stevenbal stevenbal changed the title Performance optimization: EnkelvoudigInformatieObject list endpoint Performance optimization: list endpoints slow due to permission checks Apr 18, 2023
@stevenbal stevenbal mentioned this issue Apr 18, 2023
Closed
@stevenbal
Copy link
Contributor Author

I looked into this, I noticed that removing ordering from the authorization query improves performance by quite a bit, but it still remains fairly slow due to the inner join it seems

stevenbal added a commit that referenced this issue Apr 18, 2023
@stevenbal
⚡ [#1365] Remove ordering from auth filter query
a75c744
stevenbal added a commit that referenced this issue May 8, 2023
@stevenbal
⚡ [#1365] Remove ordering from auth filter query
c4bbea6
stevenbal added a commit that referenced this issue May 8, 2023
@stevenbal
⚡ [#1365] Remove ordering from auth filter query
330a6d0
stevenbal added a commit that referenced this issue May 15, 2023
@stevenbal
⚡ [#1365] Add a limit to the ZaakPaginator.count
20469bc 
the .count for pagination is a big performance bottleneck. To fix this, a limit of 500 is set for the count to keep pagination links functioning and improve performance
stevenbal added a commit that referenced this issue May 15, 2023
@stevenbal
⚡ [#1365] Add a limit to the ZaakPaginator.count
63199f5 
the .count for pagination is a big performance bottleneck. To fix this, a limit of 500 is set for the count to keep pagination links functioning and improve performance
stevenbal added a commit that referenced this issue May 15, 2023
@stevenbal
⚡ [#1365] Add a limit to the ZaakPaginator.count
dddc718 
the .count for pagination is a big performance bottleneck. To fix this, a limit of 500 is set for the count to keep pagination links functioning and improve performance
stevenbal added a commit that referenced this issue May 15, 2023
@stevenbal
⚡ [#1365] Add a limit to the ZaakPaginator.count
bfb8e97 
the .count for pagination is a big performance bottleneck. To fix this, a limit of 500 is set for the count to keep pagination links functioning and improve performance
stevenbal added a commit that referenced this issue May 15, 2023
@stevenbal
✅ [#1365] Add tests for pagination feature flag
1d8074e
stevenbal added a commit that referenced this issue May 16, 2023
@stevenbal
⚡ [#1365] Add a limit to the ZaakPaginator.count
e6a9ed3 
the .count for pagination is a big performance bottleneck. To fix this, a limit of 500 is set for the count to keep pagination links functioning and improve performance
stevenbal added a commit that referenced this issue May 16, 2023
@stevenbal
✅ [#1365] Add tests for pagination feature flag
71446de
stevenbal added a commit that referenced this issue May 16, 2023
@stevenbal
✨ [#1365] Use pagination class with feature flag as default
4876774
stevenbal added a commit that referenced this issue May 16, 2023
@stevenbal
✨ [#1365] Use pagination class with feature flag as default
c6ee9e6
stevenbal added a commit that referenced this issue May 16, 2023
@stevenbal
✅ [#1365] Fix tests for pagination changes
5a48825
stevenbal added a commit that referenced this issue May 16, 2023
@stevenbal
✅ [#1365] Fix tests for pagination changes
8400c2c
stevenbal added a commit that referenced this issue May 16, 2023
@stevenbal
✅ [#1365] Fix tests for pagination changes
5d2cf3b
stevenbal added a commit that referenced this issue May 16, 2023
@stevenbal
🐛 [#1365] Do not use fuzzy pagination for CMIS
e616231
stevenbal added a commit that referenced this issue May 16, 2023
@stevenbal
⚡ [#1365] Remove ordering from auth filter query
a41d7b6
stevenbal added a commit that referenced this issue May 16, 2023
@stevenbal
⚡ [#1365] Add a limit to the ZaakPaginator.count
16ac03a 
the .count for pagination is a big performance bottleneck. To fix this, a limit of 500 is set for the count to keep pagination links functioning and improve performance
stevenbal added a commit that referenced this issue May 16, 2023
@stevenbal
✅ [#1365] Add tests for pagination feature flag
06d877a
stevenbal added a commit that referenced this issue May 16, 2023
@stevenbal
✨ [#1365] Use pagination class with feature flag as default
084cf41
stevenbal added a commit that referenced this issue May 16, 2023
@stevenbal
✅ [#1365] Fix tests for pagination changes
e803eb6
stevenbal added a commit that referenced this issue May 16, 2023
@stevenbal
🐛 [#1365] Do not use fuzzy pagination for CMIS
53cb8aa
stevenbal added a commit that referenced this issue Jun 6, 2023
@stevenbal
🐛 [#1365] Do not use fuzzy pagination for CMIS
5d61443
stevenbal added a commit that referenced this issue Jun 6, 2023
@stevenbal
⚡ [#1365] Improve performance for list auth lookup
648c59f 
the original query first fetched the pks for the authorized objects and used those in a filter on the original queryset. This caused bad performance for big querysets, especially if DISTINCT was used (EIO/zaak). Now this step is skipped and the objects are fetched directly
stevenbal added a commit that referenced this issue Jun 6, 2023
@stevenbal
📝 [#1365] Implement schema generation for fuzzy pagination
b45456f 
EnkelvoudigInformatieObject is not marked with the fuzzy pagination parameter, because this functionality is disabled for CMIS and CMIS_ENABLED=True when generating the schema
stevenbal added a commit that referenced this issue Jun 6, 2023
@stevenbal
✅ [#1365] Add tests for filtering/auth after performance improvement
8274800
stevenbal added a commit that referenced this issue Jun 6, 2023
@stevenbal
⚡ [#1365] Remove ordering from auth filter query
a96a935
stevenbal added a commit that referenced this issue Jun 6, 2023
@stevenbal
⚡ [#1365] Add a limit to the ZaakPaginator.count
ab09530 
the .count for pagination is a big performance bottleneck. To fix this, a limit of 500 is set for the count to keep pagination links functioning and improve performance
stevenbal added a commit that referenced this issue Jun 6, 2023
@stevenbal
✅ [#1365] Add tests for pagination feature flag
5ecd2e7
stevenbal added a commit that referenced this issue Jun 6, 2023
@stevenbal
✨ [#1365] Use pagination class with feature flag as default
8e674d4
stevenbal added a commit that referenced this issue Jun 6, 2023
@stevenbal
✅ [#1365] Fix tests for pagination changes
60f4372
stevenbal added a commit that referenced this issue Jun 6, 2023
@stevenbal
🐛 [#1365] Do not use fuzzy pagination for CMIS
1f9f61c
stevenbal added a commit that referenced this issue Jun 6, 2023
@stevenbal
⚡ [#1365] Improve performance for list auth lookup
8a23a27 
the original query first fetched the pks for the authorized objects and used those in a filter on the original queryset. This caused bad performance for big querysets, especially if DISTINCT was used (EIO/zaak). Now this step is skipped and the objects are fetched directly
stevenbal added a commit that referenced this issue Jun 6, 2023
@stevenbal
📝 [#1365] Implement schema generation for fuzzy pagination
e373cfd 
EnkelvoudigInformatieObject is not marked with the fuzzy pagination parameter, because this functionality is disabled for CMIS and CMIS_ENABLED=True when generating the schema
stevenbal added a commit that referenced this issue Jun 6, 2023
@stevenbal
✅ [#1365] Add tests for filtering/auth after performance improvement
203fa3a
@joeribekker joeribekker added this to the Release 1.9.0 milestone Jun 19, 2023
@joeribekker
Copy link
Member

I made a ticket for this at VNG: VNG-Realisatie/gemma-zaken#2284

@annashamray
Copy link
Contributor

I'll separate this into two parts:

  1. optimize pagination.count by removing unnecessary joints
  2. introducing FuzzyPagination

The first part doesn't create deviations from the standard, but can add some performance boost, so I'll add it in Release 1.9

@annashamray annashamray modified the milestones: Release 1.11, Release 1.9 Jul 13, 2023
@annashamray annashamray mentioned this issue Jul 13, 2023
Merged
annashamray added a commit that referenced this issue Jul 13, 2023
@annashamray
⚡ [#1365] use only pk for pagination count
27498d0
@annashamray annashamray self-assigned this Jul 13, 2023
annashamray pushed a commit that referenced this issue Jul 14, 2023
@stevenbal @annashamray
⚡ [#1365] Remove ordering from auth filter query
6c5fee1
annashamray pushed a commit that referenced this issue Jul 14, 2023
@stevenbal @annashamray
⚡ [#1365] Improve performance for list auth lookup
7fdcfba 
the original query first fetched the pks for the authorized objects and used those in a filter on the original queryset. This caused bad performance for big querysets, especially if DISTINCT was used (EIO/zaak). Now this step is skipped and the objects are fetched directly
annashamray pushed a commit that referenced this issue Jul 14, 2023
@stevenbal @annashamray
✅ [#1365] Add tests for filtering/auth after performance improvement
7ddfa55
@annashamray annashamray mentioned this issue Jul 14, 2023
Merged
@annashamray annashamray mentioned this issue Jul 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@stevenbal stevenbal

@annashamray annashamray

Labels
standard: zaken 1.6 topic: performance
Projects
No open projects
Development
Archived in project
Milestone
Release 1.9
3 participants
@joeribekker @stevenbal @annashamray