From: Oleg Tkachenko <oatkachenko@gmail.com>
Message-Id: <22474984-C4CD-4300-A84E-C1CA58F61CDA@gmail.com>
Content-Type: multipart/alternative;
	boundary="Apple-Mail=_2DE1DA9E-03B4-4465-9A4F-35CA96FC234D"
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.200.81.1.6\))
Subject: Re: [BUG] [PATCH] pg_basebackup produces wrong incremental files
 after relation truncation in segmented tables
Date: Mon, 15 Dec 2025 19:46:23 +0100
In-Reply-To: 
 <CA+TgmoZ7LQ40tLGpHw0U51CReH_8jv0sO0dALGRJp=FT+ZtGFQ@mail.gmail.com>
Cc: Amul Sul <sulamul@gmail.com>,
 pgsql-hackers@lists.postgresql.org,
 Stanislav Bashkyrtsev <stanislav.bashkyrtsev@elsci.io>
To: Robert Haas <robertmhaas@gmail.com>
References: <00FEFC88-EA1D-4271-B38F-EB741733A84A@gmail.com>
 <F687B661-E975-4383-8A94-F9130FE7ACC1@gmail.com>
 <CAAJ_b94y98BWWgf0KMw-E_pqM_y8+RogqHtu5nK0kA1uBE3WWw@mail.gmail.com>
 <CA+TgmoZ7LQ40tLGpHw0U51CReH_8jv0sO0dALGRJp=FT+ZtGFQ@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/22474984-C4CD-4300-A84E-C1CA58F61CDA%40gmail.com>
Precedence: bulk


--Apple-Mail=_2DE1DA9E-03B4-4465-9A4F-35CA96FC234D
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8

Hello Robert,

Thank you for the explanation.

At first, I also thought about clamping truncation_block_length to =
RELSEG_SIZE. But I hesitated because I thought the reconstructed =
relation file couldn=E2=80=99t be larger than relative_limit.

After reading the reconstruction code and the comments on top of the =
discussed block of code (many times), I finally understood that =
truncation_block_length is the minimum length of the reconstructed file, =
not just a safety limit. It determines which blocks must be fetched from =
older backups. So a simple clamp could change how reconstruction works =
if some blocks are included in incremental backups.

I=E2=80=99ve tested the version with the limit enforced to RELSEG_SIZE, =
and it works correctly.

Also, I=E2=80=99ve attached a patch based on your guidance. The changes =
are effectively the same as your suggested approach, but I would be =
happy to be listed as a contributor.

Regards,
Oleg Tkachenko

=EF=BF=BC
> On Dec 15, 2025, at 17:35, Robert Haas <robertmhaas@gmail.com> wrote:
>=20
> [ sorry for not noticing this thread sooner; thanks to Andres for
> pointing me to it ]
>=20
> On Mon, Dec 15, 2025 at 9:01=E2=80=AFAM Amul Sul <sulamul@gmail.com> =
wrote:
>> Thanks for the reproducer; I can see the reported issue, but I am not
>> quite sure the proposed fix is correct and might break other cases (I
>> haven't tried constructed that case yet) but there is a comment
>> detailing that case just before the point where you are planning to =
do
>> the changes:
>>=20
>>    /*
>>     * The truncation block length is the minimum length of the =
reconstructed
>>     * file. Any block numbers below this threshold that are not =
present in
>>     * the backup need to be fetched from the prior backup. At or =
above this
>>     * threshold, blocks should only be included in the result if they =
are
>>     * present in the backup. (This may require inserting zero blocks =
if the
>>     * blocks included in the backup are non-consecutive.)
>>     */
>>=20
>> IIUC, we might need the original assignment logic as it is. But we
>> need to ensure that truncation_block_length is not set to a value =
that
>> exceeds RELSEG_SIZE.
>=20
> I think you're right. By way of example, let's say that the current
> length of the file is 200 blocks, but the limit block is 100 blocks
> into the current segment. That means that the only blocks that we can
> get from any previous backup are blocks 0-99. Blocks 100-199 of the
> current segment are either mentioned in the WAL summaries we're using
> for this backup, or they're all zeroes. We can't set the
> truncation_block_length to a value greater than 100, or we'll go
> looking for the contents of any zero-filled blocks in previous
> backups, will will either fail or produce the wrong answer. But Oleg
> is correct that we also shouldn't set it to a value greater than
> RELSEG_SIZE. So my guess is that the correct fix might be something
> like the attached (untested, for discussion).
>=20
> --=20
> Robert Haas
> EDB: http://www.enterprisedb.com
=EF=BF=BC

--Apple-Mail=_2DE1DA9E-03B4-4465-9A4F-35CA96FC234D
Content-Type: multipart/mixed;
	boundary="Apple-Mail=_40CFB0D5-5304-4E38-8DBC-298E9786F080"


--Apple-Mail=_40CFB0D5-5304-4E38-8DBC-298E9786F080
Content-Transfer-Encoding: quoted-printable
Content-Type: text/html;
	charset=utf-8

<html aria-label=3D"message body"><head><meta http-equiv=3D"content-type" =
content=3D"text/html; charset=3Dutf-8"></head><body =
style=3D"overflow-wrap: break-word; -webkit-nbsp-mode: space; =
line-break: after-white-space;"><div><div>Hello =
Robert,</div><div><br></div><div>Thank you for the =
explanation.</div><div><br></div><div>At first, I also thought about =
clamping truncation_block_length to RELSEG_SIZE. But I hesitated because =
I thought the reconstructed relation file couldn=E2=80=99t be larger =
than relative_limit.</div><div><br></div><div>After reading the =
reconstruction code and the comments on top of the discussed block of =
code (many times), I finally understood that truncation_block_length is =
the <b><u>minimum</u></b> length of the reconstructed file, <b><u>not =
just a safety limit.</u></b> It determines which blocks must be fetched =
from older backups. So a simple clamp could change how reconstruction =
works if some blocks are included in incremental =
backups.</div><div><br></div><div>I=E2=80=99ve tested the version with =
the limit enforced to RELSEG_SIZE, and it works =
correctly.</div><div><br></div><div>Also, I=E2=80=99ve attached a patch =
based on your guidance. The changes are effectively the same as your =
suggested approach, but I would be happy to be listed as a =
contributor.</div><div><br></div><div>Regards,</div><div>Oleg =
Tkachenko</div></div><div><br></div><div></div></body></html>=

--Apple-Mail=_40CFB0D5-5304-4E38-8DBC-298E9786F080
Content-Disposition: attachment;
	filename=0001-Clamp-truncation_block_length-to-RELSEG_SIZE.patch
Content-Type: application/octet-stream;
	x-unix-mode=0644;
	name="0001-Clamp-truncation_block_length-to-RELSEG_SIZE.patch"
Content-Transfer-Encoding: quoted-printable

=46rom=20f8ab0228c1125e07e81b7f1aeaee6e5af2370b21=20Mon=20Sep=2017=20=
00:00:00=202001=0AFrom:=20Oleg=20Tkachenko=20<oleg@elsci.io>=0ADate:=20=
Mon,=2015=20Dec=202025=2019:29:50=20+0100=0ASubject:=20[PATCH]=20Clamp=20=
truncation_block_length=20to=20RELSEG_SIZE=0A=0AFix=20an=20issue=20where=20=
truncation_block_length=20could=20exceed=20RELSEG_SIZE,=0Acausing=20=
restore=20failures=20when=20reconstructing=20incremental=20backups.=0A=
---=0A=20src/backend/backup/basebackup_incremental.c=20|=2014=20=
++++++++++++++=0A=201=20file=20changed,=2014=20insertions(+)=0A=0Adiff=20=
--git=20a/src/backend/backup/basebackup_incremental.c=20=
b/src/backend/backup/basebackup_incremental.c=0Aindex=20=
7678e4f6ec3..f4ef33f6672=20100644=0A---=20=
a/src/backend/backup/basebackup_incremental.c=0A+++=20=
b/src/backend/backup/basebackup_incremental.c=0A@@=20-850,8=20+850,22=20=
@@=20GetFileBackupMethod(IncrementalBackupInfo=20*ib,=20const=20char=20=
*path,=0A=20=09{=0A=20=09=09unsigned=09relative_limit=20=3D=20=
limit_block=20-=20segno=20*=20RELSEG_SIZE;=0A=20=0A+=09=09/*=0A+=09=09=20=
*=20We=20can't=20set=20a=20truncation_block_length=20in=20excess=20of=20=
the=20limit=20block=0A+=09=09=20*=20number=20(relativized=20to=20the=20=
current=20segment).=20To=20do=20so=20would=20be=20to=0A+=09=09=20*=20=
treat=20blocks=20from=20older=20backups=20as=20valid=20current=20=
contents=20even=20if=0A+=09=09=20*=20they=20were=20subsequently=20=
truncated=20away.=0A+=09=09=20*/=0A=20=09=09if=20=
(*truncation_block_length=20<=20relative_limit)=0A=20=09=09=09=
*truncation_block_length=20=3D=20relative_limit;=0A+=0A+=09=09/*=0A+=09=09=
=20*=20We=20also=20can't=20set=20a=20truncation_block_length=20in=20=
excess=20of=20the=0A+=09=09=20*=20segment=20size,=20since=20the=20=
reconstructed=20file=20can't=20be=20larger=20than=0A+=09=09=20*=20that.=0A=
+=09=09=20*/=0A+=09=09if=20(*truncation_block_length=20>=20RELSEG_SIZE)=0A=
+=09=09=09*truncation_block_length=20=3D=20RELSEG_SIZE;=0A=20=09}=0A=20=0A=
=20=09/*=20Send=20it=20incrementally.=20*/=0A--=20=0A2.52.0=0A=0A=

--Apple-Mail=_40CFB0D5-5304-4E38-8DBC-298E9786F080
Content-Transfer-Encoding: quoted-printable
Content-Type: text/html;
	charset=utf-8

<html aria-label=3D"message body"><body style=3D"overflow-wrap: =
break-word; -webkit-nbsp-mode: space; line-break: =
after-white-space;"><head><meta http-equiv=3D"content-type" =
content=3D"text/html; charset=3Dutf-8"></head><div =
class=3D"AppleOriginalContents"><br><blockquote type=3D"cite"><div>On =
Dec 15, 2025, at 17:35, Robert Haas &lt;robertmhaas@gmail.com&gt; =
wrote:</div><br class=3D"Apple-interchange-newline"><div><div>[ sorry =
for not noticing this thread sooner; thanks to Andres for<br>pointing me =
to it ]<br><br>On Mon, Dec 15, 2025 at 9:01=E2=80=AFAM Amul Sul =
&lt;sulamul@gmail.com&gt; wrote:<br><blockquote type=3D"cite">Thanks for =
the reproducer; I can see the reported issue, but I am not<br>quite sure =
the proposed fix is correct and might break other cases (I<br>haven't =
tried constructed that case yet) but there is a comment<br>detailing =
that case just before the point where you are planning to do<br>the =
changes:<br><br> &nbsp;&nbsp;&nbsp;/*<br> &nbsp;&nbsp;&nbsp;&nbsp;* The =
truncation block length is the minimum length of the reconstructed<br> =
&nbsp;&nbsp;&nbsp;&nbsp;* file. Any block numbers below this threshold =
that are not present in<br> &nbsp;&nbsp;&nbsp;&nbsp;* the backup need to =
be fetched from the prior backup. At or above this<br> =
&nbsp;&nbsp;&nbsp;&nbsp;* threshold, blocks should only be included in =
the result if they are<br> &nbsp;&nbsp;&nbsp;&nbsp;* present in the =
backup. (This may require inserting zero blocks if the<br> =
&nbsp;&nbsp;&nbsp;&nbsp;* blocks included in the backup are =
non-consecutive.)<br> &nbsp;&nbsp;&nbsp;&nbsp;*/<br><br>IIUC, we might =
need the original assignment logic as it is. But we<br>need to ensure =
that truncation_block_length is not set to a value that<br>exceeds =
RELSEG_SIZE.<br></blockquote><br>I think you're right. By way of =
example, let's say that the current<br>length of the file is 200 blocks, =
but the limit block is 100 blocks<br>into the current segment. That =
means that the only blocks that we can<br>get from any previous backup =
are blocks 0-99. Blocks 100-199 of the<br>current segment are either =
mentioned in the WAL summaries we're using<br>for this backup, or =
they're all zeroes. We can't set the<br>truncation_block_length to a =
value greater than 100, or we'll go<br>looking for the contents of any =
zero-filled blocks in previous<br>backups, will will either fail or =
produce the wrong answer. But Oleg<br>is correct that we also shouldn't =
set it to a value greater than<br>RELSEG_SIZE. So my guess is that the =
correct fix might be something<br>like the attached (untested, for =
discussion).<br><br>-- <br>Robert Haas<br>EDB: =
http://www.enterprisedb.com<br></div></div></blockquote></div></body></htm=
l>=

--Apple-Mail=_40CFB0D5-5304-4E38-8DBC-298E9786F080
Content-Disposition: attachment;
	filename=v1-0001-Don-t-set-the-truncation_block_length-rather-than.patch
Content-Type: application/octet-stream;
	x-unix-mode=0644;
	name="v1-0001-Don-t-set-the-truncation_block_length-rather-than.patch"
Content-Transfer-Encoding: quoted-printable

=46rom=20bac3a98cd4e4b3007e140d9db334b040cf6772c5=20Mon=20Sep=2017=20=
00:00:00=202001=0AFrom:=20Robert=20Haas=20<rhaas@postgresql.org>=0ADate:=20=
Mon,=2015=20Dec=202025=2011:34:34=20-0500=0ASubject:=20[PATCH=20v1]=20=
Don't=20set=20the=20truncation_block_length=20rather=20than=0A=20=
RELSEG_SIZE.=0A=0AAlso,=20add=20some=20comments=20to=20explain=20the=20=
logic.=0A---=0A=20src/backend/backup/basebackup_incremental.c=20|=2014=20=
++++++++++++++=0A=201=20file=20changed,=2014=20insertions(+)=0A=0Adiff=20=
--git=20a/src/backend/backup/basebackup_incremental.c=20=
b/src/backend/backup/basebackup_incremental.c=0Aindex=20=
7678e4f6ec3..f4ef33f6672=20100644=0A---=20=
a/src/backend/backup/basebackup_incremental.c=0A+++=20=
b/src/backend/backup/basebackup_incremental.c=0A@@=20-850,8=20+850,22=20=
@@=20GetFileBackupMethod(IncrementalBackupInfo=20*ib,=20const=20char=20=
*path,=0A=20=09{=0A=20=09=09unsigned=09relative_limit=20=3D=20=
limit_block=20-=20segno=20*=20RELSEG_SIZE;=0A=20=0A+=09=09/*=0A+=09=09=20=
*=20We=20can't=20set=20a=20truncation_block_length=20in=20excess=20of=20=
the=20limit=20block=0A+=09=09=20*=20number=20(relativized=20to=20the=20=
current=20segment).=20To=20do=20so=20would=20be=20to=0A+=09=09=20*=20=
treat=20blocks=20from=20older=20backups=20as=20valid=20current=20=
contents=20even=20if=0A+=09=09=20*=20they=20were=20subsequently=20=
truncated=20away.=0A+=09=09=20*/=0A=20=09=09if=20=
(*truncation_block_length=20<=20relative_limit)=0A=20=09=09=09=
*truncation_block_length=20=3D=20relative_limit;=0A+=0A+=09=09/*=0A+=09=09=
=20*=20We=20also=20can't=20set=20a=20truncation_block_length=20in=20=
excess=20of=20the=0A+=09=09=20*=20segment=20size,=20since=20the=20=
reconstructed=20file=20can't=20be=20larger=20than=0A+=09=09=20*=20that.=0A=
+=09=09=20*/=0A+=09=09if=20(*truncation_block_length=20>=20RELSEG_SIZE)=0A=
+=09=09=09*truncation_block_length=20=3D=20RELSEG_SIZE;=0A=20=09}=0A=20=0A=
=20=09/*=20Send=20it=20incrementally.=20*/=0A--=20=0A2.51.0=0A=0A=

--Apple-Mail=_40CFB0D5-5304-4E38-8DBC-298E9786F080
Content-Transfer-Encoding: 7bit
Content-Type: text/html;
	charset=us-ascii

<html aria-label="message body"><head><meta http-equiv="content-type" content="text/html; charset=us-ascii"></head><body style="overflow-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;"><div><blockquote type="cite"><div><div></div></div></blockquote></div><br></body></html>
--Apple-Mail=_40CFB0D5-5304-4E38-8DBC-298E9786F080--

--Apple-Mail=_2DE1DA9E-03B4-4465-9A4F-35CA96FC234D--