Re: silent data loss with ext4 / all current versions

On 2016-01-25 16:30:47 +0900, Michael Paquier wrote:
> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
> index a2846c4..b124f90 100644
> --- a/src/backend/access/transam/xlog.c
> +++ b/src/backend/access/transam/xlog.c
> @@ -3278,6 +3278,14 @@ InstallXLogFileSegment(XLogSegNo *segno, char *tmppath,
> tmppath, path)));
> return false;
> }
> +
> + /*
> + * Make sure the rename is permanent by fsyncing the parent
> + * directory.
> + */
> + START_CRIT_SECTION();
> + fsync_fname(XLOGDIR, true);
> + END_CRIT_SECTION();
> #endif

Hm. I'm seriously doubtful that using critical sections for this is a
good idea. What's the scenario you're protecting against by declaring
this one? We intentionally don't error out in the isdir cases in
fsync_fname() cases anyway.

Afaik we need to fsync tmppath before and after the rename, and only
then the directory, to actually be safe.

> @@ -5297,6 +5313,9 @@ exitArchiveRecovery(TimeLineID endTLI, XLogRecPtr endOfLog)
> errmsg("could not rename file \"%s\" to \"%s\": %m",
> RECOVERY_COMMAND_FILE, RECOVERY_COMMAND_DONE)));
>
> + /* Make sure the rename is permanent by fsyncing the data directory. */
> + fsync_fname(".", true);
> +

Shouldn't RECOVERY_COMMAND_DONE be fsynced first here?

> ereport(LOG,
> (errmsg("archive recovery complete")));
> }
> @@ -6150,6 +6169,12 @@ StartupXLOG(void)
> TABLESPACE_MAP, BACKUP_LABEL_FILE),
> errdetail("Could not rename file \"%s\" to \"%s\": %m.",
> TABLESPACE_MAP, TABLESPACE_MAP_OLD)));
> +
> + /*
> + * Make sure the rename is permanent by fsyncing the data
> + * directory.
> + */
> + fsync_fname(".", true);
> }

Is it just me, or are the repeated four line comments a bit grating?

> /*
> @@ -6525,6 +6550,13 @@ StartupXLOG(void)
> TABLESPACE_MAP, TABLESPACE_MAP_OLD)));
> }
>
> + /*
> + * Make sure the rename is permanent by fsyncing the parent
> + * directory.
> + */
> + if (haveBackupLabel || haveTblspcMap)
> + fsync_fname(".", true);
> +

Isn't that redundant with the haveTblspcMap case above?

> /* Check that the GUCs used to generate the WAL allow recovery */
> CheckRequiredParameterValues();
>
> @@ -7305,6 +7337,12 @@ StartupXLOG(void)
> errmsg("could not rename file \"%s\" to \"%s\": %m",
> origpath, partialpath)));
> XLogArchiveNotify(partialfname);
> +
> + /*
> + * Make sure the rename is permanent by fsyncing the parent
> + * directory.
> + */
> + fsync_fname(XLOGDIR, true);

.partial should be fsynced first.

> }
> }
> }
> @@ -10905,6 +10943,9 @@ CancelBackup(void)
> BACKUP_LABEL_FILE, BACKUP_LABEL_OLD,
> TABLESPACE_MAP, TABLESPACE_MAP_OLD)));
> }
> +
> + /* fsync the data directory to persist the renames */
> + fsync_fname(".", true);
> }

Same.

> /*
> diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c
> index 277c14a..8dda80b 100644
> --- a/src/backend/access/transam/xlogarchive.c
> +++ b/src/backend/access/transam/xlogarchive.c
> @@ -477,6 +477,12 @@ KeepFileRestoredFromArchive(char *path, char *xlogfname)
> path, xlogfpath)));
>
> /*
> + * Make sure the renames are permanent by fsyncing the parent
> + * directory.
> + */
> + fsync_fname(XLOGDIR, true);

Afaics the file under the temporary filename has not been fsynced up to
here.

> + /*
> * Create .done file forcibly to prevent the restored segment from being
> * archived again later.
> */
> @@ -586,6 +592,11 @@ XLogArchiveForceDone(const char *xlog)
> errmsg("could not rename file \"%s\" to \"%s\": %m",
> archiveReady, archiveDone)));
>
> + /*
> + * Make sure the rename is permanent by fsyncing the parent
> + * directory.
> + */
> + fsync_fname(XLOGDIR "/archive_status", true);
> return;
> }

Afaics the AllocateFile() call below is not protected at all, no?

How about introducing a 'safe_rename()' that does something roughly akin
to:
fsync(oldname);
fsync(fname) || true;
rename(oldfname, fname);
fsync(fname);
fsync(basename(fname));

I'd rather have that kind of logic somewhere once, instead of repeated a
dozen times...

Greetings,

Andres Freund