At 2015-01-14 11:59:08 +0100, andres@2ndquadrant.com wrote:
>
> > + 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
OK. (I just copied the name initdb uses, because at the time I was still
thinking in terms of a later patch moving this to src/common.) What do
you think of fsync_recursively? fsync_pgdata?
I think fsync_recursively(data_directory) reads well.
> b) I think we should check here whether fsync is enabled.
OK, will do.
> c) I'm wondering if we should add fsync to the control file and also
> perform an fsync if the last shutdown was clear, but fsync was
> disabled.
Explain? "Add fsync to the control file" means store the value of the
fsync GUC setting in the control file? And would the fsync you mention
be dependent on the setting, or unconditional?
> > +static void
> > +pre_sync_fname(char *fname, bool isdir)
> > +{
>
> this essentially already exists in the backend inparts. C.f.
> pg_flush_data.
OK, I missed that. I'll rework the patch to use it.
> Theoretically you should also invoke fsync on directories.
OK.
> > + * 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?
Not sure, will investigate.
Thanks.
-- Abhijit
