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

2012-11-18 17:20 keltezéssel, Magnus Hagander írta:
> On Tue, Oct 23, 2012 at 5:08 PM, Magnus Hagander <magnus@hagander.net> wrote:
>> On Oct 23, 2012 4:52 PM, "Alvaro Herrera" <alvherre@2ndquadrant.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/