Skip to content

Fix: Large table causes sql error#2242

Open
Koc wants to merge 3 commits into
mainfrom
bugfix/fix-loading-of-large-tables
Open

Fix: Large table causes sql error#2242
Koc wants to merge 3 commits into
mainfrom
bugfix/fix-loading-of-large-tables

Conversation

@Koc

@Koc Koc commented Dec 28, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

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.

@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 mentioned this pull request Feb 12, 2026
Merged
@Koc Koc force-pushed the bugfix/fix-loading-of-large-tables branch from 8ca7e61 to 5ce5c0f Compare February 12, 2026 23:15
@enjeck enjeck force-pushed the feature/speedup-value-formatting branch from b26b1e0 to 696cb1a Compare March 4, 2026 07:49
@Koc Koc force-pushed the feature/speedup-value-formatting branch 2 times, most recently from 006839e to 75d3196 Compare March 9, 2026 23:29
@Koc Koc force-pushed the feature/speedup-value-formatting branch 2 times, most recently from 691a826 to 45ae426 Compare April 1, 2026 13:26
@Koc Koc force-pushed the feature/speedup-value-formatting branch from 45ae426 to 2dfe1f2 Compare April 10, 2026 21:25
@Koc Koc force-pushed the feature/speedup-value-formatting branch 2 times, most recently from 2a8feb5 to deb1979 Compare June 7, 2026 15:57
@enjeck 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 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
enjeck reviewed Jun 19, 2026
View reviewed changes
Comment thread lib/Migration/Version001010Date20251229000000.php Outdated
Comment thread lib/Db/Row2Mapper.php
Comment thread lib/Db/RowLoader/CachedRowLoader.php
Comment thread lib/Migration/Version2030Date20260620000000.php
Comment thread lib/Db/Row2Mapper.php
Comment on lines 636 to +678
@@ -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);

@enjeck enjeck Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@Koc Koc Jun 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Comment thread lib/Db/RowSleeveMapper.php

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

@enjeck enjeck Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

confused why raw data is better performance

@Koc Koc Jun 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I guess the same reason like in #2238

Comment thread lib/Migration/CacheSleeveCells.php Outdated
Comment thread lib/Migration/CacheSleeveCells.php Outdated
@Koc Koc force-pushed the bugfix/fix-loading-of-large-tables branch 3 times, most recently from 7f43314 to a150684 Compare June 20, 2026 13:02
@Koc

Koc commented Jun 21, 2026

Copy link
Copy Markdown
Contributor Author

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

blizzz
blizzz requested changes Jun 23, 2026
View reviewed changes

@blizzz blizzz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment thread appinfo/info.xml Outdated
Comment thread lib/Db/RowLoader/CachedRowLoader.php
Comment thread lib/Migration/Version2030Date20260620000000.php
Comment thread lib/Migration/Version002030Date20260620000000.php Outdated
Comment thread lib/Migration/CacheSleeveCells.php
}

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

@blizzz blizzz Jun 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

@blizzz blizzz Jun 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment thread lib/UserMigration/TablesMigrator.php Outdated
Comment thread lib/UserMigration/TablesMigrator.php
$cachedCells = [];

/** @var RowCellMapperSuper[] $cellMappers */
$cellMappers = [

@blizzz blizzz Jun 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

doesn't need to be within the loop?

@Koc Koc Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sorry, I didn't get. What do you mean?

Comment thread lib/UserMigration/TablesMigrator.php
}

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

@blizzz blizzz Jun 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment thread lib/Db/RowSleeveMapper.php
$sleeveAlias . '.id',
$sleeveAlias . '.table_id',
$sleeveAlias . '.cached_cells',
$sleeveAlias . '.created_by',

@blizzz blizzz Jun 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

always?

Koc added 2 commits June 26, 2026 01:32
@Koc
Fix: Large table causes sql error
c9a5ec7 
Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>
@Koc
Fix: Large table causes sql error (cache cells)
331f9e5 
Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>
@Koc Koc force-pushed the bugfix/fix-loading-of-large-tables branch from a150684 to 2218bb6 Compare June 25, 2026 23:42
@Koc
Fix: Large table causes sql error (code review fixes)
cfe7f45 
Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>
@Koc Koc force-pushed the bugfix/fix-loading-of-large-tables branch from 2218bb6 to cfe7f45 Compare June 25, 2026 23:46
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 blizzz requested changes

Requested changes must be addressed to merge this pull request.

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.

Cannot open a table with 25k rows

3 participants

@Koc @blizzz @enjeck