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

Hi,

thanks for the new review.

2012-10-10 08:58 keltezéssel, Amit Kapila írta:
> 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.

Done.

> - 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?

OK. This is only used for " requiressl='1' " (in the connection string)
and if '1' is set, PGconn.sslmode will be set to "require". All other
values are treated as it's being unset. This simplistic mapping
is used because there is only one such setting where different values
are used on the conninfo and the PGconn sides.

> 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?

Yes, it stays NULL as makeEmptyPGconn() initialized it. This case only happens
with the "requiressl" setting with its special mapping. If you set " requiressl = '0' "
then it means that " requiressl='1' " was not set so the PGconn side stays as NULL.

The special casing was there in the old code too and behaves the same.

> 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?

Err, no. The list excludes those[1] that *are* used (would be
overridden) by the walreceiver module:

----8<--------8<--------8<--------8<--------8<----
static bool
libpqrcv_connect(char *conninfo, XLogRecPtr startpoint)
{
...
         snprintf(conninfo_repl, sizeof(conninfo_repl),
                          "%s dbname=replication replication=true
fallback_application_name=walreceiver",
                          conninfo);
----8<--------8<--------8<--------8<--------8<----

[1] Actually, more than these 3 options are excluded. The deprecated
ones are also excluded.

> 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.

Done at both places.

Also, to adapt to the style of other error messages, now all my fprintf() statements
are prefixed with: "%s: ...", progname.

Best regards,
Zoltán Böszörményi

--
----------------------------------
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/