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>
In-Reply-To: <CAApHDvr8ay+31Wd0TptDGp8cAg2-NOnWddx8csnUE3R03EbvZw@mail.gmail.com>
From: Hannu Krosing <hannuk@google.com>
Date: Wed, 14 Jan 2026 11:52:04 +0100
Message-ID: <CAMT0RQT_yk32+ZyA-cBS9uMtAUOM-b+hqj0zT0Ge-ikFMrmw+A@mail.gmail.com>
Subject: Re: Patch: dumping tables data in multiple chunks in pg_dump
To: David Rowley <dgrowleyml@gmail.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"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAMT0RQT_yk32%2BZyA-cBS9uMtAUOM-b%2Bhqj0zT0Ge-ikFMrmw%2BA%40mail.gmail.com>
Precedence: bulk

On Tue, Jan 13, 2026 at 3:27=E2=80=AFAM David Rowley <dgrowleyml@gmail.com>=
 wrote:
>
> On Fri, 14 Nov 2025 at 09:34, Hannu Krosing <hannuk@google.com> wrote:
> > Added to https://commitfest.postgresql.org/patch/6219/
>
> I think this could be useful, but I think you'll need to find a way to
> not do this for non-heap tables. Per the comments in TableAmRoutine,
> both scan_set_tidrange and scan_getnextslot_tidrange are optional
> callback functions and the planner won't produce TIDRangePaths if
> either of those don't exist. Maybe that means you need to consult
> pg_class.relam to ensure the amname is 'heap' or at least the relam =3D
> 2.

Makes sense, will add.

> On testing Citus's columnar AM, I get:
> postgres=3D# select * from t where ctid between '(0,1)' and '(10,0)';
> ERROR:  UPDATE and CTID scans not supported for ColumnarScan

Should we just silently not chunk tables that have some storage
architecture that does not have tids, or should pg_dump just error out
in thiscase ?

I imagine the Citus columnar is often used with huge tables where
chunking would be most useful.

Later it likely makes sense to have another option for chunking other
types of tables, or maybe evan add something to the TableAM for
chunking support.

> 1. For the patch, I think you should tighten the new option up to mean
> the maximum segment size that a table will be dumped in. I see you
> have comments like:
>
> /* TODO: add hysteresis here, maybe < 1.1 * huge_table_chunk_pages */
>
> You *have* to put the cutoff *somewhere*, so I think it very much
> should be exactly the specified threshold. If anyone is unhappy that
> some segments consist of a single page, then that's on them to adjust
> the parameter accordingly. Otherwise, someone complaints that they got
> a 1-page segment when the table was 10.0001% bigger than the cutoff
> and then we're tempted to add a new setting to control the 1.1 factor,
> which is just silly. If there's a 1-page segment, so what? It's not a
> big deal.

Agreed, will drop the TODO

> Perhaps --max-table-segment-pages is a better name than
> --huge-table-chunk-pages as it's quite subjective what the minimum
> number of pages required to make a table "huge".

I agree. My initial thinking was that it is mainly useful for huge
tables, but indeed that does not need to be reflected in the flag name

> 2. I'm not sure if you're going to get away with using relpages for
> this. Is it really that bad to query pg_relation_size() when this
> option is set? If it really is a problem, then maybe let the user
> choose with another option. I understand we're using relpages for
> sorting table sizes so we prefer dumping larger tables first, but that
> just seems way less important if it's not perfectly accurate.

Yeah, I had thought of pg_relation_size() myself.

Another option would be something more complex which tries to estimate
the dump file sizes by figuring out  TOAST for each chunk. The think
that makes this really complex is the possible uneven distribution of
toast and needing to take into account both the compression of toast
AND the compression of resulting dump file.

> 3. You should be able to simplify the code in dumpTableData() so
> you're not adding any extra cases. You could use InvalidBlockNumber to
> indicate an unbounded ctid range and only add ctid qual to the WHERE
> clause when you have a bounded range (i.e not InvalidBlockNumber).
> That way the first segment will need WHERE ctid <=3D '...' and the final
> one will need WHERE ctid >=3D '...'.  Everything in between will have an
> upper and lower bound. That results in no ctid quals being added when
> both ranges are set to InvalidBlockNumber, which you should use for
> all tables not large enough to be segmented, thus no special case.

Makes sense, will look into it.

> TID Range scans are perfectly capable of working when only bounded at one=
 side.
>
> 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.

There can be no more than 2 * INT2_MAX pages anyway.
I thought half of the max possible size should be enough.
Do you really think that somebody would want that ?

> David