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

On Thursday, October 04, 2012 9:50 PM Boszormenyi Zoltan
> 2012-10-04 16:43 keltezéssel, Tom Lane írta:
> > Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
> >>> Did you think about something like the attached code?
> >> Or rather this one, which fixes a bug so fillPGconn() and
> >> PQconninfo() are symmetric and work for "requiressl".
> > That's incredibly ugly. I'm not sure where we should keep the "R"
> > information, but shoehorning it into the existing PQconninfoOption
> > struct like that seems totally unacceptable. Either we're willing to
> > break backwards compatibility on the Disp-Char strings, or we need to
> > put that info somewhere else.
>
> I hope only handling the new flag part is ugly. It can be hidden in the
> PQconninfoMapping[] array and PQconninfo(conn, true) pre-filters the
> list as in the attached version.

Please find the review of the patch.

Basic stuff:
------------
- patch apply failed at exports.txt file. Needs rebase to the current
master.
- Compiles cleanly with no warnings

What it does:
--------------
pg_basebackup does the base backup from the primary machine and also writes
the recovery.conf file with the option -R,
which is used for the standby to connect to primary for streaming
replication.

Testing:
---------
1. Test pg_basebackup with option -R to check that the recovery.conf file is
written to data directory.
--recovery.conf file is created in data directory.

2. Test pg_basebackup with option -R to check that the recovery.conf file is
not able to create because of disk full.
--Error is given as recovery.conf file is not able to create.

3. Test pg_basebackup with option -R including -h, -U, -p, -w and -W.
verify the recovery.conf which is created in data directory.
--All the primary connection info parameters are working fine.

4. Test pg_basebackup with conflict options (-x or -X and -R).
--Error is given when the conflict options are provided to
pg_basebackup.

5. Test pg_basebackup with option -R from a standby server.
--recovery.conf file is created in data directory.

Code Review:
-------------
1.
typedef struct PQconninfoMapping {
+ char *keyword;
+ size_t member_offset;
+ bool for_replication;
+ char *conninfoValue; /* Special value mapping
*/
+ char *connValue;
+} PQconninfoMapping;

Provide the better explanation of conninfoValue and connValue, how and where
these are used?

2. if (tmp && strncmp(tmp, mapping->conninfoValue, len) == 0)

In any case if the above condition is not satisfied then the PGconn data is
not filled with PQconninfoOption.
Is it correct?

Documentation:
-------------
1.
+ <para>
+ The <literal>for_replication</> argument can be used to exclude some

+ options from the list which are used by the walreceiver module.
+ <application>pg_basebackup</application> uses this pre-filtered list

+ to construct <literal>primary_conninfo</> in the automatically
generated
+ recovery.conf file.
+ </para>

I feel the explanation should be as follows,
exclude some options from the list which are not used by the walreceiver
module?

Observations/Issues:
-------------------
1. If the password contains any escape sequence characters, It is leading to
problems while walreceiver connecting to primary
by using the primary conninfo from recovery.conf

please log an warning message or a note in document to handle such a case
manually by the user.

With Regards,
Amit Kapila.