Re: silent data loss with ext4 / all current versions

Hi,

On 02/05/2016 10:40 AM, Michael Paquier wrote:
> On Thu, Feb 4, 2016 at 2:34 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Thu, Feb 4, 2016 at 12:02 PM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>> On Tue, Feb 2, 2016 at 4:20 PM, Michael Paquier wrote:
...
> So, attached is an updated patch that adds a new routine link_safe()
> to ensure that a hard link is on-disk persistent. link() is called
> twice in timeline.c and once in xlog.c, so those three code paths
> are impacted. I noticed as well that my previous patch was sometimes
> doing palloc calls in a critical section (oops), I fixed that on the
> way.

I've finally got around to review the v5 version of the patch. Sorry it
took so long (I blame FOSDEM, country-wide flu epidemic and my general
laziness).

I do like most of the changes to the patch, thanks for improving it. A
few comments though:

1) I'm not quite sure why the patch adds missing_ok to fsync_fname()?
The only place where we use missing_ok=true is in rename_safe, where
right at the beginning we do this:

fsync_fname(newfile, false, true);

I.e. we're fsyncing the rename target first, in case it exists. But that
seems to be conflicting with the comments in xlog.c where we explicitly
state that the target file should not exist. Why should it be OK to call
rename_safe() when the target already exists? If this really is the
right thing to do, it should be explained in the comment above
rename_safe(), probably.

2) If rename_safe/link_safe are meant as crash-safe replacements for
rename/link, then perhaps we should use the same signatures, including
the "const" pointer parameters. So while currently the signatures look
like this:

int rename_safe(char *oldfile, char *newfile);
int link_safe(char *oldfile, char *newfile);

it should probably look like this

int rename_safe(const char *oldfile, const char *newfile);
int link_safe(const char *oldfile, const char *newfile);

I've noticed this in CheckPointReplicationOrigin() where the current
code has to cast the parameters to (char*) to silence the compiler.

3) Both rename_safe and link_safe do this at the very end:

fsync_parent_path(newfile);

That however assumes both the oldfile and newfile are placed in the same
directory - otherwise we'd fsync only one of them. I don't think we have
a place where we're renaming files between directories (or do we), so
we're OK with respect to this. But it seems like a good idea to defend
against this, or at least mention that in the comments.

4) nitpicking: There are some unnecessary newlines added/removed in
RemoveXlogFile, XLogArchiveForceDone.

regards

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