Re: No error checking when reading from file using zstd in pg_dump
Daniel Gustafsson <daniel@yesql.se>
From: Daniel Gustafsson <daniel@yesql.se>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Tomas Vondra <tomas@vondra.me>,
Evgeniy Gorbanev <gorbanyoves@basealt.ru>,
pgsql-hackers@lists.postgresql.org
Date: 2025-06-26T12:19:01Z
Lists: pgsql-hackers
Attachments
- v6-0001-Initial-patch-by-Tom-Lane.patch (application/octet-stream) patch v6-0001
- v6-0002-pg_dump-compression-API-write_func.patch (application/octet-stream) patch v6-0002
- v6-0003-pg_dump-compression-API-open_func.patch (application/octet-stream) patch v6-0003
- v6-0004-pg_dump-compression-API-close_func.patch (application/octet-stream) patch v6-0004
- v6-0005-pg_dump-compression-API-LZ4Stream_init.patch (application/octet-stream) patch v6-0005
- v6-0006-pg_dump-compression-API-read_func-gets_func.patch (application/octet-stream) patch v6-0006
> On 25 Jun 2025, at 17:58, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I looked over this patchset briefly, and found a couple of nits: Thanks for looking! > 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. Instead of this I changed the write_func signature to return the number of bytes written as size_t. The API is loosely modelled around the libc stream API so going to void seemed less justifiable than size_t, even if the actual returnvalue is fairly useless due to erroring out via pg_fatal. > 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. Doh, of course, fixed in the attached. > 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. I went over all the fwrite callsites *in this patchset* and made sure they follow the pattern. There are a number of more fwrite calls in pg_dump (and elsewhere) which might need the same treatment but I left those for a separate patch to keep this focused on the compression API. -- Daniel Gustafsson
Commits
-
pg_dump: Fix compression API errorhandling
- ec017a305bd4 16.11 landed
- e686010c5b47 19 (unreleased) landed
- 92268b35d04c 17.7 landed
- 8980c724b559 18.0 landed