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

On Wed, Oct 10, 2012 at 10:12 PM, Boszormenyi Zoltan <zb(at)cybertec(dot)at> wrote:
> 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(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.
>
>
> 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.

In new patches, when I ran "pg_basebackup -D hoge -c fast -R" on MacOS,
I got the following error message. BTW, I compiled the patched PostgreSQL
with --enable-debug and --enable-cassert options.

pg_basebackup(41751) malloc: *** error for object 0x106001af0: pointer
being freed was not allocated
*** set a breakpoint in malloc_error_break to debug
Abort trap: 6

$ uname -a
Darwin hrk.local 11.4.2 Darwin Kernel Version 11.4.2: Thu Aug 23
16:25:48 PDT 2012; root:xnu-1699.32.7~1/RELEASE_X86_64 x86_64

When tar output format is specified together with -R option, recovery.conf is
not included in base.tar. I think it should.

+ setting up the standby. Since creating a backup for a standalone
+ server with <option>-x</option> or <option>-X</option> and adding
+ a recovery.conf to it wouldn't make a working standby, these options
+ naturally conflict.

Is this right? ISTM that basically we can use pg_basebackup -x to take
a base backup for starting the standby for now. No?

Regards,

--
Fujii Masao