> On Apr 2, 2026, at 09:16, Andreas Karlsson <[email protected]> wrote:
>
> On 3/23/26 3:22 AM, Chao Li wrote:
>>> My criteria for including cases in this patch were basically:
>>>
>>> * only output file descriptors
>>> * code paths where the logic is relatively clear and easy to handle
>
> Those criteria are not enough as can be evidenced from some of the cases
> which you patched. I do not see why you would want to error out in the
> following two cases:
>
Thanks for noticing this patch.
> 1. When writing to e.g. a log file and we do not call pg_fatal() if a write
> fails. Then it makes no sense to die on failed fclose().
>
That’s a good point. I think you referred to the change in pg_dumpall.c and
postmaster.c. I didn’t consider that point when I wrote the patch. But I think
it’s arguable. Like in pg_dumpall.c, there are a lot of fprintf, check results
of every fprintf might be redundant, checking a single fclose() might be
cleaner. Anyway, I don’t want to argue hard for that.
> 2. When creating an empty file. I could be wrong here but in that case
> fclose() cannot fail. Arguably maybe we should use open() and close() then
> instead of fopen() and fclose() but handling an error which can never happen
> does not add any value.
>
Even if creating an empty file, fclose() could still possible fail. For
example, creating an empty on a network storage, while fclose(), the network
connection might be broken, then fclose() could fail.
> Please review your patches a bit careful before submitting. You are doing
> some good work with finding bugs and reviewing patches but it is clear you do
> not spend enough time per mail making sure it is not a false positive.
>
I might have missed some points while working on the patch, but I’m not sure
how you concluded that I didn’t spend enough time on it. I did make some “too
quick” mistakes in my first few months working on the hacker work. Since then,
I have made it a rule to do self-review, build, and testing for every patch
before sending it out. Of course, I am still human, I cannot guarantee that I
will never make mistakes.
But your comments made me realize that a cleanup patch covering all fclose() is
not a good idea. Each case needs to be evaluated separately, so a broad cleanup
patch would be hard to get processed. For that reason, I am withdrawing this
patch.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
