> 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
