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

2012-10-14 22:26 keltezéssel, Boszormenyi Zoltan írta:
> 2012-10-14 22:23 keltezéssel, Boszormenyi Zoltan írta:
>> Hi,
>>
>> 2012-10-14 18:41 keltezéssel, Boszormenyi Zoltan írta:
>>> 2012-10-14 18:02 keltezéssel, Fujii Masao írta:
>>>> Thanks for updating the patch!
>>>>
>>>> On Sun, Oct 14, 2012 at 8:41 PM, Boszormenyi Zoltan <zb(at)cybertec(dot)at> wrote:
>>>>> Backing up a standby server without -R preserves the original recovery.conf
>>>>> of the
>>>>> standby, it points to the standby's source server.
>>>>>
>>>>> Backing up a standby server with -R overwrites the original recovery.conf
>>>>> with the new
>>>>> one pointing to the standby instead of the standby's source server. Without
>>>>> -Ft, it is
>>>>> obvious. With -Ft, there are two recovery.conf files in the tar file and
>>>>> upon extracting it,
>>>>> the last written one (the one generated via -R) overwrites the original.
>>>> The tar file is always extracted such way in all platform which PostgreSQL
>>>> supports? I'm just concerned about that some tool in some platform might
>>>> prefer the original recovery.conf when extracting tar file. If the spec of tar
>>>> format specifies such behavior (i.e., the last written file of the same name
>>>> is always preferred), it's OK.
>>>
>>> Since tar is a sequential archive format, I think this is the behaviour of
>>> every tar extractor. But I will look at adding code to skip the original
>>> recovery.conf if it exists in the tar file.
>>>
>>>> I found the bug that recovery.conf is included in the tar file of the tablespace
>>>> instead of base.tar, when there are tablespaces in the server.
>>>
>>> You are right, I am looking into this. But I don't know how it got there,
>>> I check for (rownum == 0 && writerecoveryconf) and rownum == 0
>>> supposedly means that it's the base.tar. Looking again.
>>
>> I made a mistake in the previous check, rownum is not reliable.
>> The tablespaces are sent first and base backup as the last.
>> Now recovery.conf is written into base.tar.
>>
>>>> Maybe this is nitpicky problem,,,, but...
>>>> If port number is not explicitly specified in pg_basebackup, the port
>>>> number is not
>>>> included to primary_conninfo in recovery.conf which is created during
>>>> the backup.
>>>> That is, the standby server using such recovery.conf tries to connect
>>>> to the default
>>>> port number because the port number is not supplied in primary_conninfo. This
>>>> assumes that the default port number is the same between the master and standby.
>>>> But this is not true. The default port number can be changed in --with-pgport
>>>> configure option, so the default port number might be different
>>>> between the master
>>>> and standby. To avoid this uncertainty, pg_basebackup -R should always include
>>>> the port number in primary_conninfo?
>>>
>>> I think you are right. But, I wouldn't restrict it only to the port setting.
>>> Any of the values that are set and equal to the compiled-in default,
>>> it should be written into recovery.conf.
>>
>> Now all values that are set (even those being equal to the compiled-in default)
>> are put into recovery.conf.
>>
>>>> When the password is required to connect to the server, pg_basebackup -R
>>>> always writes the password setting into primary_conninfo in recovery.conf.
>>>> But if the password is supplied from .pgpass, ISTM that the password setting
>>>> doesn't need to be written into primary_conninfo. Right?
>>>
>>> How can you deduce it from the PQconninfoOption structure?
>>>
>>> Also, if the machine you take the base backup on is different
>>> from the one where you actually use the backup on, it can be
>>> different not only in the --with-pgport compilation option but
>>> in the presence of .pgpass or the PGPASSWORD envvar, too.
>>> The administrator is there for a reason or there is no .pgpass
>>> or PGPASSWORD at all.
>>>
>>>> + The password written into recovery.conf is not escaped even if special
>>>> + characters appear in it. The administrator must review recovery.conf
>>>> + to ensure proper escaping.
>>>>
>>>> Is it difficult to make pg_basebackup escape the special characters in the
>>>> password? It's better if we can remove this restriction.
>>>
>>> It's not difficult. What other characters need to be escaped besides single quotes?
>>
>> All written values are escaped.
>>
>> Other changes: the recovery.conf in base.tar is correctly skipped if it exists
>> and -R is given. The new recovery.conf is written with padding to round up to
>> 512, the TAR chunk size.
>
> Also, the check for conflict between -R and -x/-X is now removed.

Really the last one, for today at least. The buffer for recovery.conf is freed
in both the -Fp and -Ft cases.

>
>>
>> The PQconninfo patch is also attached but didn't change since the last mail.
>>
>>>
>>>> I've not reviewed PQconninfo patch yet. Will review.
>>>
>>> Thanks in advance.
>
> 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/