Greetings of the day everyone, Thanks for the detailed review Mr. Daniel and also for the additional reproducer and suggestions.
I'm a bit busy today, so I'll go through the attached patch and reproducer carefully tomorrow and work through the review comments. I'll get back to you once I've had a chance to look through everything. Regards, Osama Abdul Qader On Sat, Sep 5, 2026 at 12:59 AM 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. > > > 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? > > > 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? > > > 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. > + */ > > > + 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? > > > +truncate_custom_compressed_data > This function needs comments to explain why it's necessary and what it's > doing. > > > + $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. > > +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. > > -- > Daniel Gustafsson > >
