Hi Chao, I went through the LZ4 part of the v9 patch and tested the frame-completion handling in the pg_restore read path.
First, I checked how frame_finished is maintained in compress_lz4.c. It is updated after every LZ4F_decompress() call using the returned status: state->frame_finished = (status == 0); I then looked at the LZ4 frame semantics and tested concatenated frames with the LZ4 command-line tool: - two complete frames: F1 + F2 - complete first frame followed by a truncated second frame: F1 + truncated F2 The complete two-frame stream was accepted, while the stream with the truncated second frame was reported as an unfinished stream. I then traced the corresponding v9 pg_restore path. In particular, _PrintFileData() repeatedly calls CFH->read_func() until it reaches EOF or an error, and the LZ4 read path continues processing buffered compressed data when bufnext < bufdata. Therefore, after a complete first frame sets frame_finished = true, a subsequent incomplete second frame is still passed to LZ4F_decompress(). Its positive return value changes frame_finished back to false, and physical EOF subsequently causes the read to fail with EIO. I also checked the LZ4Stream_close() path. Although it only checks frame_finished, the normal directory-format pg_restore path does not simply stop after the first completed frame; it continues consuming the available compressed data until EOF/error before closing the handle. Based on these tests and the source-level tracing, the v9 patch correctly detects the reported truncated LZ4 backup case in the normal pg_restore directory-format read path. I did not identify an issue with the LZ4 frame-completion handling in the path I examined. With Regards, Osama Abdul Qader On Mon, Sep 7, 2026 at 8:43 AM Chao Li <[email protected]> wrote: > > > > 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/ > > > > >
