Hi all, +1 to fail-closed on the existing LIST_PAGINATION_MAX_PAGE_SIZE, and I don't think we need another flag.
A truncated 200 on a request that never asked to page is the bad outcome. Rejecting when the unsized result would exceed the configured maximum is the contract that stays diagnosable: paginated requests are capped, an unsized request that fits is complete, one that would overflow fails instead of looking complete. Prithvi's two details have to land in the follow-up or we only fix the client lie: 1. Probe at max+1 and reject if more remain. Listing everything into heap and then throwing still pays the cost Ayush described. 2. Missing pageToken is unsized. Empty pageToken is pagination. Java Iceberg sends pageToken= and will follow next-page-token; PageTokenUtil currently treats null and "" the same, and that has to change for reject or we 400 a client that was already prepared to page. Federated listing has the same silent truncation today (null pageToken becomes the initial empty token when the max is set). Reject has to apply there too; CatalogHandlerUtils still materializes the full remote list before slicing, so max+1 on that path is a separate follow-up. Default stays off. Thanks, Vignesh On Sat, 19 Sept 2026 at 15:10, Prithvi S <[email protected]> wrote: > Hi all, > > Thank you for laying this out so clearly. Yufei is right that the current > on-path behaviour is the problem worth fixing: a 200 with a continuation > token, on a request that never asked for pagination, is a silently > incomplete list. That is worse for catalog correctness than a loud failure. > Robert already flagged the same class of client on #5282 (PyIceberg 0.10 / > 0.11 does one unparameterized list and returns it). #5533 shows it closer > to home: the generic-tables CLI follows next-page-token only when > --page-size is set, so a configured maximum would drop the rest of the > tables with no error. > I don't think we need a second flag. > Option 1 (cap an explicit pageSize, leave unsized requests complete) is the > original #5282 ceiling, and it is Iceberg-legal: pageSize is an upper > bound. It is not server protection. A client that sends no pageToken is the > one asking for every identifier under the namespace; exempting that request > leaves the unbounded case untouched. Option 2 (force pagination on that > request) does bound it, which is why the review grew in that direction, but > returning a partial 200 is the wrong shape for that bound. > > Dmitri's fail-closed rule, is the contract I would want from the existing > setting: > • paginated request: cap to LIST_PAGINATION_MAX_PAGE_SIZE > • unsized request that fits: complete list, null next-page-token > • unsized request that would exceed: reject, not a truncated 200 > Default stays -1, so nothing changes for deployments that have not opted > in. LIST_PAGINATION_ENABLED=false remains the escape hatch that ignores > pagination parameters. Operators who need a ceiling turn it on and get a > bound they can state and diagnose. Clients that believe they hold a > complete list either do, or they get an error telling them to paginate or > raise the limit. > > Two implementation details I think the follow-up PR has to get right, or > reject will not do what we are claiming. > 1. Reject has to stop at max+1, not list everything and then throw. > Page.mapped already stops after pageSize items when pagination is > requested, and the JDBC listing path has LIMIT. If the unsized path still > materializes the full result into a List, copies it into the response, and > serializes it, we pay the heap and concurrency cost Ayush described and > then fail. That protects the client from a lie; it does not protect the > server. Probe with size max+1; if a continuation would have been produced, > return 400 with the configured maximum in the message. The federated path > needs the same rule. Today, when the maximum is set and pageToken is null, > IcebergCatalogHandler injects INITIAL_PAGE_TOKEN so the remote listing is > sliced in memory. That is the same silent truncation. Request at most max+1 > from the remote catalog and reject if more remain. > 2. Distinguish a missing pageToken from an empty one. The Iceberg REST spec > is explicit: clients start pagination by sending an empty pageToken; if the > parameter is not set, the server must return the complete result. Java > Iceberg always sends pageToken= and follows next-page-token even when > rest-page-size is unset. PageTokenUtil currently treats null and "" the > same, which is harmless for force-pagination and harmful for reject: a > naive "unsized means reject if over max" would 400 the Java client that was > already prepared to page. Reject only when pageToken is absent (and > pageSize is absent). Empty pageToken, or an explicit pageSize, is > pagination and should be capped. > > On the overload question: Yufei's uncompressed JSON sizes are right for the > wire. I have not seen a published listTables OOM either. I still think a > default-off ceiling is a reasonable operator tool, peak heap is several > in-memory copies, held for the duration of the response, multiplied by > concurrency, and federation currently pulls the remote list into Polaris > before slicing it. We don't need a production incident to keep the knob. We > do need the knob not to return a 200 that looks complete and is not. > > Thanks, > Prithvi S > > On Sat, Sep 19, 2026 at 7:02 AM Ayush Saxena <[email protected]> > wrote: > > > Hi Yufei, Dmitri, > > Thanks both for working through this. > > > > I'm inclined towards Dmitri's suggestion of throwing an exception. It > > gives a contract that is easy to state and easy to diagnose: when a > > maximum is configured, that value is the bound the server will honour. > > A paginated request is capped to it; an unpaged request that fits still > > gets the complete list; an unpaged request that would exceed it is > > rejected rather than quietly truncated. A client that believes it holds > > a complete list when it does not is the failure worth designing against. > > If that sounds reasonable, I can raise a PR for it. > > > > On the overload question, I don't think the per-response payload is the > > whole picture. For a request without pagination the full result set is > > materialized in heap before anything is written — Page.mapped collects > > every row into a list, the handler copies it into the response > > collection, and it is then serialized — so peak memory is a multiple of > > the figures in the table, and it is held for the duration of the > > response rather than streamed. That cost also multiplies with > > concurrency rather than with a single listing, and a large response > > occupies the connection while it is written, so a handful of > > simultaneous unbounded lists is a different proposition from one. A > > configurable ceiling is what lets an operator put a bound on that. > > > > For context on where the flag came from: it started with a review > > comment [1]. > > > > -Ayush > > [1] https://github.com/apache/polaris/pull/5255#discussion_r3739619320 > > > > On 2026/09/19 00:02:17 Dmitri Bourlatchkov wrote: > > > Hi Yufei, > > > > > > > I worry we’re introducing a protocol violation for a risk whose > > practical > > > > impact we haven't established, while the possibility of silently > > > incomplete > > > > results is clear > > > > > > I propose [1] to address the protocol compatibility issue by failing > > > non-paginated > > > requests whose response overflow the LIST_PAGINATION_MAX_PAGE_SIZE > > > value (if configured). > > > > > > > why do we need the PR 5282? > > > > > > As for me, the rationale in the PR description is sufficient to justify > > the > > > code change. > > > > > > I hope Ayush may be able to share some practical considerations that > led > > to > > > this PR. > > > > > > [1] https://lists.apache.org/thread/ntn71oh7g0kkf1wdhskh8t986gdwh1p3 > > > > > > Cheers, > > > Dmitri. > > > > > > On Fri, Sep 18, 2026 at 7:21 PM Yufei Gu <[email protected]> wrote: > > > > > > > Hi Ayush, Dmitri, > > > > > > > > If option 1 isn't necessary, why do we need the PR 5282? Have we > seen > > list > > > > requests cause OOM in a real deployment, or reproduced this with a > > > > realistic workload? > > > > List responses contain names, not full table metadata. Let's use > > > > uncompressed JSON payload sizes as a reference: > > > > > > > > Table-name length 10K tables 100K tables > > > > 20 ASCII characters 0.55 MiB 5.53 MiB > > > > 50 ASCII characters 0.84 MiB 8.39 MiB > > > > 100 ASCII characters 1.32 MiB 13.16 MiB > > > > > > > > If the number of tables exceeds 100k under a single namespace, the > > system > > > > may have problems beyond just the list operation, for example, the > > overlap > > > > checking that can enumerate and resolve all siblings. > > > > > > > > I worry we’re introducing a protocol violation for a risk whose > > practical > > > > impact we haven't established, while the possibility of silently > > incomplete > > > > results is clear > > > > > > > > > > > > Yufei > > > > > > > > > > > > On Fri, Sep 18, 2026 at 8:43 AM Dmitri Bourlatchkov < > [email protected]> > > > > wrote: > > > > > > > > > Hi All, > > > > > > > > > > Many thanks to Ayush for referencing the earlier GH review > discussion > > > > [1]! > > > > > > > > > > From my POV, the main benefit of LIST_PAGINATION_MAX_PAGE_SIZE is > > capping > > > > > _all_ responses to protect servers from overload. > > > > > > > > > > Even now, without any other config options, administrators are able > > to > > > > > set LIST_PAGINATION_MAX_PAGE_SIZE to a negative value (default) > thus > > > > > enabling strict adherence to the IRC spec. > > > > > > > > > > If the risk of overload / OOM in the servers is substantial, the > > admin > > > > can > > > > > set LIST_PAGINATION_MAX_PAGE_SIZE to a positive value. This indeed > > will > > > > be > > > > > a deviation from the IRC spec. However, I think it is a > > > > > reasonable deviation when overload is a risk. A protocol spec ought > > not > > > > to > > > > > force implementations into behaviours susceptible to DoS attacks or > > > > general > > > > > overload under normal circumstances. > > > > > > > > > > I do not see a rationale for enforcing > LIST_PAGINATION_MAX_PAGE_SIZE > > > > _only_ > > > > > when the client expliocitly requests pagination. However, if other > > people > > > > > prefer adding yet another flag, I think it would be fine as long as > > > > > administrators can still apply global page size limits based on > their > > > > > deployment requirements. > > > > > > > > > > Another option is to error out of non-paginated requests > > > > > when LIST_PAGINATION_MAX_PAGE_SIZE is in effect. This will maintain > > > > strict > > > > > IRC spec compatibility at the expense of failing risky requests. > > Perhaps > > > > > this is the cleanest approach from the troubleshooting perspective. > > WDYT? > > > > > > > > > > [1] > > https://github.com/apache/polaris/pull/5282#discussion_r3768478185 > > > > > > > > > > Cheers, > > > > > Dmitri. > > > > > > > > > > > > > > > On Fri, Sep 18, 2026 at 8:01 AM Ayush Saxena < > [email protected] > > > > > > > > wrote: > > > > > > > > > > > Hi Yufei, > > > > > > > > > > > > Agreed that there's currently no way to get option 1 on its own — > > > > that's > > > > > a > > > > > > fair gap. > > > > > > > > > > > > Option 2 isn't incidental though; It came out of the #5282 review > > [1], > > > > > > where the concern was that a large catalog is otherwise exposed > to > > OOM > > > > > and > > > > > > DoS through arbitrary unbounded list queries. A cap that exempts > > > > requests > > > > > > without pageToken doesn't bound that case at all — a client > > sending no > > > > > > pagination parameters is precisely the one asking for every table > > in a > > > > > > namespace, so the cap would only restrain clients that were > already > > > > > > paginating. > > > > > > > > > > > > That's why it's left to the operator and defaults to off: where > > clients > > > > > > relying on a complete response are known to exist, it stays > > disabled as > > > > > it > > > > > > is today; where they are known not to exist, or supporting them > > isn't > > > > > > intended, it can be turned on as added protection. > > > > > > > > > > > > Supporting both looks reasonable, with one more flag to choose > > between > > > > > > them. If folks think we should add one more flag to toggle > between > > > > this, > > > > > we > > > > > > can add that. > > > > > > > > > > > > -Ayush > > > > > > > > > > > > [1] > > https://github.com/apache/polaris/pull/5282#discussion_r3768478185 > > > > > > > > > > > > On 2026/09/18 05:13:35 Yufei Gu wrote: > > > > > > > Hi all, > > > > > > > > > > > > > > While reviewing PR #5533, I noticed that when a client doesn't > > send > > > > > page > > > > > > > token/size, the server can send partial result, which is a > > violation > > > > of > > > > > > IRC > > > > > > > spec. It was enabled by the LIST_PAGINATION_MAX_PAGE_SIZE > > introduced > > > > in > > > > > > > #5282. I think it combines two behaviors: > > > > > > > > > > > > > > 1. Cap page sizes when the client explicitly opts into > > pagination > > > > > via > > > > > > > pageToken, while returning all results for requests without > > it. > > > > This > > > > > > > preserves the IRC contract. > > > > > > > 2. Force pagination even when the client expects a complete > > > > > response, > > > > > > > deviating from IRC. > > > > > > > > > > > > > > Currently, setting LIST_PAGINATION_MAX_PAGE_SIZE enables both. > > > > > Defaulting > > > > > > > it to unlimited avoids changing defaults, but administrators > > still > > > > > cannot > > > > > > > choose option 1 alone. > > > > > > > > > > > > > > Option 1 is valid. Option 2 can silently produce incorrect > > results > > > > for > > > > > > IRC > > > > > > > clients. I'm not sure how much value Option 2 provides. It may > > > > > > potentially > > > > > > > protect the server from OOM, I guess. Please chime in the use > > cases. > > > > > > > > > > > > > > If we really want to keep option 2, could we make it a > separate, > > > > > explicit > > > > > > > setting? > > > > > > > > > > > > > > Thoughts? > > > > > > > > > > > > > > Related discussion: > > > > > > > > https://lists.apache.org/thread/mmqllcbt2lfsrq571cfwbymgfwoxqdf0 > > > > > > > > > > > > > > Yufei > > > > > > > > > > > > > > > > > > > > > > > > > > > >
