perf: Enhance objects using constant sql queries count to get rid of N+1 queries - #2857
perf: Enhance objects using constant sql queries count to get rid of N+1 queries#2857Koc wants to merge 1 commit into
Conversation
9b6bd68 to
4033598
Compare
Koc
changed the title
enhanceTables for a multiple tables at once to get rid of N+1 queries
Koc
changed the title
29cbb47 to
44e315b
Compare
2686136 to
e9b83fb
Compare
e9b83fb to
b286459
Compare
| if ($sender !== '') { | ||
| $qb->andWhere($qb->expr()->eq('sender', $qb->createNamedParameter($sender, IQueryBuilder::PARAM_STR))); | ||
| } | ||
|
|
There was a problem hiding this comment.
Should this be moved to plural findAllSharesForNodes only (which is what the count methods need)? This adds sender filtering to the shared singular method, which previously ignored $sender and returned all shares for a node. now, it changes every existing caller that passes a user id.
There was a problem hiding this comment.
As a downstream effect of the findAllSharesForNode change above, the list all shares" endpoint now returns only shares created by the current user. An owner will no longer see shares created by co-managers on their own table or view, so they cannot review or revoke them here?
There was a problem hiding this comment.
same for sharecontroller.php
There was a problem hiding this comment.
I think we should have a test for deleteAllForContext here. Since this now only iterates shares whose sender equals the current user, so for a context with shares from more than one sender the context_navigation entries for the others are not cleaned up before the bulk deleteByNode, leaving orphaned rows???
| try { | ||
| return new DataResponse($this->viewService->formatViews($this->viewService->findAll($this->tableService->find($tableId)))); | ||
| $table = $this->tableService->find($tableId); | ||
| return new DataResponse($this->viewService->formatViews($table->getViews() ?? [])); |
There was a problem hiding this comment.
initially, we return 403 for read-only users . Now, read-only user gets 200 with an empty list, since enhanceTables only loads views for owned or managed tables. So we need to mofify the annotation?
| $counts = array_fill_keys($tableIds, 0); | ||
| foreach (array_chunk($tableIds, 1000 - 1) as $tableIdsChunk) { | ||
| $qb = $this->db->getQueryBuilder(); | ||
| $qb->select('table_id', $qb->func()->count('*', 'counter')) | ||
| ->from($this->table) | ||
| ->where($qb->expr()->in('table_id', $qb->createNamedParameter($tableIdsChunk, IQueryBuilder::PARAM_INT_ARRAY))) | ||
| ->groupBy('table_id'); | ||
|
|
||
| $result = $qb->executeQuery(); | ||
| while ($row = $result->fetch()) { | ||
| $counts[(int)$row['table_id']] = (int)$row['counter']; | ||
| } | ||
| $result->closeCursor(); |
There was a problem hiding this comment.
without try...catch, a \OCP\DB\Exception now propagates?
| $qb->select('v.*', 't.ownership') | ||
| ->from($this->table, 'v') | ||
| ->innerJoin('v', 'tables_tables', 't', 't.id = v.table_id') | ||
| ->where($qb->expr()->in('v.table_id', $qb->createNamedParameter($tableIds, IQueryBuilder::PARAM_INT_ARRAY))); |
There was a problem hiding this comment.
don't we need same array_chunk here? Since every other batch method you added chunks the IN list at about 1000, and findMany does too, but this one passes all table ids in a single IN. What if the user has more than ~1000 tables, or the DB has a 1000 element IN limit (Oracle does, i think?)??
b286459 to
306f894
Compare
…N+1 queries Assisted-by: Cascade:SWE-1.6
306f894 to
f5cb044
Compare
3 participants
In our organization we operate a large amount of tables/views, so there are performance bottleneck here:
TableService::enhanceTable()in a loop with a single call of theenhanceTables()ViewService::enhanceView()in a loop with a single call of theenhanceViews()UserHelperfor row/column counts, share counts and owner display names.🖼️ Screenshots
Here profiler results for
GET /apps/tables/tableendpoint for 18 tables and 9 views:🏁 Checklist
/backport to stableX.X🤖 AI (if applicable)