Message-ID: <4D09E624.4030706@2ndquadrant.com>
Date: Thu, 16 Dec 2010 05:12:52 -0500
From: Greg Smith <greg@2ndquadrant.com>
User-Agent: Thunderbird 2.0.0.24 (X11/20100317)
MIME-Version: 1.0
To: Joachim Wieland <joe@mcknight.de>
CC: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>,
	Tom Lane <tgl@sss.pgh.pa.us>,
	=?ISO-8859-1?Q?Jos=E9_Arthur_Benetasso_Villanova?=
	<jose.arthur@gmail.com>, pgsql-hackers@postgresql.org
Subject: Re: directory archive format for pg_dump
References: <AANLkTi=AEi3F6jC0Zy3dG9njiwJJC8Y6V090-hMonyyn@mail.gmail.com>
	<AANLkTik+PR2HCsqS7PVAdETPyKSGn2hb-eTeg9QZpRew@mail.gmail.com>
	<4CEA9F29.5060106@enterprisedb.com>	<28890.1290445623@sss.pgh.pa.us>
	<4CEAD61A.6060207@enterprisedb.com>
	<AANLkTi=neaBqvLa2ZnDpuAMSen3P4ZHE8__Q7L2pmjD0@mail.gmail.com>
	<4CF40B5F.1010407@enterprisedb.com>	<4CF655A3.4090308@enterprisedb.com>
	<4CF6562E.60506@enterprisedb.com>
	<AANLkTinauQsCGt-yeyeN_QWuN+tao+q126rwKnO2QWLP@mail.gmail.com>
	<4CF74F99.4050008@enterprisedb.com>	<4CF7F8FB.1040305@enterprisedb.com>
	<AANLkTik=3XBZq-PbC9CNTV1X6+-+Yd9gOdwf7rYjCpBp@mail.gmail.com>
In-Reply-To: <AANLkTik=3XBZq-PbC9CNTV1X6+-+Yd9gOdwf7rYjCpBp@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Moving onto the directory archive part of this patch, the feature seems 
to work as advertised; here's a quick test case:

createdb pgbench
pgbench -i -s 1 pgbench
pg_dump -F d -f test
pg_restore -k test
pg_restore -l test
createdb copy
pg_restore -d copy test

The copy made that way looked good.  There's a good chunk of code in the 
patch that revolves around BLOB support.  We need to get someone who is 
more familiar with those than me to suggest some tests for that part 
before this gets committed.  If you could suggest how to test that code, 
that would be helpful.

There's a number of small things that I'd like to see improved in new 
rev of this code

pg_dump:  help message for "--file" needs to mention that this is 
overloaded to also specify the output directory too.

pg_dump:  the documentation for --file should say the directory is 
created, and must not exist when you start.  The code catches this well, 
but that expectation is not clear until you try it.

pg_restore:  the help message "check the directory archive" would be 
clearer as "check an archive in directory format".

There are some tab vs. space whitespace inconsistencies in the 
documentation added.

The comments at the beginning of functions could be more consistent.  
Early parts of the code have a header for each function that's 
extensive.  Maybe even a bit more than needed.  I'm not sure why it's 
important to document here which of these functions is 
optional/mandatory for example, and getting rid of just those would trim 
a decent number of lines out of the patch.  But then at the end, all of 
the new functions added aren't documented at all.  Some of those are 
near trivial, but it would be better to have at least a small 
descriptive header for them.

The comment header at the beginning of pg_backup_directory is a bit 
weird.  I guess Philip Warner should still be credited as the author of 
the code this was based on, but it's a weird seeing a new file 
attributed solely to him.  Also, there's an XXX in the identification 
field there that should be filled in with the file name.

There's your feedback for this round.  I hope we'll see an updated patch 
from you as part of the next CommitFest.

-- 
Greg Smith   2ndQuadrant US    greg@2ndQuadrant.com   Baltimore, MD
PostgreSQL Training, Services and Support        www.2ndQuadrant.us
"PostgreSQL 9.0 High Performance": http://www.2ndQuadrant.com/books