Skip to content

Fix: Large table causes sql error#2242

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

Fix: Large table causes sql error#2242
Koc wants to merge 4 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 force-pushed the feature/speedup-value-formatting branch from b0c0685 to b26b1e0 Compare February 12, 2026 18:41
@Koc Koc force-pushed the bugfix/fix-loading-of-large-tables branch from f599f40 to 8ca7e61 Compare February 12, 2026 18:48
@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

@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.

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

@Koc Koc Jun 20, 2026 •
edited
Loading

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 didn't get the context. Is it the same as #2242 (comment) ? Otherwise can you provide affected method names?

@Koc Koc Jun 21, 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 import via this flow and it caches everything fine with latest changes

image

Comment thread lib/Db/RowLoader/CachedRowLoader.php
$row->setLastEditAt($sleeve['last_edit_at']);
$row->setTableId((int)$sleeve['table_id']);

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

@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.

RowService::importRow and TablesMigrator::importRowCells write sleeves and cells directly without setting cached_cells, and the repair step won't re-run (the cachingSleeveCellsComplete flag is already set), so those rows come back blank, i think. Is it better to fallback to NormalizedRowLoader?

Comment thread lib/Migration/Version002030Date20260620000000.php

@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.

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

@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.

updated. Also bumped version from 2.2 to 2.3

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 added 2 commits June 20, 2026 13:40
@Koc
Fix: Large table causes sql error
80656a6 
Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>
@Koc
Fix: Large table causes sql error (cache cells)
0d16156 
Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>
@Koc Koc force-pushed the bugfix/fix-loading-of-large-tables branch from cfc722b to 0d16156 Compare June 20, 2026 11:41
@Koc
Fix: Large table causes sql error (code review fixes WIP)
e804e3c 
Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>
@Koc Koc force-pushed the bugfix/fix-loading-of-large-tables branch from e974ce9 to 7f43314 Compare June 20, 2026 13:00
@Koc
Fix: Large table causes sql error (code review fixes WIP)
a150684 
Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>
@Koc Koc force-pushed the bugfix/fix-loading-of-large-tables branch 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

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

At least 1 approving review is required 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

2 participants

@Koc @enjeck