On Tue, Mar 8, 2016 at 12:18 PM, Andres Freund <andres@anarazel.de> wrote:
> On 2016-03-08 12:01:18 +0900, Michael Paquier wrote:
>> I have spent a couple of hours looking at that in details, and the
>> patch is neat.
>
> Cool. Doing some more polishing right now. Will be back with an updated
> version soonish.
>
> Did you do some testing?
Not much in details yet, I just ran a check-world with fsync enabled
for the recovery tests, plus quick manual tests with a cluster
manually set up. I'll do more with your new version now that I know
there will be one.
>> + /* XXX: Add racy file existence check? */
>> + if (rename(oldfile, newfile) < 0)
>
>> I am not sure we should worry about that, what do you think could
>> cause the old file from going missing all of a sudden. Other backend
>> processes are not playing with it in the code paths where this routine
>> is called. Perhaps adding a comment in the header to let users know
>> that would help?
>
> What I'm thinking of is adding a check whether the *target* file already
> exists, and error out in that case. Just like the link() based path
> normally does.
Ah, OK. Well, why not. I'd rather have an assertion instead of an error though.
--
Michael