From: | Boszormenyi Zoltan <zb(at)cybertec(dot)at> |
---|---|
To: | Magnus Hagander <magnus(at)hagander(dot)net> |
Cc: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [PATCH] Make pg_basebackup configure and start standby [Review] |
Date: | 2012-11-20 16:03:59 |
Message-ID: | 50ABA9EF.8080605@cybertec.at |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
2012-11-18 17:20 keltezéssel, Magnus Hagander írta:
> On Tue, Oct 23, 2012 at 5:08 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> On Oct 23, 2012 4:52 PM, "Alvaro Herrera" <alvherre(at)2ndquadrant(dot)com> wrote:
>>> Boszormenyi Zoltan escribió:
>>>
>>>>>> Also, the check for conflict between -R and -x/-X is now removed.
>>>> The documentation for option -R has changed to reflect this and
>>>> there is no different error code 2 now: it would make the behaviour
>>>> inconsistent between -Fp and -Ft.
>>>>
>>>>>>> The PQconninfo patch is also attached but didn't change since the
>>>>>>> last mail.
>>> Magnus,
>>>
>>> This patch is all yours to handle. I'm guessing nothing will happen
>>> until pgconf.eu is done and over, but hopefully you can share a few
>>> beers with Zoltan over the whole subject (and maybe with Peter about the
>>> PQconninfo stuff?)
>>>
>>> I'm not closing this just yet, but if you're not able to handle this
>>> soon, maybe it'd be better to punt it to the November commitfest.
>> It's on my to do list for when I get back, but correct, won't get to it
>> until after the conference.
> I finally got around to looking at this patch now. Sorry about the way
> too long delay.
>
> A few thoughts:
>
> First, on the libpq patch:
>
> I'm not sure I like the for_replication flag to PQconninfo(). It seems
> we're it's a quite limited API, and not very future proof. What's to
> say that an app would only be interested in filtering based on
> for_replication? One idea might be to have a bitmap field there, and
> assign *all* conninfo options to a category. We could then have
> categories for NORMAL and REPLICATION. But we could also for example
> have a category for PASSWORD (or similar), so that you could get with
> and without those. Passing in a 32-bit integer would allow us to have
> 32 different categories, and we could then use a bitmask to pick
> things out.
>
> It might sound a bit like overengineering, but it's also an API and
> it's a PITA to change it in the future if more needs show up..
Check.
> Second, I wonder if we really need to add the code for requiressl=1,
> or if we should just remove it. The spelling requiressl=1 was
> deprecated back in 7.4 - which has obviously been out of support for a
> long time now.
I removed this option, the code is simpler, thanks to this.
> Third, in fillPGconn. If mapping has both conninfoValue and connvalue,
> it does a free() on the old value in memberaddr, but if it doesn't it
> just overwrites memberaddr with strdup(). Shouldn't those two paths be
> the same, meaning shouldn't the if (*memberaddr) free(*memberaddr);
> check be outside the if block?
This point is now moot, see above.
> Fourth, I'm not sure I like the "memberaddr" term. It's not wrong, but
> it threw me off a couple of times while reading it. It's not all that
> important, and I'm not sure about another idea for it though - maybe
> just "connmember"?
The variable is now "connmember".
Also, I noticed that there was already a conninfo_storeval(),
the new patch uses it and there's no need to introduce a
new conninfo_setval() function.
> Then, about the pg_basebackup patch:
>
> What's the reason for the () around 512 for TARCHUNKSZ?
Removed the () from around the value.
> We have a lot of hardcoded entries of the 512 for tar in that file. We
> should either keep using 512 as a constant, or change all those to
> *also* use the #define. Right now, the code will obviously break if
> you change the #define (for example, it compares to 512, but then uses
> hdrleft = TARCHUNKSZ - tarhdrsz; to do calculation).
All 512 constants are now using the #define.
> The name choice of "basebackup" for the bool in ReceiveTarFile() is
> unfortunate, since we use the term base backup to refer to the
> complete thing, not just the main tablespace. Probably
> "basetablespcae" instead. And it should then be assigned at the top of
> the function (to the result of PQgetisnull()), and used in the main
> if() branch as well.
Done without your typo, so the variable is "basetablespace". ;-)
> Should we really print the status message even if not in verbose mode?
> We only print the "base backup complete" messages in verbose mode, for
> example.
The message is written only in verbose mode now.
> It seems wrong to free() recoveryconf in ReceiveTarFile(). It's
> allocated globally at the beginning. While that loop should only be
> called once (since only one tablespace can be the main one), it's a
> confusing location for the free.
See below.
> The whole tar writing part of the code needs a lot more comments. It's
> entirely unclear what the code does there. Why does the recovery.conf
> writing code need to be split up in multiple locations inside
> ReceiveTarFile(), for example? It either needs to be simplified, or
> explained why it can't be simplified in comments.
>
> _tarCreateHeader() is really badly named, since it specifically
> creates a tar header for recovery.conf only. Either that needs to be a
> parameter, or it needs to have a name that indicates this is the only
> thing it does. The former is probably better.
_tarCreateHeader() now accepts the file name and the file size arguments.
> Much of the tar stuff is very similar (I haven't looked to see if it's
> identical) to the stuff in backend/replication/basebackup.c. Perhaps
> we should have a src/port/tarutil.c?
I will implement it as a separate patch.
> escape_string() - already exists as escape_quotes() in initdb, AFAICT.
> We should either move it to src/port/, or at least copy it so it's
> exactly the same.
A copy of escape_quotes() is now in pg_basebackup.c and is used.
I will also unify the copies of it in a separate patch.
> CreateRecoveryConf() should just use PQExpBuffer (in libpq), I think -
> that does away with a lot of code. We already use this from e.g.
> pg_dump, so there's a precedent for using internal code from libpq in
> frontends.
PQexpBuffer is used now and it's created and destroyed inside BaseBackup().
> Again, my apologies for this review taking so long. I will try to be
> more attentive to the next round :S
Please, review the new patches.
Best regards,
Zoltán Böszörményi
> --
> Magnus Hagander
> Me: http://www.hagander.net/
> Work: http://www.redpill-linpro.com/
>
>
--
----------------------------------
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/
Attachment | Content-Type | Size |
---|---|---|
01-PQconninfo-v15.patch | text/x-patch | 15.7 KB |
02-pg_basebackup-v15.patch | text/x-patch | 20.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Boszormenyi Zoltan | 2012-11-20 16:14:57 | Re: [PATCH] Make pg_basebackup configure and start standby [Review] |
Previous Message | Albe Laurenz | 2012-11-20 15:11:28 | Re: [v9.3] writable foreign tables |