Fix: Large table causes sql error by Koc · Pull Request #2242 · nextcloud/tables

Koc · 2025-12-28T15:54:26Z

This PR built on top of #2238.
Closes #1490.

It fixes loading of the large tables. Right now it is not possible to load a table with 30k rows and 8 columns due to an error:

"Previous":{"Exception":"Doctrine\\DBAL\\Exception\\DriverException","Message":
"An exception occurred while executing a query: SQLSTATE[HY000]: 
General error: 7 number of parameters must be between 0 and 65535",

PR contains 2 commits: fix itself and performance improvement.

I've measured latency for a various scenarios for /row/table/{tableId} endpoint:

Scenario	master	#2238	1st commit from this PR	2nd commit from this PR
8 columns x 2k rows	0.68s	0.31s	0.31s	0.17s
8 columns x 10k rows	2.86s	1.21s	1.21s	0.44s
8 columns x 60k rows	🐞 sql error	🐞 sql error	6.97s	2.27s

📹 Video for performance comparison

Next step: move filtering/sorting/pagination to BE side and speedup FE. But this is completely separate topic which will be implemented in upcoming PRs.

enjeck · 2026-06-19T08:00:44Z

This import path writes cells directly via the cell mapper, bypassing Row2Mapper, so tables_row_sleeves.cached_cells is never populated for imported rows?

I didn't get the context. Is it the same as #2242 (comment) ? Otherwise can you provide affected method names?

I did import via this flow and it caches everything fine with latest changes

enjeck · 2026-06-19T08:26:42Z

we should update the migration name. It's been so long since this was started, new migrations have been added now

updated. Also bumped version from 2.2 to 2.3

Actually the migration name needs to be so that it sorts beyond the last existing one:

Otherwise it would not be executed.

Yeah, we introduced a mistakes in the migration names 😔

enjeck · 2026-06-19T08:35:34Z

@@ -768,9 +670,12 @@ public function update(Row2 $row, ?string $userId = null): Row2 {
 		$this->columnMapper->preloadColumns(array_column($changedCells, 'columnId'));

 		// write all changed cells to its db-table
+		$cachedCells = $sleeve->getCachedCellsArray();
 		foreach ($changedCells as $cell) {
-			$this->insertOrUpdateCell($sleeve->getId(), $cell['columnId'], $cell['value']);
+			$cachedCells[$cell['columnId']] = $this->insertOrUpdateCell($sleeve->getId(), $cell['columnId'], $cell['value']);
 		}
+		$sleeve->setCachedCellsArray($cachedCells);
+		$this->rowSleeveMapper->update($sleeve);


update() writes the sleeve twice per row edit. Can we fold the metadata change and the cached_cells update into a single update() call.

I did some changes here, testing now.

TBH would be nice to wrap all this db changes into a transaction. But this is completely separate topic

enjeck · 2026-06-19T08:41:55Z


 	/**
 	 * @param int[] $ids
-	 * @return RowSleeve[]


confused why raw data is better performance

I guess the same reason like in #2238

Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>

Koc · 2026-06-21T11:02:32Z

@enjeck @blizzz I did all requested changes, re-tested affected flows manually, we have green pipeline again. Please re-review one more time

blizzz

I have a few questions and scaling concerns (also nitpicks added 😇 )

blizzz · 2026-06-23T11:40:46Z


 ]]></description>
-	<version>2.2.0</version>
+	<version>2.3.0</version>


Suggested change

<version>2.3.0</version>

<version>2.3.0-dev.0</version>

Would go for a pre-release number, unless we'd immediately release afterwards.

blizzz · 2026-06-23T11:41:33Z

@@ -0,0 +1,92 @@
+<?php
+


Suggested change

declare(strict_types=1);

blizzz · 2026-06-23T11:44:45Z

Actually the migration name needs to be so that it sorts beyond the last existing one:

Otherwise it would not be executed.

Yeah, we introduced a mistakes in the migration names 😔

blizzz · 2026-06-23T11:47:22Z

+
+		$table = $schema->getTable('tables_row_sleeves');
+		if (!$table->hasColumn('cached_cells')) {
+			$table->addColumn('cached_cells', Types::JSON, [


We should avoid JSON types, as they work poorly across different DB vendors and caused trouble in the past. Use Text instead. Types::JSON is also officially deprecated for these reasons since NC 33.

blizzz · 2026-06-23T11:59:26Z

+					}
+
+					$sleeve = $this->rowSleeveMapper->find($rowId);
+					$sleeve->setCachedCellsArray($cachedCells);


Correct me if I am wrong. This stores all cell values of a how table in a single row? On MySQL this would be limited to whopping 4GB, but generally, this sounds risky and could lead to transferal of a huge amount of data on really big tables. What do you think?

Also as a post-migration step, this might take a real long time to accomplish on instanced with a big amount of data.

blizzz · 2026-06-23T12:03:55Z

+					foreach ($cells as $cell) {
+						$cachedCells[$cell->getColumnId()] = $mapper->toArray($cell);
+					}
+				} catch (\Exception $e) {


It's a too broad catch, isn't it?

blizzz · 2026-06-23T12:04:46Z

+			$cachedCells = [];
+
+			/** @var RowCellMapperSuper[] $cellMappers */
+			$cellMappers = [


doesn't need to be within the loop?

blizzz · 2026-06-23T12:05:47Z

 	}

+	private function rebuildCachedCells(array $rowIdMap): void {
+		foreach ($rowIdMap as $newRowId) {


Especially if this is running in user facing scripts, this may run into a timeout on big tables and fail.

blizzz · 2026-06-23T12:06:28Z

 			$sleeveAlias . '.id',
 			$sleeveAlias . '.table_id',
+			$sleeveAlias . '.cached_cells',
 			$sleeveAlias . '.created_by',


Koc requested review from blizzz and enjeck as code owners December 28, 2025 15:54

Koc marked this pull request as draft December 29, 2025 09:37

Koc force-pushed the bugfix/fix-loading-of-large-tables branch 2 times, most recently from 410c7ff to e497d0b Compare December 29, 2025 23:45

Koc changed the base branch from main to feature/speedup-value-formatting December 29, 2025 23:45

Koc force-pushed the bugfix/fix-loading-of-large-tables branch 19 times, most recently from 4e43530 to f792683 Compare January 4, 2026 13:59

Koc added bug Something isn't working performance Performance issues and optimisations labels Jan 4, 2026

Koc marked this pull request as ready for review January 4, 2026 14:29

Koc force-pushed the feature/speedup-value-formatting branch 2 times, most recently from 3eb630d to 491ee41 Compare January 12, 2026 11:58

Koc force-pushed the bugfix/fix-loading-of-large-tables branch from f599f40 to 8ca7e61 Compare February 12, 2026 18:48

Koc mentioned this pull request Feb 12, 2026

Perf: Optimize shares loading by eliminating N+1 queries #2293

Merged

Koc force-pushed the bugfix/fix-loading-of-large-tables branch from 8ca7e61 to 5ce5c0f Compare February 12, 2026 23:15

enjeck force-pushed the feature/speedup-value-formatting branch from b26b1e0 to 696cb1a Compare March 4, 2026 07:49

Koc force-pushed the feature/speedup-value-formatting branch 2 times, most recently from 006839e to 75d3196 Compare March 9, 2026 23:29

Koc force-pushed the feature/speedup-value-formatting branch 2 times, most recently from 691a826 to 45ae426 Compare April 1, 2026 13:26

Koc force-pushed the feature/speedup-value-formatting branch from 45ae426 to 2dfe1f2 Compare April 10, 2026 21:25

Koc force-pushed the feature/speedup-value-formatting branch 2 times, most recently from 2a8feb5 to deb1979 Compare June 7, 2026 15:57

enjeck force-pushed the feature/speedup-value-formatting branch from deb1979 to c6912b7 Compare June 16, 2026 06:25

Base automatically changed from feature/speedup-value-formatting to main June 18, 2026 11:07

Koc force-pushed the bugfix/fix-loading-of-large-tables branch 7 times, most recently from c86e7ec to cfc722b Compare June 18, 2026 22:42

enjeck reviewed Jun 19, 2026

View reviewed changes

Koc added 2 commits June 20, 2026 13:40

Fix: Large table causes sql error

80656a6

Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>

Fix: Large table causes sql error (cache cells)

0d16156

Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>

Koc force-pushed the bugfix/fix-loading-of-large-tables branch from cfc722b to 0d16156 Compare June 20, 2026 11:41

Fix: Large table causes sql error (code review fixes WIP)

e804e3c

Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>

Koc force-pushed the bugfix/fix-loading-of-large-tables branch from e974ce9 to 7f43314 Compare June 20, 2026 13:00

Fix: Large table causes sql error (code review fixes WIP)

a150684

Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>

Koc force-pushed the bugfix/fix-loading-of-large-tables branch from 7f43314 to a150684 Compare June 20, 2026 13:02

blizzz requested changes Jun 23, 2026

View reviewed changes

Uh oh!

Conversation

Koc commented Dec 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Koc Jun 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Koc commented Jun 21, 2026

Uh oh!

blizzz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Koc commented Dec 28, 2025 •

edited

Loading

Koc Jun 20, 2026 •

edited

Loading