pg.ddx.io  pgsql-hackers@postgresql.org mailing list archive  

help / color / mirror / Atom feed
From: Greg Smith <greg@2ndquadrant.com>
To: Joachim Wieland <joe@mcknight.de>
Cc: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: José Arthur Benetasso Villanova <jose.arthur@gmail.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: directory archive format for pg_dump
Date: Thu, 16 Dec 2010 05:12:52 -0500
Message-ID: <4D09E624.4030706@2ndquadrant.com> (raw)
In-Reply-To: <AANLkTik=3XBZq-PbC9CNTV1X6+-+Yd9gOdwf7rYjCpBp@mail.gmail.com>
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>

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




view thread (40+ messages)  latest in thread

Message-ID: <4D09E624.4030706@2ndquadrant.com>
Permalink:  ../4D09E624.4030706@2ndquadrant.com/
Also on:    postgresql.org/message-id/4D09E624.4030706@2ndquadrant.com

 · 

reply
Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: pgsql-hackers@postgresql.org
  Cc: greg@2ndquadrant.com, joe@mcknight.de, heikki.linnakangas@enterprisedb.com, tgl@sss.pgh.pa.us, jose.arthur@gmail.com
  Subject: Re: directory archive format for pg_dump
  In-Reply-To: <4D09E624.4030706@2ndquadrant.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox