Обсуждение: checking return value from unlink in write_relcache_init_file
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.
Thanks
Вложения
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