Re: Fix detection of truncated zstd-compressed backups

From: Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>
To: Daniel Gustafsson <daniel(at)yesql(dot)se>
Cc: Japin Li <japinli(at)hotmail(dot)com>, Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Osama Abdul Qader <osamaabdulqader(dot)cs(at)gmail(dot)com>
Subject: Re: Fix detection of truncated zstd-compressed backups
Date: 2026-10-08 23:28:51
Message-ID: AAF3BDDB-B9D1-4348-88AC-E0F5F864AF4F@gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On Oct 9, 2026, at 06:36, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>
>> On 7 Sep 2026, at 05:13, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>
>> PFA v9.
>
> Sorry for returning to this late, things have been busy.

No worries at all.

> I think v9 is pretty
> much ready to go in, but I have a few small comments/questions.
>
> - pg_fatal("could not uncompress data: %s", zp->msg);
> + pg_fatal("could not uncompress data: %s",
> + zp->msg ? zp->msg : "unknown error");
>
> Is this change a precaution or fixing a known issue which can happen?

This change was to address one of your previous comments:
```
>> 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.
```

> /* Lazy init */
> if (!LZ4Stream_init(state, false /* decompressing */ ))
> - {
> - pg_log_error("unable to initialize LZ4 library: %s",
> - LZ4F_getErrorName(state->errcode));
> return -1;
> - }
>
> I understand the rationale for this change, but I don't think it's improving
> things. As it stands now we are losing context in the messaging when all
> errors say "can't read from input file", and the fread error cannot be captured
> with LZ4Stream_get_error since fread doesn't set errno. The double log isn't
> pretty, but getting worse errormessages isn't better so we need to stick with
> it for now. A refactoring for 20 could be to pass down an error buffer and
> psprintf the message there for the caller to log. This close to v20 RC I don't
> want to redesign any part though.

Sounds fair.

>
> The above point, along with minor cleanup and polish here and there is in the
> attached v10.
>
> --
> Daniel Gustafsson
>
> <v10-0001-Fix-detection-of-truncated-compressed-backups.patch><v10-0002-Fix-detection-of-truncated-zstd-and-LZ4-dump-dat.patch>

I compared v10 with v9:

* v10-0001 has only a formatting change and an updated commit message.
* v10-0002 restores the original error messages, as discussed in your second comment. It also has some formatting changes on tests.

So, v10 looks good to me.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Manu 2026-10-08 23:36:53 Re: [PATCH] Extensible ReadyForQuery wire protocol message and C hook, for connection pools and WAIT FOR LSN
Previous Message surya poondla 2026-10-08 22:36:52 Re: pg_walinspect: add functions to locate and list WAL by time and LSN