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 g...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
