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

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Daniel Gustafsson <daniel(at)yesql(dot)se>
Cc: 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-16 14:20:07
Message-ID: 520142.1750083607@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:
>> On 16 Jun 2025, at 15:56, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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. Apparently, our tests do not cover the case of an
unexpected EOF.

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.

regards, tom lane

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Aleksander Alekseev 2025-06-16 14:20:51 Re: [PATCH] Remove unused #include's in src/backend/utils/adt/*
Previous Message Daniel Gustafsson 2025-06-16 14:11:58 Re: No error checking when reading from file using zstd in pg_dump