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.