Skip to content

Fix: Large table causes sql error#2242

Open
Koc wants to merge 2 commits intofeature/speedup-value-formattingfrom
bugfix/fix-loading-of-large-tables
Open

Fix: Large table causes sql error#2242
Koc wants to merge 2 commits intofeature/speedup-value-formattingfrom
bugfix/fix-loading-of-large-tables

Conversation

@Koc
Copy link
Contributor

@Koc Koc commented Dec 28, 2025 •
edited
Loading

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

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

@Koc Koc requested review from blizzz and enjeck as code owners December 28, 2025 15:54
@Koc Koc marked this pull request as draft December 29, 2025 09:37
@Koc 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 Koc changed the base branch from main to feature/speedup-value-formatting December 29, 2025 23:45
@Koc 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 Koc added bug Something isn't working performance Performance issues and optimisations labels Jan 4, 2026
@Koc Koc marked this pull request as ready for review January 4, 2026 14:29
@Koc Koc force-pushed the feature/speedup-value-formatting branch 2 times, most recently from 3eb630d to 491ee41 Compare January 12, 2026 11:58
@Koc Koc force-pushed the feature/speedup-value-formatting branch from 491ee41 to b0c0685 Compare January 15, 2026 19:20
Koc added 2 commits January 15, 2026 20:21
@Koc
Fix: Large table causes sql error
e1b69b8 
Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>
@Koc
Fix: Large table causes sql error (cache cells)
f599f40 
Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>
@Koc Koc force-pushed the bugfix/fix-loading-of-large-tables branch from f792683 to f599f40 Compare January 15, 2026 19:21
enjeck
enjeck reviewed Jan 27, 2026
View reviewed changes
Copy link
Contributor

@enjeck enjeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing locally:
10k rows before vs after I don't notice any speedups. How are you measuring the timing?
And 100k rows fails for both

enjeck
enjeck reviewed Jan 27, 2026
View reviewed changes
lib/Db/RowLoader/CachedRowLoader.php
$row->setLastEditAt($sleeve['last_edit_at']);
$row->setTableId((int)$sleeve['table_id']);

$cachedCells = json_decode($sleeve['cached_cells'] ?? '{}', true);
Copy link
Contributor

@enjeck enjeck Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we falling back to the normal way if nothing is returned from cached_cells somewhere else?

Copy link
Contributor Author

@Koc Koc Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it's not needed as we are caching all rows during migration, see OCA\Tables\Migration\CacheSleeveCells and we're updating cached_cells on any cell edit. So, I have no any use-case when cached_cells can be missing/outdated.

enjeck
enjeck reviewed Jan 27, 2026
View reviewed changes
lib/Db/Row2Mapper.php
Comment on lines +211 to +213
foreach ($columnIds as $columnId) {
$columns[$columnId] = $this->columnMapper->find($columnId);
$mappers[$columnId] = $this->columnsHelper->getCellMapperFromType($columns[$columnId]->getType());
Copy link
Contributor

@enjeck enjeck Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we run into the n+1 problem?

Copy link
Contributor Author

@Koc Koc Jan 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure that this is a big problem here, as we're calling ColumnMapper::preloadColumns() before Row2Mapper::getRows() call and it memoizes loaded columns

@Koc
Copy link
Contributor Author

Koc commented Jan 27, 2026

hey @enjeck!

10k rows before vs after I don't notice any speedups. How are you measuring the timing?

I'm really surprised that you haven't any performance improvement with my PRs. I've spent some time and recorded screencast for you that compares:

nextcloud-tables-performance-2026-01-27_17.42.33.mp4

And 100k rows fails for both

I'm so sorry, but are you sure that you have switched to a correct branch? Can you share xlsx file with table data, so I can import it on my instance and re-test?

BTW as we're talking about importing of the large files - I highly recommend to merge another my PR #1801 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@enjeck enjeck enjeck left review comments

@blizzz blizzz Awaiting requested review from blizzz blizzz is a code owner

Assignees

No one assigned

Labels

bug Something isn't working performance Performance issues and optimisations

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Koc @enjeck