Erroring out for the incomplete list sounds good to me. Thanks for the discussion!
Yufei On Sat, Sep 19, 2026 at 9:00 PM Ayush Saxena <[email protected]> wrote: > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
