On 6/16/25 17:41, Daniel Gustafsson wrote: >> 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.. >
Yes. It's been a while since this commit, but I recall we started with a gzip-only implementation. 16 introduced this API, but it's definitely the case it was based on the initial gzip code. Regarding the Z_NULL, I believe it has always been ignored like this, at least since 9.1. The code simply returns what gzgets() returns, and then compares that to NULL, etc. Is there there's a better way to deal with Z_NULL? I suppose we could explicitly check/translate Z_NULL to NULL, although Z_NULL is simply defined as 0. I don't recall if NULL has some additional magic. regards -- Tomas Vondra
