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>
 <CAN4CZFOTOX1nFkPrmM7fQ9qnrccvjwywXPf4Eo7C+DF-h-x96g@mail.gmail.com>
 <CAMT0RQTbQxjdN5nv6M_HhFWiLqdT84=NYBM1ZYQKaAcf8Ufyaw@mail.gmail.com>
 <CAN4CZFOjG2kUciKeVBpxrHJZNkZuzY_Q5ij_qpDZcAS3Ak2GxA@mail.gmail.com>
 <CAMT0RQStjytRrGTe0X03ErC7anwxNRHAULYBsSmdWZV3fr4-Dg@mail.gmail.com>
 <CAMT0RQROPMSPwfxAUCm1gZs9cUr7FmvwX+eO6Wzq_wWdd6eEAQ@mail.gmail.com>
 <CAMT0RQQ8DX+K7OTw3Lg+Yp2ew8TsZduiqtPszfiBixcpxKbz-A@mail.gmail.com>
 <CAApHDvo29-vQz=xV6+x5hU--NZ9qGPXsCNBuOAf88pAHjTpvvQ@mail.gmail.com>
 <CAMT0RQSXhbLPe3Tru-CU+tB4yrzYWx4P6g9R6gya13ckhqVMyA@mail.gmail.com> <CAMT0RQQuMNkgJBQhWwWKFX5oqPPCXYFYf=_iTKAFgWJHNOrJig@mail.gmail.com>
In-Reply-To: <CAMT0RQQuMNkgJBQhWwWKFX5oqPPCXYFYf=_iTKAFgWJHNOrJig@mail.gmail.com>
From: Zsolt Parragi <zsolt.parragi@percona.com>
Date: Tue, 3 Feb 2026 21:10:12 +0000
Message-ID: <CAN4CZFMHr0gFx-DpKBy4SkVzuJC5f3S=pnkrMDC54OswBSuJNQ@mail.gmail.com>
Subject: Re: Patch: dumping tables data in multiple chunks in pg_dump
To: Hannu Krosing <hannuk@google.com>
Cc: David Rowley <dgrowleyml@gmail.com>, 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/CAN4CZFMHr0gFx-DpKBy4SkVzuJC5f3S%3DpnkrMDC54OswBSuJNQ%40mail.gmail.com>
Precedence: bulk

Hello!

I did some testing with this patch, and I think there are some issues
during restoration:

1. Isn't there a possible race / scheduling mistake during restore
because of missing dependencies? The code now prints out "TABLE DATA
(pages %u:%u)", while the restore code checks for the explicit "TABLE
DATA" string for dependency tracking (pg_backup_archiver.c:2013 and a
few other places). This causes POST DATA to have no dependency on the
table data, and can be scheduled before we load all table data.

I was able to verify the scheduling issue with an index: the INDEX
part is scheduled too early, before all TABLE DATA completes, but then
locking prevents it from progressing, so everything completed fine in
the end. Even if that's guaranteed, which I'm not 100% sure of, it's
still based on luck and not proper logic, and takes up a slot (or
multiple), reducing parallelism.

2. Fixing the TABLE DATA strcmp checks solves the scheduling issue,
but it's not that simple, because then it causes truncation issues
during restore, which needs additional changes in the restore code. I
did a quick fix for that by adding an additional condition to the
created flag, and with that it seems to restore everything properly,
and with proper ordering, only starting index/constraint/etc after all
table data is completed. However this was definitely just a quick test
fix, this needs a proper better solution.

Other issues I see are more minor, but numerous:

3. The patch still has lots of debug output (pg_log_WARNING("CHUNKING
...")); Is this intended? Shouldn't these be behind some verbose
check, and maybe use info instead of warning?

4. The is_segment macro should have () around the use of tdiptr

5. There's still a 32000 magic constant, shouldn't that have some
descriptive name / explanatory comment?

6. formatting issues at multiple places, mostly missing spaces after
if/while/for statements

7. inconsistent error messages (not in range vs must be in range)

8. There's a remaining TODO that seems stale, current_chunk_start is
already uint64

9. typo: "the computed from pog_relation_size" -> "then computed from
pg_relation_size"