Re: initdb -S and tablespaces

Hi,

On 2014-11-06 17:56:53 +0530, Abhijit Menon-Sen wrote:
> +    /*
> +     * If we need to perform crash recovery, we issue an fsync on the
> +     * data directory and its contents to try to ensure that any data
> +     * written before the crash are flushed to disk. Otherwise a power
> +     * failure in the near future might cause earlier unflushed writes
> +     * to be lost, even though more recent data written to disk from
> +     * here on would be persisted.
> +     */
> +
> +    if (ControlFile->state != DB_SHUTDOWNED &&
> +        ControlFile->state != DB_SHUTDOWNED_IN_RECOVERY)
> +        perform_fsync(data_directory);
> +

a) Please think of a slightly more descriptive name than perform_fsync
b) I think we should check here whether fsync is enabled.
c) I'm wondering if we should add fsync to the control file and also  perform an fsync if the last shutdown was clear,
butfsync was  disabled.
 
>      if (ControlFile->state == DB_SHUTDOWNED)
>      {
>          /* This is the expected case, so don't be chatty in standalone mode */
> @@ -11262,3 +11281,168 @@ SetWalWriterSleeping(bool sleeping)
>      XLogCtl->WalWriterSleeping = sleeping;
>      SpinLockRelease(&XLogCtl->info_lck);
>  }
> +
> +/*
> + * Hint to the OS that it should get ready to fsync() this file.
> + *
> + * Adapted from pre_sync_fname in initdb.c
> + */
> +static void
> +pre_sync_fname(char *fname, bool isdir)
> +{

this essentially already exists in the backend inparts. C.f. pg_flush_data.

> +/*
> + * walkdir: recursively walk a directory, applying the action to each
> + * regular file and directory (including the named directory itself).
> + *
> + * Adapted from copydir() in copydir.c.
> + */
> +static void
> +walkdir(char *path, void (*action) (char *fname, bool isdir))
> +{
> +    DIR           *dir;
> +    struct dirent *de;
> +
> +    dir = AllocateDir(path);
> +    while ((de = ReadDir(dir, path)) != NULL)
> +    {
> +        char        subpath[MAXPGPATH];
> +        struct stat fst;
> +
> +        CHECK_FOR_INTERRUPTS();
> +
> +        if (strcmp(de->d_name, ".") == 0 ||
> +            strcmp(de->d_name, "..") == 0)
> +            continue;
> +
> +        snprintf(subpath, MAXPGPATH, "%s/%s", path, de->d_name);
> +
> +        if (lstat(subpath, &fst) < 0)
> +            ereport(ERROR,
> +                    (errcode_for_file_access(),
> +                     errmsg("could not stat file \"%s\": %m", subpath)));
> +
> +        if (S_ISDIR(fst.st_mode))
> +            walkdir(subpath, action);
> +        else if (S_ISREG(fst.st_mode))
> +            (*action) (subpath, false);

Theoretically you should also invoke fsync on directories.

> +/*
> + * Issue fsync recursively on PGDATA and all its contents, including the
> + * links under pg_tblspc.
> + *
> + * Adapted from perform_fsync in initdb.c
> + */
> +static void
> +perform_fsync(char *pg_data)
> +{
> +    char        pdir[MAXPGPATH];
> +    char        pg_tblspc[MAXPGPATH];
> +
> +    /*
> +     * We need to name the parent of PGDATA.  get_parent_directory() isn't
> +     * enough here, because it can result in an empty string.
> +     */
> +    snprintf(pdir, MAXPGPATH, "%s/..", pg_data);
> +    canonicalize_path(pdir);

Hm. Why is this neded? Just syncing . should work?

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services