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