Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(client): Fix batch deletes causing call stack overflow #1394

Merged
merged 13 commits into from
Jun 26, 2024

Conversation

msfstef
Copy link
Contributor

@msfstef msfstef commented Jun 20, 2024

Addresses VAX-1985

Apparently the method we were using, where we chained OR and AND clauses in the WHERE clause, led to:

  1. SQLite call stack overflow (at least with wa-sqlite) at around 6k OR clauses, far below the max parameter limit
  2. Sub-optimal performance in both SQLite and PGlite

I've converted the batching to use IN statements for both single-column queries and multi-column ones, like with composite primary keys. The performance is significantly improved and has room for more improvement:

Old Batching

Shadow Table 30k deletes
  • ~4700ms pglite
  • ~500ms wa-sqlite (arbitrary small batching to avoid overflow)
Comment 9k deletes
  • ~700ms pglite
  • ~2000ms wa-sqlite (max 6k batches)

New Batching

Shadow Table 30k deletes
  • ~100ms pglite
  • ~450ms wa-sqlite
Comment 9k deletes
  • ~40ms pglite
  • ~700ms wa-sqlite

While the shadow table deletes take similar time for wa-sqlite (~500ms), the shadow table uses a triple composite key of 3 string columns. There's room for significant optimization there, concatenating the parameters and deleting based on the concatenated PK columns reduced the time to execute from ~500ms to ~50ms - pglite also had a very small improvement but hard to measure.

I think it's possible to avoid having a composite primary key for the shadow table since we know it's 3 string columns and they can be collapsed into one if that proves to be a significant optimization, but that is outside of the scope of this fix, just raising it here as a potential optimization.

NOTE: this improvement is the result of the same investigation that yielded #1389, both together result in a useable linearlite with >2k issues otherwise we run into timeouts and call stack overflows.

@msfstef msfstef changed the title fix(client): Fix batch deletes fix(client): Fix batch deletes causing call stack overflow Jun 20, 2024
Copy link
Contributor

@kevin-dp kevin-dp left a comment

Choose a reason for hiding this comment

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

I left some comments, mainly simplifications for the unit tests.
Also don't forget to add a changeset.

clients/typescript/test/migrators/builder.ts Outdated Show resolved Hide resolved
clients/typescript/test/migrators/builder.ts Outdated Show resolved Hide resolved
@msfstef msfstef requested a review from kevin-dp June 25, 2024 07:58
Copy link
Contributor

@icehaunter icehaunter left a comment

Choose a reason for hiding this comment

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

small nitpicks, but GJ otherwise!

clients/typescript/test/migrators/builder.ts Outdated Show resolved Hide resolved
clients/typescript/src/satellite/process.ts Outdated Show resolved Hide resolved
@msfstef msfstef requested a review from icehaunter June 25, 2024 11:21
Copy link
Contributor

@kevin-dp kevin-dp left a comment

Choose a reason for hiding this comment

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

Great job, left a few minor comments.

@msfstef msfstef merged commit 5e2e276 into main Jun 26, 2024
10 checks passed
@msfstef msfstef deleted the msfstef/fix-batch-deletes branch June 26, 2024 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants