Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.400.21\))
Subject: Re: Check some unchecked fclose() results
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: <53d0ef41-e11e-4d2a-9aaa-48e9f0cf02f9@proxel.se>
Date: Thu, 2 Apr 2026 12:15:48 +0800
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <7E8BB5B2-CECC-4D1E-AC41-5D5142629271@gmail.com>
References: <191398CF-C65F-4751-B704-7C904BCBB80B@gmail.com>
 <BFE80E46-3730-4178-B305-0AA1E6A831A9@gmail.com>
 <53d0ef41-e11e-4d2a-9aaa-48e9f0cf02f9@proxel.se>
To: Andreas Karlsson <andreas@proxel.se>
Archived-At: 
 <https://www.postgresql.org/message-id/7E8BB5B2-CECC-4D1E-AC41-5D5142629271%40gmail.com>
Precedence: bulk


> On Apr 2, 2026, at 09:16, Andreas Karlsson <andreas@proxel.se> wrote:
>=20
> On 3/23/26 3:22 AM, Chao Li wrote:
>>> My criteria for including cases in this patch were basically:
>>>=20
>>> * only output file descriptors
>>> * code paths where the logic is relatively clear and easy to handle
>=20
> 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:
>=20

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().
>=20

That=E2=80=99s a good point. I think you referred to the change in =
pg_dumpall.c and postmaster.c. I didn=E2=80=99t consider that point when =
I wrote the patch. But I think it=E2=80=99s 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=E2=80=99t 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.
>=20

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.
>=20

I might have missed some points while working on the patch, but I=E2=80=99=
m not sure how you concluded that I didn=E2=80=99t spend enough time on =
it. I did make some =E2=80=9Ctoo quick=E2=80=9D 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/