On Tue, Oct 23, 2012 at 5:08 PM, Magnus Hagander <mag...@hagander.net> wrote: > > On Oct 23, 2012 4:52 PM, "Alvaro Herrera" <alvhe...@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.. 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. 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? 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"? Then, about the pg_basebackup patch: What's the reason for the () around 512 for TARCHUNKSZ? 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). 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. 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. 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. 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. 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? 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. 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. Again, my apologies for this review taking so long. I will try to be more attentive to the next round :S -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers