On 01/22/2016 06:45 AM, Michael Paquier wrote:

So, I have been playing with a Linux VM with VMware Fusion and on
ext4 with data=ordered the renames are getting lost if the root
folder is not fsync. By killing-9 the VM I am able to reproduce that
really easily.

Yep. Same experience here (with qemu-kvm VMs).

Here are some comments about your patch after a look at the code.

Regarding the additions in fsync_fname() in xlog.c:
1) In InstallXLogFileSegment, rename() will be called only if
HAVE_WORKING_LINK is not used, which happens only on Windows and
cygwin. We could add it for consistency, but it should be within the
#else/#endif block. It is not critical as of now.
2) The call in RemoveXlogFile is not necessary, the rename happening
only on Windows.

Hmmm, OK. Are we sure HAVE_WORKING_LINK is false only on Windows, or could there be some other platforms? And are we sure the file systems on those platforms are safe without the fsync call?

That is, while the report references ext4, there may be other file systems with the same problem - ext4 was used mostly as it's the most widely used Linux file system.

3) In exitArchiveRecovery if the rename is not made durable I think
it does not matter much. Even if recovery.conf is the one present
once the node restarts node would move back again to recovery, and
actually we had better move back to recovery in this case, no?

I'm strongly against this "optimization" - I'm more than happy to exchange the one fsync for not having to manually fix the database after crash.

I don't really see why switching back to recovery should be desirable in this case? Imagine you have a warm/hot standby, and that you promote it to master. The clients connect, start issuing commands and then the system crashes and loses the recovery.conf rename. The system reboots, database performs local recovery but then starts as a standby and starts rejecting writes. That seems really weird to me.

4) StartupXLOG for the tablespace map. I don't think this one is
needed as well. Even if the tablespace map is not removed after a
power loss user would get an error telling that the file should be

Please no, for the same reasons as in (3).

5) For the one where haveBackupLabel || haveTblspcMap. If we do the
fsync, we guarantee that there is no need to do again the recovery.
But in case of a power loss, isn't it better to do the recovery again?

Why would it be better? Why should we do something twice when we don't have to? Had this not be reliable, then the whole recovery process is fundamentally broken and we better fix it instead of merely putting a band-aid on it.

6) For the one after XLogArchiveNotify() for the last partial
segment of the old timeline, it doesn't really matter to not make the
change persistent as this is mainly done because it is useful to
identify that it is a partial segment.

OK, although I still don't quite see why that should be a reason not to do the fsync. It's not really going to give us any measurable performance advantage (how often we do those fsyncs), so I'd vote to keep it and make sure the partial segments are named accordingly. Less confusion is always better.

7) In CancelBackup, this one is not needed as well I think. We would
surely want to get back to recovery if those files remain after a
power loss.

I may be missing something, but why would we switch to recovery in this case?

For the ones in xlogarchive.c:
1) For KeepFileRestoredFromArchive, it does not matter here, we are
renaming a file with a temporary name to a permanent name. Once the
node restarts, we would see an extra temporary file if the rename
was not effective.

So we'll lose the segment (won't have it locally under the permanent name), as we've already restored it and won't do that again. Is that really a good thing to do?

2) XLogArchiveForceDone, the only bad thing that would happen here is
to leave behind a .ready file instead of a .done file. I guess that we
could have it though as an optimization to not have to archive again
this file.


For the one in pgarch.c:
1) In pgarch_archiveDone, we could add it as an optimization to
actually let the server know that the segment has been already
archived, preventing a retry.

Not sure what you mean by "could add it as an optimization"?

In timeline.c:
1) writeTimeLineHistoryFile, it does not matter much. In the worst
case we would have just a dummy temporary file, and startup process
would come back here (in exitArchiveRecovery() we may finish with an
unnamed backup file similarly).


2) In writeTimeLineHistoryFile, similarly we don't need to care
much, in WalRcvFetchTimeLineHistoryFiles recovery would take again
the same path



Thanks for the review and comments. I think the question is whether we only want to do the additional fsync() only when it ultimately may lead to data loss, or even in cases where it may cause operational issues (e.g. switching back to recovery needlessly).

I'd vote for the latter, as I think it makes the database easier to operate (less manual interventions) and the performance impact is 0 (as those fsyncs are really rare).


Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to