On Mon, 2012-06-18 at 20:57 +0200, Andres Freund wrote:
> > > - defaulting to initdb -N in the regression suite is not a good imo,
> > > because that way the buildfarm won't catch problems in that area...
> > I removed the -N as you suggest. How much does performance matter on the
> > buildfarm?
> I don't think the difference in initdb cost is relevant when running the
> regression tests. Should it prove to be we can re-add -N after a week or two
> in the buildfarm machines. I just remember that there were several OS specific
> regression when adding the pre-syncing for createdb.
That sounds reasonable to me. Both patches are out there, so we can
figure out what the consensus is.
> > > - could the copydir.c and initdb.c versions of walkdir/sync_fname et al
> > > be unified?
> > There's a lot of backend-specific code in the copydir versions, like
> > using ereport() and CHECK_FOR_INTERRUPTS(). I gave a brief attempt at
> > unifying them before, and concluded that it wouldn't add to the
> > readability, so I just commented where they came from.
> Ok. Sensible reasons. I dislike that we know have two files using different
> logic (copydir.c only using fadvise, initdb using sync_file_range if
> available). Maybe we should just move the fadvise and sync_file_range calls
> into its own common function?
I don't see fadvise in copydir.c, it looks like it just uses fsync. It
might speed it up to use a pre-sync call there, too -- database creation
does take a while.
If that's in the scope of this patch, I'll do it.
> Btw, I just want to have said this, although I don't think its particularly
> relevant as it doesn't affect correctness: Its possible to have a system where
> sync_file_range is in the system headers but the kernel during runtime doesn't
> support it. It is relatively new (2.6.17). It would be possible to fallback to
> posix_fadvise which has been around far longer in that case...
Interesting point, but I'm not too worried about it.
Regards,Jeff Davis