> On Aug 11, 2026, at 00:28, Daniel Gustafsson <[email protected]> wrote: > >> On 10 Aug 2026, at 10:11, Chao Li <[email protected]> wrote: >>> On Aug 10, 2026, at 14:45, Chao Li <[email protected]> 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/
v5-0001-Fix-detection-of-truncated-compressed-backups.patch
Description: Binary data
