Hi, Sharing some thoughts after a first round of reviewing, where I only had time to read the patch itself.
Joachim Wieland <j...@mcknight.de> writes: > Since the compression is currently all down in the custom format > backup code, > the first thing I've done was refactoring the compression functions > into a > separate file. While at it, I have added support for liblzf > compression. I think I'd like to see a separate patch for the new compression support. Sorry about that, I realize that's extra work… And it could be about personal preferences, but the way you added the liblzf support strikes me at odd, with all those #ifdefs everywhere. Is it possible to have a specific file for each supported compression format, then some routing code in src/bin/pg_dump/compress_io.c? The routing code already exists but then the file is full of #ifdef sections to define the right supporting function when I think having a compress_io_zlib and a compress_io_lzf files would be better. Then there's the bulk of the new dump format feature in the other part of the patch, namely src/bin/pg_dump/pg_backup_directory.c. You have to update the copyright in the file header there, at least :) I'm yet to devote more time on this part of the patch but it seems like it's rewriting the full support without using the existing bits. That's something I have to check, didn't have time to read the existing other archive formats code there. I'm hesitant as far as marking the patch "Waiting on author" to get it split. Joachim, what do you think? Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers