Re: No error checking when reading from file using zstd in pg_dump

> On 16 Jun 2025, at 16:20, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Daniel Gustafsson <daniel@yesql.se> writes:
>>> On 16 Jun 2025, at 15:56, Tom Lane <tgl@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