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.paquier@gmail.com> wrote:
>> On Thu, Feb 4, 2016 at 12:02 PM, Michael Paquier
>> <michael.paquier@gmail.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