Re: pgsql: Improve error handling in RemovePgTempFiles().
Thomas Munro writes: > Perhaps RemovePgTempFiles() could check if each one exists before > calling RemovePgTempFilesInDir(), like in the attached? Alternatively > we could make RemovePgTempFilesInDir() return early if temp_dir == > NULL (going against your commit message above), or I suppose we could > arrange for temporary directories always to exist in base and each > tablespace. After further thought I don't especially like your solution as written: it has a race condition if the directory is deleted between the pre-check and the AllocateDir proper. What seems better to me is a tighter version of your second alternative: let's add a "missing_ok" parameter to RemovePgTempFilesInDir, which'd be passed as true only at the top level, and do something like temp_dir = AllocateDir(tmpdirname); + if (temp_dir == NULL && errno == ENOENT && missing_ok) + return; + while ((temp_de = ReadDirExtended(temp_dir, tmpdirname, LOG)) != NULL) This might be problematic if RemovePgTempFilesInDir were a globally exposed API, but it isn't, so adding a parameter seems fine. Hmmm ... actually, in the recursive call case, it wouldn't be that awful to ignore ENOENT either; if a directory goes away between being stat'd and being opened, you'd still get a log message about rmdir failure at the caller level. So maybe we should just do your second alternative as-is (ie, code as above but without missing_ok). Having an explicit control parameter seems marginally clearer but I'm not sure it's buying anything meaningful. Thoughts? regards, tom lane
pgsql: Back off chattiness in RemovePgTempFiles().
Back off chattiness in RemovePgTempFiles(). In commit 561885db0, as part of normalizing RemovePgTempFiles's error handling, I removed its behavior of silently ignoring ENOENT failures during directory opens. Thomas Munro points out that this is a bad idea at the top level, because we don't create pgsql_tmp directories until needed. Thus this coding could produce LOG messages in perfectly normal situations, which isn't what I intended. Restore the suppression of ENOENT logging, but only at top level --- it would still be unexpected for a nested temp directory to disappear between seeing it in the parent directory and opening it. Discussion: https://postgr.es/m/CAEepm=2y06SehAkTnd5sU_eVqdv5P-=srt1y5vynqk6yvdv...@mail.gmail.com Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/eeb3c2df429c943b2f8d028d110b55ac0a53dc75 Modified Files -- src/backend/storage/file/fd.c | 28 1 file changed, 20 insertions(+), 8 deletions(-)
Re: pgsql: Improve error handling in RemovePgTempFiles().
I wrote: > Hmmm ... actually, in the recursive call case, it wouldn't be that > awful to ignore ENOENT either; if a directory goes away between being > stat'd and being opened, you'd still get a log message about rmdir > failure at the caller level. So maybe we should just do your > second alternative as-is (ie, code as above but without missing_ok). > Having an explicit control parameter seems marginally clearer but > I'm not sure it's buying anything meaningful. Thoughts? Hearing no comments, I did it the first way. regards, tom lane
Re: pgsql: Improve error handling in RemovePgTempFiles().
On Mon, Jan 8, 2018 at 2:41 PM, Tom Lane wrote: > I wrote: >> Hmmm ... actually, in the recursive call case, it wouldn't be that >> awful to ignore ENOENT either; if a directory goes away between being >> stat'd and being opened, you'd still get a log message about rmdir >> failure at the caller level. So maybe we should just do your >> second alternative as-is (ie, code as above but without missing_ok). >> Having an explicit control parameter seems marginally clearer but >> I'm not sure it's buying anything meaningful. Thoughts? > > Hearing no comments, I did it the first way. It's funny that the two boolean arguments are always opposite. They're essentially both saying "top level?". I was also a bit confused about who else would delete the file in between the check and the attempt to open it with my proposal, considering this is code running in the postmaster at startup, so I figured I must be missing something and hadn't got around to figure out what and replying. Thanks for fixing this. -- Thomas Munro http://www.enterprisedb.com
Re: pgsql: Improve error handling in RemovePgTempFiles().
Thomas Munro writes: > On Mon, Jan 8, 2018 at 2:41 PM, Tom Lane wrote: >> Hearing no comments, I did it the first way. > It's funny that the two boolean arguments are always opposite. > They're essentially both saying "top level?". Yeah. I thought about using a single "bool top_level" argument instead, but concluded that this way is clearer and potentially more flexible. There are other places where we have similar cases of flag arguments that are redundant in current usage, but we leave it like that for clarity as to what the function's API spec is. regards, tom lane
