MIME-Version: 1.0
References: <CAMT0RQT_0qVxcTT6ycM20QUN-pEQ6iMLbz6gLWgLpeF0NmNOUA@mail.gmail.com>
 <CAExHW5t54GPKFbW3KLzintJ6jMMRYwb-t2Fjm4JTxEcZbGDomA@mail.gmail.com>
 <CAMT0RQTHoL8S7OonFWC_aDSC-2oX7BGBBLAQ+OOBhRPcxV2eiw@mail.gmail.com>
 <CAMT0RQQAH1a8kY-mx7B07Uzn3T_zeaU9detqFFtW36_k67Su+A@mail.gmail.com>
 <CAMT0RQQr7KtPAY903+F42csiHc1EPHo70Xji-znkxEhwdoKa6w@mail.gmail.com>
 <CAMT0RQSNHFffbCmDNxQogVBD8H5gTDJNwhUR2btCVE+Lq1sGGw@mail.gmail.com>
 <CAMT0RQTEFGctCfgVx3u2XgVRCAj_QURV2tfdzL0HOQi=u0sV2A@mail.gmail.com>
 <CAApHDvr8ay+31Wd0TptDGp8cAg2-NOnWddx8csnUE3R03EbvZw@mail.gmail.com> <CAMT0RQShjXPPdXQS-5uzDC3bXt+QEZR5tO02o1NHdXWNu2quvw@mail.gmail.com>
In-Reply-To: <CAMT0RQShjXPPdXQS-5uzDC3bXt+QEZR5tO02o1NHdXWNu2quvw@mail.gmail.com>
From: David Rowley <dgrowleyml@gmail.com>
Date: Tue, 20 Jan 2026 15:20:13 +1300
Message-ID: <CAApHDvruGg8u9t2OQbVU+Sxe31H0CuJi5S69FjZXds1Y3Fx3HA@mail.gmail.com>
Subject: Re: Patch: dumping tables data in multiple chunks in pg_dump
To: Hannu Krosing <hannuk@google.com>
Cc: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, 
	Nathan Bossart <nathandbossart@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CAApHDvruGg8u9t2OQbVU%2BSxe31H0CuJi5S69FjZXds1Y3Fx3HA%40mail.gmail.com>
Precedence: bulk

On Tue, 20 Jan 2026 at 08:01, Hannu Krosing <hannuk@google.com> wrote:
> I changed the last open-ended chunk to use ctid >= (N,1) for clarity
> but did not change anything else.

You have:

int max_table_segment_pages; /* chunk when relpages is above this */

and:

opts->max_table_segment_pages = UINT32_MAX; /* == InvalidBlockNumber,
disable chunking by default */

It's not valid to assign UINT32_MAX to a signed int.


> To me it looked like having a loop around the whole thing when there
> is no chunking would complicate things for anyone reading the code.

The problem I have with it is the duplicate code. If you don't want to
loop around the standard code, then make a function and call that
instead of copying and pasting the code.

I'd also get rid of the "chunking" boolean and make use of
InvalidBlockNumber to determine if the range is constrained. It also
seems very strange that you opted to do that just for endPage and not
for startPage.

> > 4. I think using "int" here is a future complaint waiting to happen.
> >
> > + if (!option_parse_int(optarg, "--huge-table-chunk-pages", 1, INT32_MAX,
> > +   &dopt.huge_table_chunk_pages))
> >
> > I bet we'll eventually see a complaint that someone can't make the
> > segment size larger than 16TB. I think option_parse_uint32() might be
> > called for.
>
> I have not yet done anything with this yet, so the maximum chunk size
> for now is half of the maximum relpages.

OK. I can look again once all that's done.

David