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

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.

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