Re: checking return value from unlink in write_relcache_init_file

From: Justin Pryzby <pryzby(at)telsasoft(dot)com>
To: Zhihong Yu <zyu(at)yugabyte(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: checking return value from unlink in write_relcache_init_file
Date: 2021-06-03 22:54:14
Message-ID: 20210603225413.GM14099@telsasoft.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2021-06-03 22:56:55 Re: checking return value from unlink in write_relcache_init_file
Previous Message Zhihong Yu 2021-06-03 22:44:13 checking return value from unlink in write_relcache_init_file