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

Chunk large satellite payloads #1399

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

robacourt
Copy link
Contributor

To prevent the 100MB payload limit:

[Symbol(kError)]: RangeError: Max payload size exceeded
      at Receiver.haveLength (/Users/rob/src/electric-sql/electric/node_modules/.pnpm/[email protected]/node_modules/ws/lib/receiver.js:419:28)
      at Receiver.getPayloadLength64 (/Users/rob/src/electric-sql/electric/node_modules/.pnpm/[email protected]/node_modules/ws/lib/receiver.js:406:10)
      at Receiver.startLoop (/Users/rob/src/electric-sql/electric/node_modules/.pnpm/[email protected]/node_modules/ws/lib/receiver.js:161:16)
      at Receiver._write (/Users/rob/src/electric-sql/electric/node_modules/.pnpm/[email protected]/node_modules/ws/lib/receiver.js:94:10)
      at writeOrBuffer (node:internal/streams/writable:564:12)
      at _write (node:internal/streams/writable:493:10)
      at Writable.write (node:internal/streams/writable:502:10)
      at Socket.socketOnData (/Users/rob/src/electric-sql/electric/node_modules/.pnpm/[email protected]/node_modules/ws/lib/websocket.js:1303:35)
      at Socket.emit (node:events:519:28)
      at addChunk (node:internal/streams/readable:559:12) {
    code: 'WS_ERR_UNSUPPORTED_MESSAGE_LENGTH',
    [Symbol(status-code)]: 1009
    ```

@robacourt robacourt marked this pull request as ready for review June 24, 2024 13:11
@alco
Copy link
Member

alco commented Jun 24, 2024

@robacourt Hey, I don't know if there was a discussion behind this that I missed, but this change doesn't address the issue stated in the PR description. For one, a single INSERT or UPDATE may be quite large, so the limit of 100 ops per SatOpLog does not in itself prevent it from overflowing.

More importantly, a single SatOpLog is treated by the client as a transaction. This change effectively splits a single PG transaction into multiple client-side transactions. This might not be an issue in the current implementation due to the fact that transactions are processed on the client in the same order as they are sent by the server, but any addition of concurrency to client-side processing further down the road could lead to subtle bugs because of this.

A proper solution to the payload size problem requires changing the Satellite protocol to support splitting a single transaction into multiple protocol messages.

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.

Great find! Would you mind adding a test to the client side (in clients/typescript/test/satellite/client.test.ts) that ensures the transaction is emitted correctly when begin and commit fall into separate oplog messages?

@robacourt
Copy link
Contributor Author

robacourt commented Jun 27, 2024

Great find! Would you mind adding a test to the client side (in clients/typescript/test/satellite/client.test.ts) that ensures the transaction is emitted correctly when begin and commit fall into separate oplog messages?

I think we have one already:

test.serial('receive transaction over multiple messages', async (t) => {

Is that enough?

@robacourt robacourt force-pushed the rob/chunk-large-satellite-payloads branch from badfd40 to 5c5916c Compare June 27, 2024 15:55
Copy link
Member

@alco alco left a comment

Choose a reason for hiding this comment

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

LGTM!

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