Re: silent data loss with ext4 / all current versions

Hi,

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
> removed.

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.

Yes.

>
> 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).

OK

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

OK

>
> Thoughts?
>

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).

regards

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