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

On Sun, 01 Jul 2012 13:02:17 +0200 Boszormenyi Zoltan wrote:

>attached is a patch that does $SUBJECT.

>It's a usability enhancement, to take a backup, write

>a minimalistic recovery.conf and start the streaming

>standby in one go.

>Comments?

[Review of Patch]

Basic stuff:
----------------------
- Patch applies OK
- Compiles cleanly with no warnings

What it does:
-------------------------
The pg_basebackup tool does the backup of Cluster from server to the
specified location.
This new functionality will also writes the recovery.conf in the database
directory and start the standby server based on options passed to
pg_basebackup.

Usability
----------------
For usability aspect, I am not very sure how many users would like to start
the standby server using basebackup.
According to me it can be useful for users who have automated scripts to
start server after backup can use this feature.

Feature Testing:
-----------------------------

1. Test pg_basebackup with option -R to check that the recovery.conf file is
written to data directory.
--recovery.conf file is created in data directory.

2. Test pg_basebackup with option -R to check that the recovery.conf file is
not able to create because of disk full.
--Error is given as recovery.conf file is not able to create.

3. Test pg_basebackup with option -S to check the standby server start on
the same/different machine.
--Starting standby server is success in if pg_basebackup is taken in
different machine.

4. Test pg_basebackup with both options -S and -R to check the standby
server start on same/different machine.
--Starting standby server is success in if pg_basebackup is taken in
different machine.

5. Test pg_basebackup with option -S including -h, -U, -p, -w and -W to
check the standy server start
and verify the recovery.conf which is created in data directory.
--Except password, rest of the primary connection info parameters are
working fine.

6. Test pg_basebackup with conflict options (-x or -X and -R or -S).
--Error is given when the conflict options are provided to
pg_basebackup.

7. Test pg_basebackup with option -S where pg_ctl/postmaster binaries are
not present in the path.
--Error is given as not able to execute.

8. Test pg_basebackup with option -S by connecting to a standby server.
--standby server is started successfully when pg_basebackup is made from
a standby server also.

Code Review:
----------------------------
1. In function WriteRecoveryConf(), un-initialized filename is used.
due to which it can print junk for below line in code
printf("add password to primary_conninfo in %s if needed\n", filename);

2. In function WriteRecoveryConf(), in below code if fopen fails (due to
disk full or any other file related error) it will print the error and
exits.
So now it can be confusing to user, in respect to can he consider
backup as successfull and proceed. IMO, either error meesage or
documentation
can suggest the for such error user can proceed with backup to write
his own recovery.conf and start the standby.

+ cf = fopen(filename, "w");
+ if (cf == NULL)
+ {
+ fprintf(stderr, _("cannot create %s"), filename);
+ exit(1);
+ }

3. In function main,
instead of the following code it can be changed in two different ways,

if (startstandby)
writerecoveryconf = true;

change1:
case 'R':
writerecoveryconf = true;
break;
case 'S':
startstandby = true;
writerecoveryconf = true;
break;

change2:
case 'S':
startstandby = true;
case 'R':
writerecoveryconf = true;
break;

4. The password is not written to primary_conninfo even if the dbpassword is
present because of this reason
connecting to the primary is failing because of authentication failure.

5. write the function header for the newly added functions.

6. execvp function is deprecated beginning in Visual C++ 2005. which is used
to fork the pg_ctl process.
http://msdn.microsoft.com/en-us/library/ms235414.aspx

7. In StartStandby function, it is better to free the memory allocated for
path (path = xstrdup(command);)

Defects:
-------------
1. If the pg_basebackup is used in the same machine with the option of -S,
the standby server start
will fail as the port already in use because of using the same
postgresql.conf.

2. If the hot_standby=off in master conf file, the same is copied to
subscriber and starts the server. with that
no client connections are allowed to the server.

Documentation issues:
--------------------------------
1. For -R option,
Conflicts with <option>--xlog
I think it is better to explain the reason of conflict.

2. For -S option,
"Start the standby database server. Implies -R option."
I think the above can be improved to
"Writes the recovery.conf and start the standby database server. There
is no need for user to specify -R option explicitly."
or something similar.

With Regards,

Amit Kapila.