> On 16 Jun 2025, at 16:20, Tom Lane <t...@sss.pgh.pa.us> wrote: > > Daniel Gustafsson <dan...@yesql.se> writes: >>> On 16 Jun 2025, at 15:56, Tom Lane <t...@sss.pgh.pa.us> wrote: >>> I've not checked to see what the other users of this API do, but >>> if they're all like this then we need to fix that comment. > >> AFAICT all other callers of this API are throwing an error with pg_fatal, and >> so does the function in question for ZStd decompression errors. > > I think I agree that we need to handle the ferror() case inside the > read_func for consistency. But there is another problem, which is > that neither of its callers are paying attention to the API: as > defined, the read_func can never return anything but "true", > which is how Zstd_read behaves. But both the callers in > compress_zstd.c seem to think they should test its result to > detect EOF.
Attached is a stab at fixing both the error handling in read_func as well as the callers misuse of the API. > Apparently, our tests do not cover the case of an > unexpected EOF. I admittedly ran out of steam when looking at adding something like this to our pg_dump tests. > This API is actually quite bizarrely defined, if you ask me. > Returning the byte count is optional, but if you don't pay > attention to the byte count you cannot know if you got any > data. At best, that's bug-encouraging. Agreed. Given that many of implementations in the gzip support code directly return the gzXXX function I suspect it was modeled around GZip but thats an unresearched guess. The fact that this returns Z_NULL where the API defines NULL is unlikely to ever be an issue, but it also doesn't seem entirely neat.. -- Daniel Gustafsson
0001-pg_dump-Handle-errors-in-reading-ZStd-streams.patch
Description: Binary data
