Fix detection of truncated zstd-compressed backups
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253344psql -h localhost -U postgresBuilt from patchset v20 (message #20), August 23, 2026 at 05:00 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253344_20 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253344_20 && git checkout t253344_20Patchset v20 (message #20) is on t253344_20
Hi,
One of our users reported this issue to me on PG18. pg_verifybackup reported success for a zstd-compressed backup, but zstdfailed to decompress it. After debugging with the user’s data, I found that the backup was truncated, but pg_verifybackup does not verify that the final zstd frame completed. I also tested current master, where the problem still exists.
I created a simple repro. First, use the following commands to create a fake backup folder and truncate one byte from the compressed tar file. The final zstd command will fail:
```
workdir=$(mktemp -d /tmp/zstd-trunc.XXXXXX)
mkdir "$workdir/truncated"
dd if=/dev/zero of="$workdir/base.tar" bs=1024 count=2
zstd -q -f "$workdir/base.tar" -o "$workdir/base.tar.zst"
size=$(stat -f %z "$workdir/base.tar.zst")
dd if="$workdir/base.tar.zst" of="$workdir/truncated/base.tar.zst" bs=1 count=$((size - 1))
manifest_prefix=$'{"PostgreSQL-Backup-Manifest-Version": 1,\n "Files": [],\n "WAL-Ranges": [],\n'
printf '%s' "$manifest_prefix" > "$workdir/manifest-prefix"
manifest_checksum=$(shasum -a 256 "$workdir/manifest-prefix" | awk '{print $1}')
printf '%s"Manifest-Checksum": "%s"}\n' "$manifest_prefix" "$manifest_checksum" > $workdir/truncated/backup_manifest
zstd -t $workdir/truncated/base.tar.zst
```
However, pg_verifybackup doesn't report an error for the truncated backup:
```
% pg_verifybackup -n -F t -s /tmp/zstd-trunc.YHQ9P4/truncated
backup successfully verified
```
Looking at this code in astreamer_zstd_decompressor_content()
```
ret = ZSTD_decompressStream(mystreamer->dctx,
&mystreamer->zstd_outBuf, &inBuf);
if (ZSTD_isError(ret))
pg_fatal("could not decompress data: %s",
ZSTD_getErrorName(ret));
```
While decompressing the truncated tar file, ZSTD_decompressStream() returns 1. The doc for ZSTD_decompressStream() at [1]https://github.com/facebook/zstd/blob/v1.5.7/lib/zstd.h#L887-L905 says that a return > 0 means that there is still decoding or flushing to do before the current frame is complete. In this case, it may indicate an incomplete frame.
So, this patch records the return value in astreamer_zstd_frame and checks it in astreamer_zstd_decompressor_finalize(). See the attached patch for details.
I also added a loop to call ZSTD_decompressStream() again in astreamer_zstd_decompressor_finalize(), because the doc [1]https://github.com/facebook/zstd/blob/v1.5.7/lib/zstd.h#L887-L905 says that, when output.pos == output.size and the return value is greater than 0, the caller must call ZSTD_decompressStream() again to flush the remaining buffered output.
With the fix, now pg_verifybackup reports a failure:
```
% pg_verifybackup -n -F t -s /tmp/zstd-trunc.YHQ9P4/truncated
pg_verifybackup: error: could not decompress data: compressed stream is incomplete
```
BTW, I would also like to bump my previous patch [2]/messages/by-id/3B062561-C39C-4367-AB1E-F4C27BC6F736@gmail.com. After changing fe_utils/astreamer_zstd.c, pg_verifybackup is not rebuilt automatically, which is inconvenient. I felt the pain again while working on this patch.
[1]: https://github.com/facebook/zstd/blob/v1.5.7/lib/zstd.h#L887-L905
[2]: /messages/by-id/3B062561-C39C-4367-AB1E-F4C27BC6F736@gmail.com
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Hello!
Isn't this a generic problem, also present in gzip/lz4 compressed backups?
On Aug 8, 2026, at 03:42, Zsolt Parragi <zsolt.parragi@percona.com> wrote:
Hello!
Isn't this a generic problem, also present in gzip/lz4 compressed backups?
Maybe, but I haven’t checked them yet. I will do that next week.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On Aug 8, 2026, at 08:00, Chao Li <li.evan.chao@gmail.com> wrote:
On Aug 8, 2026, at 03:42, Zsolt Parragi <zsolt.parragi@percona.com> wrote:
Hello!
Isn't this a generic problem, also present in gzip/lz4 compressed backups?
Maybe, but I haven’t checked them yet. I will do that next week.
I just verified gzip has the same problem using a similar script:
```
workdir=$(mktemp -d /tmp/gzip-trunc.XXXXXX)
mkdir "$workdir/truncated"
dd if=/dev/zero of="$workdir/base.tar" bs=1024 count=2
gzip -c "$workdir/base.tar" > "$workdir/base.tar.gz"
size=$(stat -f %z "$workdir/base.tar.gz")
dd if="$workdir/base.tar.gz" of="$workdir/truncated/base.tar.gz" bs=1 count=$((size - 1))
manifest_prefix=$'{"PostgreSQL-Backup-Manifest-Version": 1,\n "Files": [],\n "WAL-Ranges": [],\n'
printf '%s' "$manifest_prefix" > "$workdir/manifest-prefix"
manifest_checksum=$(shasum -a 256 "$workdir/manifest-prefix" | awk '{print $1}')
printf '%s"Manifest-Checksum": "%s"}\n' "$manifest_prefix" "$manifest_checksum" > "$workdir/truncated/backup_manifest"
gzip -t "$workdir/truncated/base.tar.gz"
pg_verifybackup -F t -s "$workdir/truncated"
```
gzip fails, while pg_verifybackup succeeds.
Looking at the code, astreamer_gzip_decompressor_content() stops processing when it has consumed the input buffer. However, the inflate() doc, see [2] and [3], says that only Z_STREAM_END indicates that decompression has completed. My debugging also shows that, when reading the truncated gzip file, inflate() returns Z_OK, while it returns Z_STREAM_END for a valid gzip file. Therefore, this patch records whether inflate() has returned Z_STREAM_END and checks that state in astreamer_gzip_decompressor_finalize().
Unlike zstd, we don't need to call inflate() again in astreamer_gzip_decompressor_finalize(). Once all input has been consumed without Z_STREAM_END, calling inflate() with no input cannot complete the stream.
See attached 0002 for the fix of gzip streamer. I will check the lz4 streamer next.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
t253344_4v2-0001-Fix-detection-of-truncated-zstd-compressed-backup.patchapplication/octet-stream; name=v2-0001-Fix-detection-of-truncated-zstd-compressed-backup.patch; x-unix-mode=0644Download+38-1
v2-0002-Fix-detection-of-truncated-gzip-compressed-backup.patchapplication/octet-stream; name=v2-0002-Fix-detection-of-truncated-gzip-compressed-backup.patch; x-unix-mode=0644Download+6-1
On Aug 10, 2026, at 14:45, Chao Li <li.evan.chao@gmail.com> wrote:
On Aug 8, 2026, at 08:00, Chao Li <li.evan.chao@gmail.com> wrote:
On Aug 8, 2026, at 03:42, Zsolt Parragi <zsolt.parragi@percona.com> wrote:
Hello!
Isn't this a generic problem, also present in gzip/lz4 compressed backups?
Maybe, but I haven’t checked them yet. I will do that next week.
I just verified gzip has the same problem using a similar script:
```
workdir=$(mktemp -d /tmp/gzip-trunc.XXXXXX)
mkdir "$workdir/truncated"
dd if=/dev/zero of="$workdir/base.tar" bs=1024 count=2
gzip -c "$workdir/base.tar" > "$workdir/base.tar.gz"
size=$(stat -f %z "$workdir/base.tar.gz")
dd if="$workdir/base.tar.gz" of="$workdir/truncated/base.tar.gz" bs=1 count=$((size - 1))manifest_prefix=$'{"PostgreSQL-Backup-Manifest-Version": 1,\n "Files": [],\n "WAL-Ranges": [],\n'
printf '%s' "$manifest_prefix" > "$workdir/manifest-prefix"
manifest_checksum=$(shasum -a 256 "$workdir/manifest-prefix" | awk '{print $1}')
printf '%s"Manifest-Checksum": "%s"}\n' "$manifest_prefix" "$manifest_checksum" > "$workdir/truncated/backup_manifest"gzip -t "$workdir/truncated/base.tar.gz"
pg_verifybackup -F t -s "$workdir/truncated"
```gzip fails, while pg_verifybackup succeeds.
Looking at the code, astreamer_gzip_decompressor_content() stops processing when it has consumed the input buffer. However, the inflate() doc, see [2] and [3], says that only Z_STREAM_END indicates that decompression has completed. My debugging also shows that, when reading the truncated gzip file, inflate() returns Z_OK, while it returns Z_STREAM_END for a valid gzip file. Therefore, this patch records whether inflate() has returned Z_STREAM_END and checks that state in astreamer_gzip_decompressor_finalize().
Unlike zstd, we don't need to call inflate() again in astreamer_gzip_decompressor_finalize(). Once all input has been consumed without Z_STREAM_END, calling inflate() with no input cannot complete the stream.
See attached 0002 for the fix of gzip streamer. I will check the lz4 streamer next.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/<v2-0001-Fix-detection-of-truncated-zstd-compressed-backup.patch><v2-0002-Fix-detection-of-truncated-gzip-compressed-backup.patch>
Sorry, forgot to add the reference links:
[2]: https://zlib.net/zlib_how.html
[3]: https://zlib.net/manual.html
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On Aug 10, 2026, at 14:45, Chao Li <li.evan.chao@gmail.com> wrote:
See attached 0002 for the fix of gzip streamer. I will check the lz4 streamer next.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/<v2-0001-Fix-detection-of-truncated-zstd-compressed-backup.patch><v2-0002-Fix-detection-of-truncated-gzip-compressed-backup.patch>
Confirmed that lz4 also has the same problem. See the similar repro script:
```
workdir=$(mktemp -d /tmp/lz4-trunc.XXXXXX)
mkdir "$workdir/truncated"
dd if=/dev/zero of="$workdir/base.tar" bs=1024 count=2
lz4 -q -f "$workdir/base.tar" "$workdir/base.tar.lz4"
size=$(stat -f %z "$workdir/base.tar.lz4")
dd if="$workdir/base.tar.lz4" of="$workdir/truncated/base.tar.lz4" bs=1 count=$((size - 1))
manifest_prefix=$'{"PostgreSQL-Backup-Manifest-Version": 1,\n "Files": [],\n "WAL-Ranges": [],\n'
printf '%s' "$manifest_prefix" > "$workdir/manifest-prefix"
manifest_checksum=$(shasum -a 256 "$workdir/manifest-prefix" | awk '{print $1}')
printf '%s"Manifest-Checksum": "%s"}\n' "$manifest_prefix" "$manifest_checksum" > "$workdir/truncated/backup_manifest"
lz4 -t "$workdir/truncated/base.tar.lz4"
pg_verifybackup -F t -s "$workdir/truncated"
```
lz4 fails to decompress the truncated tar file, but pg_verifybackup succeeds.
The doc for LZ4F_decompress() [4]https://github.com/lz4/lz4/blob/dev/lib/lz4frame.h#L470-L500 says that a return value >0 is a hint about how many source bytes are needed next, 0 means that the frame is complete, and an error return is identified with LZ4F_isError().
So, as in 0001, we can record the return value of LZ4F_decompress() in astreamer_lz4_frame and check it in astreamer_lz4_decompressor_finalize(). Unlike zstd, we don't need to call LZ4F_decompress() again because it has no documented case where a positive return value with a full output buffer requires an empty-input call to flush internally buffered output.
[4]: https://github.com/lz4/lz4/blob/dev/lib/lz4frame.h#L470-L500
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
t253344_6v3-0001-Fix-detection-of-truncated-zstd-compressed-backup.patchapplication/octet-stream; name=v3-0001-Fix-detection-of-truncated-zstd-compressed-backup.patch; x-unix-mode=0644Download+38-1
v3-0002-Fix-detection-of-truncated-gzip-compressed-backup.patchapplication/octet-stream; name=v3-0002-Fix-detection-of-truncated-gzip-compressed-backup.patch; x-unix-mode=0644Download+6-1
v3-0003-Fix-detection-of-truncated-LZ4-compressed-backups.patchapplication/octet-stream; name=v3-0003-Fix-detection-of-truncated-LZ4-compressed-backups.patch; x-unix-mode=0644Download+9-1
On 10 Aug 2026, at 10:11, Chao Li <li.evan.chao@gmail.com> wrote:
On Aug 10, 2026, at 14:45, Chao Li <li.evan.chao@gmail.com> wrote:
See attached 0002 for the fix of gzip streamer. I will check the lz4 streamer next.
Confirmed that lz4 also has the same problem. See the similar repro script:
Thanks for this patchset, I think this is something we should fix. I took the
liberty to squash the patchset into a single patch to start preparing it for
the final shape, as well as adding a testcase to verify this.
+ /* Reject empty input, which does not contain a complete zstd frame. */
+ streamer->decompression_ret = 1;I am not a huge fan of this, we claim that we save the return value but then we
assign a value which hasn't yet been returned as a sentinel. Given that the
return is a size_t we also can't really invent a sentinel. Since we don't
actually use the returned value for anything but "done or not-done", so I
propose something like the attached which interprets the value and stores a
named state. What are your thoughts on this?
--
Daniel Gustafsson
On Aug 11, 2026, at 00:28, Daniel Gustafsson <daniel@yesql.se> wrote:
On 10 Aug 2026, at 10:11, Chao Li <li.evan.chao@gmail.com> wrote:
On Aug 10, 2026, at 14:45, Chao Li <li.evan.chao@gmail.com> wrote:
See attached 0002 for the fix of gzip streamer. I will check the lz4 streamer next.
Confirmed that lz4 also has the same problem. See the similar repro script:
Thanks for this patchset, I think this is something we should fix. I took the
liberty to squash the patchset into a single patch to start preparing it for
the final shape, as well as adding a testcase to verify this.
Thank for taking care of this patch.
+ /* Reject empty input, which does not contain a complete zstd frame. */ + streamer->decompression_ret = 1;I am not a huge fan of this, we claim that we save the return value but then we
assign a value which hasn't yet been returned as a sentinel. Given that the
return is a size_t we also can't really invent a sentinel. Since we don't
actually use the returned value for anything but "done or not-done", so I
propose something like the attached which interprets the value and stores a
named state. What are your thoughts on this?
I agree with the direction. The new proposal can distinguish an empty stream from a truncated stream.
<v4-0001-Fix-detection-of-truncated-compressed-backups.patch>
A few comments with v4:
1 - ztsd
```
+ if (mystreamer->state != STREAM_FINISHED)
+ pg_fatal("could not decompress data: compressed stream is incomplete");
+ else if (unlikely(mystreamer->state == STREAM_NEW))
+ pg_fatal("could not decompress data: compressed stream is empty");
```This is a small logic error. As STREAM_NEW!=STREAM_FINISHED already, the “else if” is unreachable. We should check if (unlikely(mystreamer->state == STREAM_NEW) first.
2 - ztsd
```
+ /* The stream is only done when ZSTD_decompressStream returns 0 */
+ if (ret)
+ mystreamer->state = STREAM_HAS_DATA;
+ else
+ mystreamer->state = STREAM_FINISHED;
```When ret == 0, that only means the current frame is complete, so the comment “the stream is only done” sounds too strong, I would change to “The frame is only done when …”.
3 - ztsd and lz4
```
+typedef enum
+{
+ STREAM_NEW,
+ STREAM_HAS_DATA,
+ STREAM_FINISHED,
+} pg_stream_state;
```Similar to comment 2, return == 0 means the current is complete and >0 means the current frame is incomplete, thus I would rename STREAM_HAS_DATA to FRAME_HAS_DATA, and STREAM_FINISHED to FRAME_FINISHED.
I addressed all the 3 comments in v5.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Hi Chao,
I noticed your commit f80cb3ae0737 ("Fix detection of truncated
zstd-compressed backups"), which addresses the truncated ZSTD backup issue
I had reported.
I added a regression test to src/bin/pg_verifybackup/t/008_untar.pl. The
test creates a server-side ZSTD backup, verifies the intact backup,
truncates base.tar.zst by one byte, and then verifies that pg_verifybackup
rejects the truncated backup.
The test passes with your fix:
make check -C src/bin/pg_verifybackup TESTS=t/008_untar
Result: PASS.
I noticed that your commit changes astreamer_zstd.c but does not add a
regression test to 008_untar.pl. Would this test be useful to include with
the fix?
Thanks,
Osama Abdul Qader
On Tue, Aug 11, 2026 at 2:55 AM Chao Li <li.evan.chao@gmail.com> wrote:
Show quoted text
On Aug 11, 2026, at 00:28, Daniel Gustafsson <daniel@yesql.se> wrote:
On 10 Aug 2026, at 10:11, Chao Li <li.evan.chao@gmail.com> wrote:
On Aug 10, 2026, at 14:45, Chao Li <li.evan.chao@gmail.com> wrote:
See attached 0002 for the fix of gzip streamer. I will check the lz4
streamer next.
Confirmed that lz4 also has the same problem. See the similar repro
script:
Thanks for this patchset, I think this is something we should fix. I
took the
liberty to squash the patchset into a single patch to start preparing it
for
the final shape, as well as adding a testcase to verify this.
Thank for taking care of this patch.
+ /* Reject empty input, which does not contain a complete zstd frame. */ + streamer->decompression_ret = 1;I am not a huge fan of this, we claim that we save the return value but
then we
assign a value which hasn't yet been returned as a sentinel. Given that
the
return is a size_t we also can't really invent a sentinel. Since we
don't
actually use the returned value for anything but "done or not-done", so I
propose something like the attached which interprets the value andstores a
named state. What are your thoughts on this?
I agree with the direction. The new proposal can distinguish an empty
stream from a truncated stream.<v4-0001-Fix-detection-of-truncated-compressed-backups.patch>
A few comments with v4:
1 - ztsd ``` + if (mystreamer->state != STREAM_FINISHED) + pg_fatal("could not decompress data: compressed stream is incomplete"); + else if (unlikely(mystreamer->state == STREAM_NEW)) + pg_fatal("could not decompress data: compressed stream is empty"); ```This is a small logic error. As STREAM_NEW!=STREAM_FINISHED already, the
“else if” is unreachable. We should check if (unlikely(mystreamer->state ==
STREAM_NEW) first.2 - ztsd ``` + /* The stream is only done when ZSTD_decompressStream returns 0 */ + if (ret) + mystreamer->state = STREAM_HAS_DATA; + else + mystreamer->state = STREAM_FINISHED; ```When ret == 0, that only means the current frame is complete, so the
comment “the stream is only done” sounds too strong, I would change to “The
frame is only done when …”.3 - ztsd and lz4 ``` +typedef enum +{ + STREAM_NEW, + STREAM_HAS_DATA, + STREAM_FINISHED, +} pg_stream_state; ```Similar to comment 2, return == 0 means the current is complete and >0
means the current frame is incomplete, thus I would rename STREAM_HAS_DATA
to FRAME_HAS_DATA, and STREAM_FINISHED to FRAME_FINISHED.I addressed all the 3 comments in v5.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On Aug 11, 2026, at 15:40, Osama Abdul Qader <osamaabdulqader.cs@gmail.com> wrote:
Hi Chao,
I noticed your commit f80cb3ae0737 ("Fix detection of truncated zstd-compressed backups"), which addresses the truncated ZSTD backup issue I had reported.
I added a regression test to src/bin/pg_verifybackup/t/008_untar.pl. The test creates a server-side ZSTD backup, verifies the intact backup, truncates base.tar.zst by one byte, and then verifies that pg_verifybackup rejects the truncated backup.
The test passes with your fix:
make check -C src/bin/pg_verifybackup TESTS=t/008_untarResult: PASS.
I noticed that your commit changes astreamer_zstd.c but does not add a regression test to 008_untar.pl. Would this test be useful to include with the fix?
Thanks,
Osama Abdul Qader
Hi Osama,
Thanks for your review and for adding the test. I think the test Daniel added to 010_client_untar.pl covers all three astreamer types.
Your test covers server-side compression, but from the astreamer’s perspective, I don't think there is much difference between client side and server side compression. Therefore, I am not sure that an additional server side test is needed. Let’s see what Daniel thinks.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Hi Chao,
Thanks for the feedback, That's make sense to me, I'll wait for Mr.
Daniel's thought on whether the existing 010_client_untar.pl coverage is
sufficient.
Thanks,
Osama Abdul Qader
On Tue, Aug 11, 2026 at 1:28 PM Chao Li <li.evan.chao@gmail.com> wrote:
Show quoted text
On Aug 11, 2026, at 15:40, Osama Abdul Qader <
osamaabdulqader.cs@gmail.com> wrote:
Hi Chao,
I noticed your commit f80cb3ae0737 ("Fix detection of truncated
zstd-compressed backups"), which addresses the truncated ZSTD backup issue
I had reported.I added a regression test to src/bin/pg_verifybackup/t/008_untar.pl.
The test creates a server-side ZSTD backup, verifies the intact backup,
truncates base.tar.zst by one byte, and then verifies that pg_verifybackup
rejects the truncated backup.The test passes with your fix:
make check -C src/bin/pg_verifybackup TESTS=t/008_untarResult: PASS.
I noticed that your commit changes astreamer_zstd.c but does not add a
regression test to 008_untar.pl. Would this test be useful to include
with the fix?Thanks,
Osama Abdul QaderHi Osama,
Thanks for your review and for adding the test. I think the test Daniel
added to 010_client_untar.pl covers all three astreamer types.Your test covers server-side compression, but from the astreamer’s
perspective, I don't think there is much difference between client side and
server side compression. Therefore, I am not sure that an additional server
side test is needed. Let’s see what Daniel thinks.Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On Aug 11, 2026, at 16:32, Osama Abdul Qader <osamaabdulqader.cs@gmail.com> wrote:
Hi Chao,
Thanks for the feedback, That's make sense to me, I'll wait for Mr. Daniel's thought on whether the existing 010_client_untar.pl coverage is sufficient.
Thanks,
Osama Abdul QaderOn Tue, Aug 11, 2026 at 1:28 PM Chao Li <li.evan.chao@gmail.com> wrote:
On Aug 11, 2026, at 15:40, Osama Abdul Qader <osamaabdulqader.cs@gmail.com> wrote:
Hi Chao,
I noticed your commit f80cb3ae0737 ("Fix detection of truncated zstd-compressed backups"), which addresses the truncated ZSTD backup issue I had reported.
I added a regression test to src/bin/pg_verifybackup/t/008_untar.pl. The test creates a server-side ZSTD backup, verifies the intact backup, truncates base.tar.zst by one byte, and then verifies that pg_verifybackup rejects the truncated backup.
The test passes with your fix:
make check -C src/bin/pg_verifybackup TESTS=t/008_untarResult: PASS.
I noticed that your commit changes astreamer_zstd.c but does not add a regression test to 008_untar.pl. Would this test be useful to include with the fix?
Thanks,
Osama Abdul QaderHi Osama,
Thanks for your review and for adding the test. I think the test Daniel added to 010_client_untar.pl covers all three astreamer types.
Your test covers server-side compression, but from the astreamer’s perspective, I don't think there is much difference between client side and server side compression. Therefore, I am not sure that an additional server side test is needed. Let’s see what Daniel thinks.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Hi, Osama,
FYI, when you want to attach a patch file, please include “nocfbot” in the file name. Otherwise, CommitFest will pick up your patch file and incorrectly ask for a rebase. See [1]https://wiki.postgresql.org/wiki/Cfbot#When_are_patches_tested?.
Resending v5 to recover the CF entry.
[1]: https://wiki.postgresql.org/wiki/Cfbot#When_are_patches_tested?
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
t253344_12v5-0001-Fix-detection-of-truncated-compressed-backups.patchapplication/octet-stream; name=v5-0001-Fix-detection-of-truncated-compressed-backups.patch; x-unix-mode=0644Download+94-2
I have two minor comment for v5, otherwise it looks good to me:
+typedef enum
+{
+ STREAM_NEW,
+ STREAM_HAS_DATA,
+ STREAM_FINISHED,
+} pg_stream_state;
+This is duplicated in two files, wouldn't be astreamer.h a better place for it?
+ if (!mystreamer->stream_finished)
+ pg_fatal("could not decompress data: compressed stream is incomplete");
+The other two checks distinguish empty from incomplete, is this difference intended?
On Aug 13, 2026, at 05:56, Zsolt Parragi <zsolt.parragi@percona.com> wrote:
I have two minor comment for v5, otherwise it looks good to me:
Hi Zsolt,
Thanks a lot for reviewing.
+typedef enum +{ + STREAM_NEW, + STREAM_HAS_DATA, + STREAM_FINISHED, +} pg_stream_state; +This is duplicated in two files, wouldn't be astreamer.h a better place for it?
I think this makes sense. Plus your second comment, this enum can be shared by all 3 astreamers. I moved it to astreamer.h and renamed it to astreamer_decompression_state.
+ if (!mystreamer->stream_finished) + pg_fatal("could not decompress data: compressed stream is incomplete"); +The other two checks distinguish empty from incomplete, is this
difference intended?
I do not think that is intentional. My initial implementation did not check for an empty stream, and gzip uses a different pattern from zstd and LZ4, which is probably why Daniel initially added the empty-stream check only to zstd and lz4.
I have now added the same check to gzip so that all three astreamers are consistent.
PFA v6: addressed Zsolt’s comments.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
t253344_14v6-0001-Fix-detection-of-truncated-compressed-backups.patchapplication/octet-stream; name=v6-0001-Fix-detection-of-truncated-compressed-backups.patch; x-unix-mode=0644Download+93-2
References: <5962B878-C43D-4EBC-9E95-1F945CE5E586@gmail.com>
<CAN4CZFNuV2NKXbPTGkN8chga01sVGpUcNinM-HpdyFtH0eJR0w@mail.gmail.com>
<06FBF640-1A1D-48B0-BBEA-F4EBB4017309@gmail.com>
<91EEEF17-A331-4CEA-ABEA-E63C4FBDB173@gmail.com>
<2E9971CA-062B-4EC0-8858-9979C73A3888@gmail.com>
<CF427D27-3AF6-4C0E-ABAE-9FF7DED20FCD@yesql.se>
<1381EA72-8932-4734-9F7B-8315208EDB96@gmail.com>
<CAC+8b5imyLMnif=JBYnDK9H6xLhAgSP_Yw2WtU-vTOb34iqW5w@mail.gmail.com>
<6D85CC75-CB35-45E3-ADBB-8287DF6BB429@gmail.com>
<CAC+8b5itHRfCcgSt4WKCxkcOZ5+DvmQR2mavj1gU6YhBBpvZrQ@mail.gmail.com>
<6A9A9FC1-B59E-4454-8B95-570054038EFE@gmail.com>
<CAN4CZFM_RaAxiRcf0JmhmNqUac+9-3KYGVu+Uvc43VxH7bibaw@mail.gmail.com>
<3664E105-4512-4BD2-99A7-0F5C25EBB173@gmail.com>
User-Agent: mu4e 1.14.1; emacs 30.2
Hi Chao,
Thanks for updating the patch.
Date: Thu, 13 Aug 2026 11:32:59 +0800
On Thu, 13 Aug 2026 at 09:47, Chao Li <li.evan.chao@gmail.com> wrote:
On Aug 13, 2026, at 05:56, Zsolt Parragi <zsolt.parragi@percona.com> wrote:
I have two minor comment for v5, otherwise it looks good to me:
Hi Zsolt,
Thanks a lot for reviewing.
+typedef enum +{ + STREAM_NEW, + STREAM_HAS_DATA, + STREAM_FINISHED, +} pg_stream_state; +This is duplicated in two files, wouldn't be astreamer.h a better place for it?
I think this makes sense. Plus your second comment, this enum can be shared by all 3 astreamers. I moved it to astreamer.h and renamed it to astreamer_decompression_state.
The astreamer_decompression_state should be added to pgindent's typedefs.list.
+ if (!mystreamer->stream_finished) + pg_fatal("could not decompress data: compressed stream is incomplete"); +The other two checks distinguish empty from incomplete, is this
difference intended?I do not think that is intentional. My initial implementation did not
check for an empty stream, and gzip uses a different pattern from zstd
and LZ4, which is probably why Daniel initially added the empty-stream
check only to zstd and lz4.I have now added the same check to gzip so that all three astreamers are consistent.
PFA v6: addressed Zsolt’s comments.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.
On Aug 13, 2026, at 11:39, Japin Li <japinli@hotmail.com> wrote:
Hi Chao,
Thanks for updating the patch.
Hi Jipan, thanks a lot for reviewing.
Date: Thu, 13 Aug 2026 11:32:59 +0800
On Thu, 13 Aug 2026 at 09:47, Chao Li <li.evan.chao@gmail.com> wrote:
On Aug 13, 2026, at 05:56, Zsolt Parragi <zsolt.parragi@percona.com> wrote:
I have two minor comment for v5, otherwise it looks good to me:
Hi Zsolt,
Thanks a lot for reviewing.
+typedef enum +{ + STREAM_NEW, + STREAM_HAS_DATA, + STREAM_FINISHED, +} pg_stream_state; +This is duplicated in two files, wouldn't be astreamer.h a better place for it?
I think this makes sense. Plus your second comment, this enum can be shared by all 3 astreamers. I moved it to astreamer.h and renamed it to astreamer_decompression_state.
The astreamer_decompression_state should be added to pgindent's typedefs.list.
Yes, I missed that part.
PFA v7: addressed Jipan’s comment.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
t253344_16v7-0001-Fix-detection-of-truncated-compressed-backups.patchapplication/octet-stream; name=v7-0001-Fix-detection-of-truncated-compressed-backups.patch; x-unix-mode=0644Download+94-2
Hi Chao,
Regarding the nocfbot, I get you, and sure in my future patches, I'll make
sure to use nocfbot in my patches, Thanks for letting me know about that
and I regret for the inconvenience that occurs.
With best regards
Osama Abdul Qader.
On Thu, 13 Aug, 2026, 10:42 am Chao Li, <li.evan.chao@gmail.com> wrote:
Show quoted text
On Aug 13, 2026, at 11:39, Japin Li <japinli@hotmail.com> wrote:
Hi Chao,
Thanks for updating the patch.
Hi Jipan, thanks a lot for reviewing.
Date: Thu, 13 Aug 2026 11:32:59 +0800
On Thu, 13 Aug 2026 at 09:47, Chao Li <li.evan.chao@gmail.com> wrote:
On Aug 13, 2026, at 05:56, Zsolt Parragi <zsolt.parragi@percona.com>
wrote:
I have two minor comment for v5, otherwise it looks good to me:
Hi Zsolt,
Thanks a lot for reviewing.
+typedef enum +{ + STREAM_NEW, + STREAM_HAS_DATA, + STREAM_FINISHED, +} pg_stream_state; +This is duplicated in two files, wouldn't be astreamer.h a better
place for it?
I think this makes sense. Plus your second comment, this enum can be
shared by all 3 astreamers. I moved it to astreamer.h and renamed it to
astreamer_decompression_state.The astreamer_decompression_state should be added to pgindent's
typedefs.list.
Yes, I missed that part.
PFA v7: addressed Jipan’s comment.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On 13 Aug 2026, at 07:11, Chao Li <li.evan.chao@gmail.com> wrote:
PFA v7: addressed Jipan’s comment.
Sorry for being slow on this, things are quite busy but I hope to have a review
soon. While poking at this I realized that our compression code in pg_dump
likely has the same issue. I hacked up a quick PoC diff (attached) but it's
untested (can one actually test the data-in-zstd-internal-buffers case at all?)
and mainly a sketch. If you want to pick it up and rework into this patchset
to tackle it treewide then that would be fantastic.
--
Daniel Gustafsson
Attachments:
t253344_18pg_dump.diff.txttext/plain; name=pg_dump.diff.txt; x-unix-mode=0644Download+21-0
On Aug 14, 2026, at 04:57, Daniel Gustafsson <daniel@yesql.se> wrote:
On 13 Aug 2026, at 07:11, Chao Li <li.evan.chao@gmail.com> wrote:
PFA v7: addressed Jipan’s comment.
Sorry for being slow on this, things are quite busy but I hope to have a review
soon. While poking at this I realized that our compression code in pg_dump
likely has the same issue. I hacked up a quick PoC diff (attached) but it's
untested (can one actually test the data-in-zstd-internal-buffers case at all?)
and mainly a sketch. If you want to pick it up and rework into this patchset
to tackle it treewide then that would be fantastic.--
Daniel Gustafsson<pg_dump.diff.txt>
I can work on this today.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On Aug 14, 2026, at 06:59, Chao Li <li.evan.chao@gmail.com> wrote:
On Aug 14, 2026, at 04:57, Daniel Gustafsson <daniel@yesql.se> wrote:
On 13 Aug 2026, at 07:11, Chao Li <li.evan.chao@gmail.com> wrote:
PFA v7: addressed Jipan’s comment.
Sorry for being slow on this, things are quite busy but I hope to have a review
soon. While poking at this I realized that our compression code in pg_dump
likely has the same issue. I hacked up a quick PoC diff (attached) but it's
untested (can one actually test the data-in-zstd-internal-buffers case at all?)
and mainly a sketch. If you want to pick it up and rework into this patchset
to tackle it treewide then that would be fantastic.--
Daniel Gustafsson<pg_dump.diff.txt>
I can work on this today.
I just checked pg_dump/pg_restore. The problem exists only with zstd and lz4, gzip doesn't have the problem.
Daniel’s PoC covers the custom-archive-format path, but not the directory-format path.
For the directory-format path, we can reproduce the problem by simply truncating one byte from a compressed data file. For the custom-archive path, reproducing the problem is less straightforward because the compressed data is stored inside length-prefixed archive blocks. Simply truncating the file can make archive parsing fail before the decompressor sees the truncated frame. I created a repro script, see the attached shell script.
Before the fix, the output contains:
```
custom zstd: exit status 0
directory zstd: exit status 0
custom lz4: exit status 0
directory lz4: exit status 0
pg_restore: error: could not uncompress data: (null)
custom gzip: exit status 1
pg_restore: error: could not close data file "/tmp/pgdump-trunc.1rxbf0/gzip-dir-bad/3931.dat": Undefined error: 0
directory gzip: exit status 1
```
This shows that gzip reports failure, while zstd and lz4 silently accept the truncated dump files.
After the fix, zstd and lz4 report failures as well:
```
pg_restore: error: could not decompress data: compressed stream is incomplete
custom zstd: exit status 1
pg_restore: error: could not decompress data: compressed stream is incomplete
directory zstd: exit status 1
pg_restore: error: could not decompress data: compressed stream is incomplete
custom lz4: exit status 1
pg_restore: error: could not read from input file: Input/output error
directory lz4: exit status 1
pg_restore: error: could not uncompress data: (null)
custom gzip: exit status 1
pg_restore: error: could not close data file "/tmp/pgdump-trunc.XjUiJ4/gzip-dir-bad/3931.dat": Undefined error: 0
directory gzip: exit status 1
```
While testing, I also found a small issue in LZ4Stream_read_internal(). Its error branches call pg_log_error() and then return -1, but callers immediately call pg_fatal() when the return value <0. This results in duplicate error messages. So, I removed those pg_log_error() calls.
See 0002 for the fix. I added tests only for zstd and lz4, since gzip is not changed.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/