-
Notifications
You must be signed in to change notification settings - Fork 7
Instances list endpoint pagination #64
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
base: master
Are you sure you want to change the base?
Conversation
mattoberle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good.
Appreciate the additional work that went into performance testing too!
I left feedback on a couple of documentation-related items, nothing critical.
One case that is not covered in the tests is explicitly setting the CLAY_STORAGE_PAGE_SIZE environment variable. That could be a little tricky to mock depending on where your test is importing postgres/client.js, but might be worth covering since a future change to the parsing logic or default value could constitute a breaking change. What do you think?
docs/environment-setup.md
Outdated
| ### `CLAY_STORAGE_PAGE_SIZE` | ||
|
|
||
| **Default:** `null` _(Number)_ | ||
|
|
||
| Default page size for list queries. Enables pagination by default on list endpoints. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should note in the docs here that a page size of 0 will explicitly disable pagination.
docs/environment-setup.md
Outdated
| - [`CLAY_STORAGE_POSTGRES_DB`](#clay_storage_postgres_db) | ||
| - [`CLAY_STORAGE_POSTGRES_CACHE_ENABLED`](#clay_storage_postgres_cache_enabled) | ||
| - [`CLAY_STORAGE_POSTGRES_CACHE_HOST`](#clay_storage_postgres_cache_host) | ||
| - [`CLAY_STORAGE_POSTGRES_PAGE_SIZE`](#clay_storage_postgres_page_size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is referred to everywhere else as CLAY_STORAGE_PAGE_SIZE.
It seems that we already have an inconsistent naming scheme for the environment variables in this project, but maybe stick with the more generic CLAY_STORAGE_PAGE_SIZE (something that other storage back-ends could adhere to).
jjpaulino
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍕
paginate()to postgres query functions.PAGE_SIZEenv var, that when defined, sets up pagination by defaultPAGE_SIZEis not definedPagination will work by fetching the first page, then doing another request with the last item that was returned
prev=<uri>. Paginating in this way using the "seek" method isn't obviously better than usingoffsetat first glance, especially considering that it requires anorder byclause, but in local testing it is significantly faster especially at high offsets.Here are the results from querying a table of
clay-paragraphcomponents with a page size of 10:0th item
10,000
100,000
500,000
No paginating, get all instances
This approach closes a current DoS vector and also avoids creating an even worse vector.