From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Daniel Gustafsson <daniel(at)yesql(dot)se> |
Cc: | Tomas Vondra <tomas(at)vondra(dot)me>, Evgeniy Gorbanev <gorbanyoves(at)basealt(dot)ru>, pgsql-hackers(at)lists(dot)postgresql(dot)org |
Subject: | Re: No error checking when reading from file using zstd in pg_dump |
Date: | 2025-06-25 15:58:54 |
Message-ID: | 1296195.1750867134@sss.pgh.pa.us |
Views: | Whole Thread | Raw Message | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Daniel Gustafsson <daniel(at)yesql(dot)se> writes:
> I spent a little bit of time reading over all the implementations and cross
> referencing the API for conformity, and came up with the attached. The 0001
> patch is the one from upstream, and each subsequent commit is fixing one
> function for all the implementations. Before pushing it should all be squashed
> into a single commit IMHO.
Thanks for tackling this!
I looked over this patchset briefly, and found a couple of nits:
v5-0002, in compress_io.h:
+ * Returns true on success and throws error for all error conditions.
It doesn't return true anymore. Should be more like
+ * Returns nothing. Exits via pg_fatal for all error conditions.
In LZ4Stream_write: you dropped the bit about
- errno = (errno) ? errno : ENOSPC;
but I think that's still necessary: we must assume ENOSPC if fwrite
doesn't set errno. Other fwrite callers (write_none, Zstd_write) need
this too. v5-0004 has an instance too, in Zstd_close. I did not check
to see if other fwrite calls are OK, but it'd be good to verify
that they all follow the pattern of presetting errno to 0 and then
replacing that with ENOSPC.
regards, tom lane
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2025-06-25 16:28:02 | Re: Unnecessary scan from non-overlapping range predicates |
Previous Message | Álvaro Herrera | 2025-06-25 15:53:44 | Re: pg_dump misses comments on NOT NULL constraints |