Re: [PATCH] Make pg_basebackup configure and start standby [Review] - Mailing list pgsql-hackers

From Magnus Hagander
Subject Re: [PATCH] Make pg_basebackup configure and start standby [Review]
Date
Msg-id CABUevEww0Zhuca6rtXA2kijbC-ngWLfinsLggY2qWicHCuKYig@mail.gmail.com
Whole thread Raw
In response to Re: [PATCH] Make pg_basebackup configure and start standby [Review]  (Boszormenyi Zoltan <zb@cybertec.at>)
List pgsql-hackers

On Tue, Jan 1, 2013 at 7:13 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
2013-01-01 18:25 keltezéssel, Magnus Hagander írta:

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.

Hmm. Somehow, I missed that part of the discussion. Sorry, didn't realize it had been mentioned already.
 

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...

I thought commit add6c3179a4d4fa3e62dd3e86a00f23303336bac at leasdt broke that? In particular, did you test where any of those values were different between client and server? Or maybe just didn't test in -x mode?

I actually thought there was something else that broke the compatibility, but I can't seem to find it so perhaps that was something that was never committed.

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

pgsql-hackers by date:

Previous
From: Tom Lane
Date:
Subject: Re: dynamic SQL - possible performance regression in 9.2
Next
From: Boszormenyi Zoltan
Date:
Subject: Re: [PATCH] Make pg_basebackup configure and start standby [Review]