Re: Fix detection of truncated zstd-compressed backups

> On Sep 5, 2026, at 03:29, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>
>> On 14 Aug 2026, at 06:28, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> 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/