Re: [PATCH] Make pg_basebackup configure and start standby [Review]

On Fri, Nov 30, 2012 at 10:13 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:

Hi,

now that PQconninfo() was committed, I rebased the subsequent
patches. Actual code change was only in the last patch, to
conform to the committed PQconninfo() API.

I've applied a modified version of the "tar unification" patch to master. It didn't apply cleanly so I ended up redoing a number of the things manually, but the end result is fairly similar. I don't think it'll cause anything but really trivial merge conflicts against the 04 patch, but I didn't actually check that (e.g. it's tarCreateHeader() not _tarCreateHeader() now).

Thanks.

I took at look at the basebackup patch as well. Easier to get now that it's commented :) There's quite a lot of code in there whereby it tries to remove recovery.conf from the tar stream if it's already in there. That seems like an ugly way to do it. I see two other options, that I think both are better. If we do need it to be removed, we should probably pass that as a parameter up to the walsender, and make sure the file isn't included in the first place. But we can also leave it in there - the tar format is perfectly happy to have multiple copies of the same file in the archive - it'll just use whichever copy comes last.

IIRC, Fujii Masao's wish was to remove the recovery.conf from the tar stream.
I know that the tar format is perfectly happy with two files with the same
path name but his reasoning was that GUIs (like WinRar) may get confused
by two such files and cannot decide which one to extract. I didn't actually
test this but it is a reasonable suspicion.

Passing a parameter to the walsender may be a better solution.
It's a bad idea only because it wasn't mine. ;-)

The only problem with this idea is that currently there's nothing that stops
pg_basebackup to be a generic and backwards-compatible tool.
It simply receives a TAR stream via plain PQgetCopyData() and optionally
extracts it. The client-side solution to skip the recovery.conf file keeps this
backward compatible feature. I tested this and pg_basebackup from 9.3dev
happily backs up a 9.2.2 database...

Given that the code there is already fairly difficult to track, I think just keeping it simpler is definitely worth doing one of those two. I would vote for just leaving two copies of recovery.conf in there.

Comments/thoughts from others?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Best regards,
Zoltán Böszörmény

-- 
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de     http://www.postgresql.at/