Add a configurable maximum for client-requested list page sizes - #5282
Add a configurable maximum for client-requested list page sizes#5282ayushtkn wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a server-side ceiling for client-requested list pagination sizes to prevent arbitrarily large list pages, aligning Polaris behavior with the Iceberg REST spec’s “page size is an upper bound” semantics.
Changes:
- Introduces
LIST_PAGINATION_MAX_PAGE_SIZE(default100, overridable per-catalog viapolaris.config.list-pagination-max-page-size) in core configuration. - Applies page-size bounding to
listNamespaces,listTables, andlistViewsinIcebergCatalogHandlerfor both local and federated catalogs. - Adds/updates tests plus user-facing documentation and a changelog entry describing the new limit.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| site/content/in-dev/unreleased/configuration/config-sections/flags-polaris_features.md | Documents the new LIST_PAGINATION_MAX_PAGE_SIZE feature/config key and defaults. |
| polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java | Defines the new typed configuration constant and catalog override key. |
| runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java | Enforces the configured maximum page size in list endpoints (local + federated). |
| runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerTest.java | Adds parameterized coverage for local catalog page-size bounding behavior. |
| runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapterTest.java | Adds coverage ensuring federated listing pagination is bounded to the max. |
| CHANGELOG.md | Notes the new server-side maximum for list page sizes and configuration key. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
6a3bd49 to
8e49320
Compare
| int maxPageSize = maxPageSize(); | ||
| Integer boundedPageSize = pageSize == null ? null : Math.min(pageSize, maxPageSize); | ||
| PageToken pageRequest = PageToken.build(pageToken, boundedPageSize, this::shouldDecodeToken); | ||
| if (pageRequest.pageSize().orElse(0) > maxPageSize) { |
There was a problem hiding this comment.
I'm not sure about this logic 🤔 PageToken.build() already accounts for user-provided new page size values (via boundedPageSize). If that is not set, the only other option is that the previous page token had a very large page size... but in that case, I tend to think that PageToken.build() should take care of that adjustment.
In other words, could we pass maxPageSize to PageToken.build() and deal with all size adjustments there?
Also, do we still want to return READ_EVERYTHING from PageToken.build()? It kind of contradicts having the page size limit 🤔
The IRC spec states:
Servers that support pagination must return all results in a single response with the value
of `next-page-token` set to `null` if the query parameter `pageToken` is not set in the
request.
Yet, I do not think it is a sound requirement. This makes a large catalog vulnerable to DoS attacks via "list" query overload.
How about we always perform pagination when shouldDecodeToken is true and maxPageSize is set, but allow the required IRC "unlimited" behaviour when maxPageSize is NOT set?
This both allows users to protect Polaris from overload and also support (legacy) cases where clients rely on "reading everything" in one response.
There was a problem hiding this comment.
Thanx @dimas-b for the review. I have addressed the comments
PageToken.build() now takes maxPageSize and every size decision happens inside decodePageRequest: the explicitly requested size, the size carried by an incoming page token, and the size used when a client requests neither. The handler just passes the config through, so the earlier two-step adjustment is gone.
On READ_EVERYTHING — agreed, and implemented as you proposed. With a maximum configured, a listing that supplies no page token is now paginated rather than returning every row; with the maximum set to <= 0 the IRC-required behaviour is restored, so anyone relying on reading everything in one response can opt out explicitly.
| private @Nullable Integer boundedPageSize(@Nullable Integer requestedPageSize) { | ||
| int maxPageSize = maxPageSize(); | ||
| if (requestedPageSize == null || maxPageSize <= 0) { | ||
| return requestedPageSize; |
There was a problem hiding this comment.
When the request does not set a page size, this allows the federated catalog to return a lot of data 🤔 Should we limit that similarly to our own PageToken.build()?
There was a problem hiding this comment.
.... ideally reuse the same utility method?
15b5db2 to
58de78e
Compare
| * page token is supplied instead, making the listing paginate from the first request as it does | ||
| * for a local catalog. | ||
| */ | ||
| private @Nullable String boundedPageToken(@Nullable String pageToken) { |
There was a problem hiding this comment.
This turns a request that asked for no pagination into a paginated one, and since the max defaults to 100 it lands on every federated catalog unless an operator opts out. Would it make sense to bound only an explicitly requested pageSize here and leave a null pageToken alone?
There was a problem hiding this comment.
This one really depends on the comment below — let's settle that first, since if the federated bounding comes out this goes with it.
For context on why it's here: it's aiming for parity with the local path. Per earlier suggestion, we now paginate when shouldDecodeToken is true and a maximum is set, even if the client specified no page size or token, rather than returning everything. This does the same for federated catalogs.
The asymmetry you've spotted is real though: shouldDecodeToken() reads LIST_PAGINATION_ENABLED, and the federated branch never calls it, so that first condition is effectively always true there. In practice the bound kicks in unless an operator sets the maximum to -1, whereas on the local path it stays dormant until pagination is explicitly enabled. But once Pagination is enabled for local as well, it is similar.
| * PageTokenUtil#boundPageSize}. An absent page size becomes the maximum, so the remote result set | ||
| * is not returned whole. | ||
| */ | ||
| private @Nullable Integer boundedPageSize(@Nullable Integer requestedPageSize) { |
There was a problem hiding this comment.
IIUC the remote result set is still returned whole, just not to the client. CatalogHandlerUtils calls listTables on the remote catalog with no limit and slices the full list in memory, and it repeats that fetch for every page request. So a 1000-table namespace goes from one full remote listing to ten, and Polaris holds all 1000 either way. The token here is an offset rather than a cursor, so bounding the actual work probably means pushing a limit into the remote call instead.
There was a problem hiding this comment.
my intent was bounding the response rather than the work here, like sending back a huge response.
Pushing a limit into the remote call isn't available either: Iceberg 1.11.0 exposes only List<TableIdentifier> listTables(Namespace) and List<Namespace> listNamespaces(Namespace), with no limit or page-token parameter, so there's nothing to pass a bound through without an upstream API change. Bounding the actual work on federated catalogs looks like its own piece of work rather than something this PR can do.
Unless you think capping the response alone is worth the extra remote fetches, I'll drop the federated bounding here and keep the change to local catalogs. Let me know wdyt
There was a problem hiding this comment.
Federated REST Catalogs are fundamentally different from Federated non-REST Catalogs (e.g. Glue) as the Iceberg java catalog API is not isomorphic to the IRC API.
Properly forwarding pagination parameters to a Federated REST Catalog is possible, but I believe it will require a major code refactoring... certainly beyond the scope of this PR.
| * that a listing cannot return an unbounded response. A {@code maxPageSize} of zero or less means | ||
| * unlimited and leaves the request untouched. | ||
| */ | ||
| public static OptionalInt boundPageSize(OptionalInt requestedPageSize, int maxPageSize) { |
There was a problem hiding this comment.
This returns before maxPageSize is read, and LIST_PAGINATION_ENABLED defaults to false, so on a default deployment the ceiling never fires for a local catalog and listTables still returns every row. IMO a ceiling is an operator safety limit rather than a pagination feature, so I'd expect it to hold whether or not pagination is enabled. Right now an operator sets two flags to get one guarantee. WDYT?
There was a problem hiding this comment.
You're right that the ceiling is inert on a default deployment. I don't think it can be made to hold independently of LIST_PAGINATION_ENABLED though, because the flag doesn't just gate a feature — it gates the mechanism the ceiling depends on.
With pagination disabled, page tokens are not decoded. So if the ceiling applied anyway, a listing over the limit would return 100 rows and a null next-page-token, and the client would take that as the complete result: silent truncation. Emitting a token instead doesn't help, since the follow-up request carrying it would be ignored and served from the start again. A ceiling without a continuation mechanism isn't a ceiling, it's data loss — and that seems clearly worse than an unbounded response, especially for a client that never asked to paginate and is entitled under the IRC spec to expect everything in one response.
So I'd frame the two settings as mechanism and policy rather than two flags for one guarantee: LIST_PAGINATION_ENABLED makes paged listings possible, LIST_PAGINATION_MAX_PAGE_SIZE bounds them once they are. An operator who wants the safety limit does have to enable pagination, and I'm happy to make that dependency explicit in the config description so it isn't something they have to infer.
If the goal is for Polaris to be protected out of the box, I think the real lever is defaulting LIST_PAGINATION_ENABLED to true rather than decoupling the ceiling from it.
There was a problem hiding this comment.
+1 to @ayushtkn 's points.
I'd welcome defaulting LIST_PAGINATION_ENABLED to true, but it feels like it's beyond the scope of this PR 😅
There is an IRC spec compliance problem related to that too (discussed above). While I believe using limited and paginated responses by default is a good idea, I'd be cautious about it as some clients may expect strict behaviour per IRC spec. I think this needs a dev ML discussion and a separate PR.
The Iceberg REST specification treats a client's page-size as an upper bound, so a server is free to return fewer results, but Polaris currently honours whatever size a client asks for — there is no server-side ceiling, so a single request can ask for an arbitrarily large page. This adds
LIST_PAGINATION_MAX_PAGE_SIZE(default100, overridable per catalog viapolaris.config.list-pagination-max-page-size) and reduces any larger requested page size to it, onlistNamespaces,listTablesandlistViews. A request above the limit is reduced rather than rejected, since rejecting would break callers that pass a large page size today and succeed; a requested size of 0 is unchanged, and negative values continue to be rejected upstream as before. The bound is applied to both local and federated catalogs.For local catalogs it is applied before the page token is built, so it sits above the persistence layer and takes effect uniformly for every persistence backend rather than being implemented per backend.
Federated catalogs need it independently, because those listings are paginated by Polaris itself — the full result set is retrieved from the remote catalog and then sliced in memory by
CatalogHandlerUtils— and that path deliberately does not consultLIST_PAGINATION_ENABLED, so bounding only the flag-gated paths would have left the one listing path that always paginates as the only unbounded one.Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)