Received: from maia.hub.org (maia-3.hub.org [200.46.204.243])
	by mail.postgresql.org (Postfix) with ESMTP id DEB25133740B
	for <pgsql-hackers-postgresql.org@mail.postgresql.org>;
	Fri, 19 Nov 2010 15:44:26 -0400 (AST)
Received: from mail.postgresql.org ([200.46.204.86])
	by maia.hub.org (mx1.hub.org [200.46.204.243]) (amavisd-maia,
	port 10024) with ESMTP id 91084-04
	for <pgsql-hackers-postgresql.org@mail.postgresql.org>;
	Fri, 19 Nov 2010 19:44:19 +0000 (UTC)
X-Greylist: from auto-whitelisted by SQLgrey-1.7.6
Received: from acidenitrix.villemain.org (acidenitrix.villemain.org
	[91.121.90.165])
	by mail.postgresql.org (Postfix) with ESMTP id C6E0D1337B80
	for <pgsql-hackers@postgresql.org>;
	Fri, 19 Nov 2010 15:44:18 -0400 (AST)
Received: from 89-158-241-48.rev.dartybox.com ([89.158.241.48]
	helo=DimIMac.local) by acidenitrix.villemain.org with esmtpsa
	(TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.69)
	(envelope-from <dimitri@2ndQuadrant.fr>)
	id 1PJWsY-000276-4V; Fri, 19 Nov 2010 20:44:14 +0100
Received: by DimIMac.local (Postfix, from userid 501)
	id F0A1911F6677; Fri, 19 Nov 2010 20:44:06 +0100 (CET)
From: Dimitri Fontaine <dimitri@2ndQuadrant.fr>
To: Joachim Wieland <joe@mcknight.de>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: directory archive format for pg_dump
Organization: 2ndQuadrant
References: <AANLkTimUELTXwRSQDQNwxik_k1y3YcH1u-9NgHZqpi9e@mail.gmail.com>
User-Mail-Address: dimitri@2ndQuadrant.fr
Date: Fri, 19 Nov 2010 20:44:06 +0100
In-Reply-To: <AANLkTimUELTXwRSQDQNwxik_k1y3YcH1u-9NgHZqpi9e@mail.gmail.com>
	(Joachim Wieland's message of "Sun, 14 Nov 2010 18:48:07 -0500")
Message-ID: <m28w0pm73t.fsf@2ndQuadrant.fr>
User-Agent: Gnus/5.110011 (No Gnus v0.11) Emacs/24.0.50 (darwin)
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
X-Virus-Scanned: Maia Mailguard 1.0.1
X-Spam-Status: No, hits=-1.9 tagged_above=-5 required=5 tests=BAYES_00=-1.9
X-Spam-Level: 
X-Archive-Number: 201011/1295
X-Sequence-Number: 174438

Hi,

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

Joachim Wieland <joe@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=E2=80=A6

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=C2=A0:)

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,
--=20
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support