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

On Thursday, October 04, 2012 9:50 PM Boszormenyi Zoltan
> 2012-10-04 16:43 keltezéssel, Tom Lane írta:
> > Boszormenyi Zoltan <zb@cybertec.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
datadirectory.    --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
providedto 
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.