On Thursday, October 04, 2012 9:50 PM Boszormenyi Zoltan
> 2012-10-04 16:43 keltezéssel, Tom Lane írta:
> > Boszormenyi Zoltan <z...@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 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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
