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

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.

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/