Thanx Everyone for the feedback so far. I have created a PR to reject in case of overflow [1], in case the discussion drifts in different direction, will update accordingly.
-Ayush [1] https://github.com/apache/polaris/pull/5553 On 2026/09/19 12:35:24 vignesh a wrote: > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
