On Sun, Mar 17, 2019 at 10:01:20AM +0100, Fabien COELHO wrote: > The implementation basically removes a lot of copy paste and calls the new > update_controlfile function instead. I like removing useless code:-)
Yes, I spent something like 10 minutes looking at that code yesterday and I agree that removing the control file to recreate it is not really necessary, even if the window between its removal and recreation is short. > I do not see the value of *not* fsyncing the control file when writing it, > as it is by definition very precious, so I added a fsync. The server side > branch uses the backend available "pg_fsync", which complies with server > settings there and can do nothing if fsync is disabled. The issue here is that trying to embed directly the fsync routines from file_utils.c into pg_resetwal.c messes up the inclusions because pg_resetwal.c includes backend-side includes, which themselves touch fd.h :( In short your approach avoids some extra mess with the include dependencies. . > Maybe the two changes could be committed separately. I was thinking about this one, and for pg_rewind we don't care about the fsync of the control file because the full data folder gets fsync'd afterwards and in the event of a crash in the middle of a rewind the target data folder is surely not something to use, but we do for pg_checksums, and we do for pg_resetwal. Even if there is the argument that usually callers of update_controlfile() would care a lot about the control file and fsync it, I think that we need some control on if we do the fsync or not because many tools have a --no-sync and that should be fully respected. So while your patch is on a good track, I would suggest to do the following things to complete it: - Add an extra argument bits16 to update_controlfile to pass a set of optional flags, with NOSYNC being the only and current value. The default is to flush the file. - Move the wait event calls WAIT_EVENT_CONTROL_FILE_WRITE_UPDATE and WAIT_EVENT_CONTROL_FILE_SYNC_UPDATE to controldata_utils.c. - And then delete UpdateControlFile() in xlog.c, and use update_controlfile() instead to remove even more code. The version in xlog.c uses BasicOpenFile(), so we should use also that in update_controlfile() instead of OpenTransientFile(). As any errors result in a PANIC we don't care about leaking fds. -- Michael
signature.asc
Description: PGP signature
