> On Sep 5, 2026, at 03:29, Daniel Gustafsson <[email protected]> wrote: > >> On 14 Aug 2026, at 06:28, Chao Li <[email protected]> wrote: > > Sorry for the long delay in responding. > >> 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. > > Thanks for expanding the fix, I ran out of time after realizing there was an > issue through my hack. > >> 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. > > This reasoning should be added as a comment in the test file to aid future > readers.
Added a comment in 0002.
>
>> pg_restore: error: could not uncompress data: (null)
>
> While not strictly related we should take the opportunity to fix this while in
> here. Perhaps use "unkown error" in case zp->msg is null when erroring out?
Fixed by replacing (null) with “unknown error” in 0002.
>
>> 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.
>
> Makes sense. The gets function does however not exit with pg_fatal, do we
> need
> any special handling there?
Good catch. Fixed in 0002 by moving the error logging from
LZ4Stream_read_internal() to LZ4Stream_gets().
>
>> See 0002 for the fix. I added tests only for zstd and lz4, since gzip is not
>> changed.
>
> A few comments on the patches:
>
> +typedef enum
> +{
> + ASTREAMER_STREAM_NEW,
> + ASTREAMER_FRAME_INCOMPLETE,
> + ASTREAMER_FRAME_COMPLETE,
> +} astreamer_decompression_state;
> This needs better commenting. Perhaps something along the lines of:
>
> -/* State of the most recently processed compressed frame. */
> +/*
> + * State of the most recently processed compressed frame. When decompression
> + * requires more input data to complete, or a bigger output buffer to store
> + * the result the state is set to ASTREAMER_FRAME_INCOMPLETE. Exactly how to
> + * resolve an _INCOMPLETE state is compression library dependent. Before a
> + * stream has decompressed any frames is has the state ASTREAMER_STREAM_NEW.
> + */
>
Integrated the suggested comment in 0001.
>
> + bool frame_finished;
> Nitpick: I'm not a fan of using a local variable with the same name (and
> function) as a struct member. Maybe also a comment explaining why we're not
> pulling out an LZ4State from the private member?
>
Renamed the local variable to dec_done that has the naming style as the other
local variable dec_opts. Also added a comment.
>
> +truncate_custom_compressed_data
> This function needs comments to explain why it's necessary and what it's
> doing.
>
Added a comment.
>
> + $pos = index($data, $magic);
> + die "compressed frame magic not found in $path" if $pos < 5;
> + $pos -= 5;
> If the function takes the magic as a parameter it should not make any
> assumptions about the length of the magic. This should either be inferred
> from
> the parameter (best option IMHO) or passed in separately.
Replaced the magic 4 and 5 to local variables and added comments to explain
them.
>
> +SKIP:
> +{
> + skip "zstd compression not supported by this build", 1 if !$supports_zstd;
> Please add a comment explaining why this testcase isn't applicable to LZ4.
>
Added a comment.
PFA v9.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
v9-0001-Fix-detection-of-truncated-compressed-backups.patch
Description: Binary data
v9-0002-Fix-detection-of-truncated-zstd-and-LZ4-dump-data.patch
Description: Binary data
