Re: fallocate / posix_fallocate for new WAL file creation (etc...)

On 5/16/13 9:16 AM, Jon Nelson wrote:
> Am I doing this the right way? Should I be posting the full patch each
> time, or incremental patches?

There are guidelines for getting your patch in the right format at
https://wiki.postgresql.org/wiki/Working_with_Git#Context_diffs_with_Git
that would improve this one. You have some formatting issues with tab
spacing at lines 120 through 133 in your v3 patch. And it looks like
there was a formatting change on line 146 that is making the diff larger
than it needs to be.

The biggest thing missing from this submission is information about what
performance testing you did. Ideally performance patches are submitted
with enough information for a reviewer to duplicate the same test the
author did, as well as hard before/after performance numbers from your
test system. It often turns tricky to duplicate a performance gain, and
being able to run the same test used for initial development eliminates
a lot of the problems.

Second bit of nitpicking. There are already some GUC values that appear
or disappear based on compile time options. They're all debugging
related things though. I would prefer not to see this one go away when
it's implementation isn't available. That's going to break any scripts
that SHOW the setting to see if it's turned on or not as a first
problem. I think the right model to follow here is the IFDEF setup used
for effective_io_concurrency. I wouldn't worry about this too much
though. Having a wal_use_fallocate GUC is good for testing. But if it
works out well, when it's ready for commit I don't see why anyone would
want it turned off on platforms where it works. There are already too
many performance tweaking GUCs. Something has to be very likely to be
changed from the default before its worth adding one for it.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com