Re: Re: pg_basebackup with -R option and start standby have problems with escaped password

On 17.05.2013 19:03, Boszormenyi Zoltan wrote:
> 2013-05-17 16:05 keltezéssel, Heikki Linnakangas írta:
>> On 18.02.2013 16:35, Boszormenyi Zoltan wrote:
>>> 2013-01-29 11:15 keltezéssel, Magnus Hagander írta:
>>>> On Thu, Jan 24, 2013 at 7:04 AM, Hari Babu <haribabu(dot)kommi(at)huawei(dot)com>
>>>> wrote:
>>>>> On Wed, Jan 23, 2013 11:48 PM, Magnus Hagander wrote:
>>>>>> On Wed, Jan 23, 2013 at 10:18 AM, Hari Babu
>>>>>> <haribabu(dot)kommi(at)huawei(dot)com>
>>>>> wrote:
>>>>>>> Test scenario to reproduce:
>>>>>>> 1. Start the server
>>>>>>> 2. create the user as follows
>>>>>>> ./psql postgres -c "create user user1 superuser login
>>>>>>> password 'use''1'"
>>>>>>>
>>>>>>> 3. Take the backup with -R option as follows.
>>>>>>> ./pg_basebackup -D ../../data1 -R -U user1 -W
>>>>>>>
>>>>>>> The following errors are occurring when the new standby on the
>>>>>>> backup
>>>>>>> database starts.
>>>>>>>
>>>>>>> FATAL: could not connect to the primary server: missing "=" after
>>>>>>> "1'"
>>>>> in
>>>>>>> connection info string
>>>>>> What does the resulting recovery.conf file look like?
>>>>> The recovery.conf which is generated is as follows
>>>>>
>>>>> standby_mode = 'on'
>>>>> primary_conninfo = 'user=''user1'' password=''use''1'' port=''5432'' '
>>>>>
>>>>>
>>>>> I observed the problem is while reading primary_conninfo from the
>>>>> recovery.conf file
>>>>> the function "GUC_scanstr" removes the quotes of the string and also
>>>>> makes
>>>>> the
>>>>> continuos double quote('') as single quote(').
>>>>>
>>>>> By using the same connection string while connecting to primary
>>>>> server the
>>>>> function "conninfo_parse" the escape quotes are not able to parse
>>>>> properly
>>>>> and it is leading
>>>>> to problem.
>>>>>
>>>>> please correct me if any thing wrong in my observation.
>>>> Well, it's clearly broken at least :O
>>>>
>>>> Zoltan, do you have time to look at it? I won't have time until at
>>>> least after FOSDEM, unfortunately.
>>>
>>> I looked at it shortly. What I tried first is adding another pair of
>>> single
>>> quotes manually like this:
>>>
>>> primary_conninfo = 'user=''user1'' password=''use''''1''
>>> host=''192.168.1.2'' port=''5432'' sslmode=''disable''
>>> sslcompression=''1'' '
>>>
>>> But it doesn't solve the problem either, I got:
>>>
>>> FATAL: could not connect to the primary server: missing "=" after "'1'"
>>> in connection info string
>>>
>>> This worked though:
>>>
>>> primary_conninfo = 'user=user1 password=use\'1 host=192.168.1.2
>>> port=5432 sslmode=disable sslcompression=1 '
>>>
>>> When I added an elog() to print the conninfo string in
>>> libpqrcv_connect(),
>>> I saw that the double quotes were properly eliminated by ParseConfigFp()
>>> in the first case.
>>>
>>> So, there is a bug in generating recovery.conf by not double-escaping
>>> the values and another bug in parsing the connection string in libpq
>>> when the parameter value starts with a single-quote character.
>>
>> No, the libpq connection string parser is working as intended. Per the
>> docs on PQconnectdb:
>>
>>> The passed string can be empty to use all default parameters, or it
>>> can contain one or more parameter settings separated by whitespace.
>>> Each parameter setting is in the form keyword = value. Spaces around
>>> the equal sign are optional. To write an empty value, or a value
>>> containing spaces, surround it with single quotes, e.g., keyword = 'a
>>> value'. Single quotes and backslashes within the value must be
>>> escaped with a backslash, i.e., \' and \\.
>>
>> So, the proper way to escape a quote in a libpq connection string is
>> \', not ''. There are two escaping systems layered on top of each
>> other; the recovery.conf parser's, where you use '', and the libpq
>> system, where you use \'. So we need two different escaping functions
>> in pg_basebackup to get this right.
>
> Why is extending the libpq parser to allow doubling the single
> quotes not a good solution? It would be consistent in this regard
> with the recovery.conf parser. From this POV the first patch only
> needs a little change in the libpq docs.

I guess that would work too, but I don't see any meaningful advantage in
doing that.

>> Apart from that, does it bother anyone else that the the
>> primary_conninfo line that pg_basebackup creates is butt-ugly?
>>
>> primary_conninfo = 'user=''heikki'' host=''localhost'' port=''5432''
>> sslmode=''prefer'' sslcompression=''1'' '
>
> Not a single bit. It's machine generated and the code is generic.
> The parser can deal with it.

Oh, ok :-). Well, IMO that's really ugly; the file ought to be
human-readable too.

>> Also, do we really need to include the ssl options when they are the
>> defaults?
>
> I think we were through about this bullet point. IIRC the reasoning and
> the consensus was that the backup and the generated recovery.conf
> should also work on another machine with a possibly different set of
> compilation options for libpq and envvars present on the system.

Ok.

>> I think the attached patch fixes the original test scenario correctly,
>> without changing libpq's quoting rules, and only quotes when
>> necessary. I didn't do anything about the ssl options. Please take a
>> look.
>
> Your patch should work, too.

Thanks, I've applied that.

- Heikki