Hi Dimitri,
thanks for reviewing my patch!
On Fri, Nov 19, 2010 at 2:44 PM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:
> I think I'd like to see a separate patch for the new compression
> support. Sorry about that, I realize that's extra work…
I guess it wouldn't be a very big deal but I also doubt that it makes
the review that much easier. Basically the compression refactor patch
would just touch pg_backup_custom.c (because this is the place where
the libz compression is currently burried into) and the two new
compress_io.(c|h) files. Everything else is pretty much the directory
stuff and is on top of these changes.
> 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?
Sure we could. But I wanted to wait with any fancy function pointer
stuff until we have decided if we want to include the liblzf support
at all. The #ifdefs might be a bit ugly but in case we do not include
liblzf support, it's the easiest way to take it out again. As written
in my introduction, this patch is not really about liblzf, liblzf is
just a proof of concept for factoring out the compression part and I
have included it, so that people can use it and see how much speed
improvement they get.
> 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.
Sure! I completely agree...
> 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 :)
Well, not sure if we can just change the copyright notice, because in
the end the structure was copied from one of the other files which all
have the copyright notice in them, so my work is based on those other
files...
> I'm hesitant as far as marking the patch "Waiting on author" to get it
> split. Joachim, what do you think?
I will see if I can split it.
Joachim