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.

Sign up for GitHub

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

Jump to bottom

Extend Request/Response limit in sql-over-http to 32MiB #8984

Closed
wants to merge 2 commits into from
Closed

Extend Request/Response limit in sql-over-http to 32MiB #8984

wants to merge 2 commits into from
+3 −3

Conversation

nicksrandall
Copy link
Contributor

@nicksrandall nicksrandall commented Sep 11, 2024 •
edited
Loading

Problem

Fixes: #8983

TLDR; We are hitting this limit in more than one app, each with real-world use-cases.

Summary of changes

This extends the limit from 10MiB to 32MiB (as a sanity check, Planet Scale's limit is currently 64MiB). I'm open to changing this to whatever you think makes the most sense.

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.
  • Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
  • If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

  • Do not forget to reformat commit message to not include the above checklist

@nicksrandall nicksrandall requested a review from a team as a code owner September 11, 2024 20:49
orca-security-us[bot]
orca-security-us bot reviewed Sep 11, 2024
View reviewed changes
Copy link

@orca-security-us orca-security-us bot left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Orca Security Scan Summary

Status Check Issues by priority
Passed Passed Infrastructure as Code high 0   medium 0   low 0   info 0 View in Orca
Passed Passed Secrets high 0   medium 0   low 0   info 0 View in Orca
Passed Passed Vulnerabilities high 0   medium 0   low 0   info 0 View in Orca

@github-actions github-actions bot added the external A PR or Issue is created by an external user label Sep 11, 2024
@arpad-m arpad-m added the approved-for-ci-run Changes are safe to trigger CI for the PR label Sep 11, 2024
@github-actions github-actions bot removed the approved-for-ci-run Changes are safe to trigger CI for the PR label Sep 11, 2024
@vipvap vipvap mentioned this pull request Sep 11, 2024
Closed
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 11, 2024 •
edited
Loading

3857 tests run: 3743 passed, 0 failed, 114 skipped (full report)

Code coverage* (full report)

  • functions: 31.7% (7354 of 23179 functions)
  • lines: 50.0% (59547 of 119152 lines)

* collected from Rust tests only

The comment gets automatically updated with the latest test results
abb6af7 at 2024-09-12T10:48:07.273Z :recycle:

@arpad-m arpad-m added the approved-for-ci-run Changes are safe to trigger CI for the PR label Sep 12, 2024
@github-actions github-actions bot removed the approved-for-ci-run Changes are safe to trigger CI for the PR label Sep 12, 2024
@nicksrandall
Copy link
Contributor Author

My thinking around 32MiB is that it could be an incremental step towards 64MiB (maybe even beyond). You could monitor this change for a period of time and when you are confident that it hasn't had an adverse effect on the system then we can bump it even more. Ideally this limit gets as large as we can make it while still protecting the system.

@nicksrandall
Copy link
Contributor Author

@conradludgate any feedback for this PR? Who would be the right person to review it?

@conradludgate
Copy link
Contributor

conradludgate commented Sep 16, 2024 •
edited
Loading

I will consider this, I'm not 100% sure just yet. Is it possible that you could use the webhooks API for these large queries? I expect the large number of TCP packets needing to be acknowledged would dwarf the extra roundtrips for the websocket startup cost.

@nicksrandall
Copy link
Contributor Author

It is not possible for us to switch to websocket for large queries as the queries are authored by our users (not us) so we don’t know before hand the size of result of the query.

For what it's worth, here are the limits used by other DB providers who offer sql-over-http:

  • Supabase: limit is configurable with no hard limit
  • Xata: no limit
  • Planetscale: 64MiB
  • Cloudflare D1: 30 seconds

@nicksrandall
Copy link
Contributor Author

Looks like this PR has been superseded by #9029.

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

@orca-security-us Orca Security (US) orca-security-us left review comments

@conradludgate conradludgate Awaiting requested review from conradludgate conradludgate is a code owner automatically assigned from neondatabase/proxy

Assignees
No one assigned
Labels
external A PR or Issue is created by an external user
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extend Response Size limit for sql-over-http responses?
3 participants
@nicksrandall @conradludgate @arpad-m