Re: directory archive format for pg_dump

Hi,

Sharing some thoughts after a first round of reviewing, where I only had
time to read the patch itself.

Joachim Wieland <joe(at)mcknight(dot)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