Re: Fix detection of truncated zstd-compressed backups

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(dot)evan(dot)chao(at)gmail(dot)com> wrote:

>
>
> > On Aug 11, 2026, at 00:28, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> >
> >> On 10 Aug 2026, at 10:11, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
> >>> On Aug 10, 2026, at 14:45, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)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/
>
>
>
>
>