On 02/05/2016 10:40 AM, Michael Paquier wrote:
On Thu, Feb 4, 2016 at 2:34 PM, Michael Paquier
<michael.paqu...@gmail.com> wrote:
On Thu, Feb 4, 2016 at 12:02 PM, Michael Paquier
<michael.paqu...@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

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:


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.


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