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

On Thu, Jun 3, 2021 at 6:16 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

Hi,
Here is the patch with a bit more comment on the unlink() call.

Cheers