Re: checking return value from unlink in write_relcache_init_file

On Thu, Jun 03, 2021 at 03:44:13PM -0700, Zhihong Yu wrote:
> Hi,
> I was looking at write_relcache_init_file() where an attempt is made to
> unlink the tempfilename.
>
> However, the return value is not checked.
> If the tempfilename is not removed (the file exists), I think we should log
> a warning and proceed.
>
> Please comment on the proposed patch.

- unlink(tempfilename); /* in case it exists w/wrong permissions */
+ /* in case it exists w/wrong permissions */
+ if (unlink(tempfilename) && errno != ENOENT)
+ {
+ ereport(WARNING,
+ (errcode_for_file_access(),
+ errmsg("could not unlink relation-cache initialization file \"%s\": %m",
+ tempfilename),
+ errdetail("Continuing anyway, but there's something wrong.")));
+ return;
+ }
+

fp = AllocateFile(tempfilename, PG_BINARY_W);

The comment here is instructive: the unlink is in advance of AllocateFile(),
and if the file exists with wrong permissions, then AllocateFile would itself fail,
and then issue a warning:

errmsg("could not create relation-cache initialization file \"%s\": %m",
tempfilename),
errdetail("Continuing anyway, but there's something wrong.")));

In that context, I don't think it's needed to check the return of unlink().

--
Justin