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

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.

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

We can't avoid quoting option values that need it, but that's probably
very rare in practice. I think we should work a bit harder and leave out
the quotes where not necessary. Also, do we really need to include the
ssl options when they are the defaults?

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.

- Heikki