Обсуждение: 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

Zhihong Yu <zyu@yugabyte.com> writes:
> Please comment on the proposed patch.

If the unlink fails, there's only really a problem if the subsequent
open() fails to overwrite the file --- and that stanza is perfectly
capable of complaining for itself.  So I think the code is fine and
there's no need for a separate message about the unlink.  Refusing to
proceed, as you've done here, is strictly worse than what we have.

            regards, tom lane

On 2021-Jun-03, Tom Lane wrote:

> If the unlink fails, there's only really a problem if the subsequent
> open() fails to overwrite the file --- and that stanza is perfectly
> capable of complaining for itself.  So I think the code is fine and
> there's no need for a separate message about the unlink.  Refusing to
> proceed, as you've done here, is strictly worse than what we have.

It does seem to deserve a comment explaining this.

-- 
Álvaro Herrera       Valdivia, Chile
"Pensar que el espectro que vemos es ilusorio no lo despoja de espanto,
sólo le suma el nuevo terror de la locura" (Perelandra, C.S. Lewis)

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2021-Jun-03, Tom Lane wrote:
>> If the unlink fails, there's only really a problem if the subsequent
>> open() fails to overwrite the file --- and that stanza is perfectly
>> capable of complaining for itself.  So I think the code is fine and
>> there's no need for a separate message about the unlink.  Refusing to
>> proceed, as you've done here, is strictly worse than what we have.

> It does seem to deserve a comment explaining this.

Agreed, the existing comment there is a tad terse.

            regards, tom lane