[PATCH] Make pg_basebackup configure and start standby [Review] - Mailing list pgsql-hackers

<div class="WordSection1"><p class="MsoNormal"><span lang="EN" style="font-size:12.0pt;font-family:"Times New
Roman","serif";color:black">On</span><span lang="EN" style="font-size:12.0pt;font-family:"Times New
Roman","serif";color:black">Sun,01 Jul 2012 13:02:17 +0200 Boszormenyi Zoltan wrote:</span><span lang="EN"
style="font-size:12.0pt;font-family:"TimesNew Roman","serif";color:black"></span><p class="MsoNormal"><span lang="EN"
style="font-size:12.0pt;font-family:"TimesNew Roman","serif";color:black">>attached is a patch that does
$SUBJECT.</span><pclass="MsoNormal"><span lang="EN" style="font-size:12.0pt;font-family:"Times New
Roman","serif";color:black"> </span><pclass="MsoNormal"><span lang="EN" style="font-size:12.0pt;font-family:"Times New
Roman","serif";color:black">>It'sa usability enhancement, to take a backup, write</span><p class="MsoNormal"><span
lang="EN"style="font-size:12.0pt;font-family:"Times New Roman","serif";color:black">>a minimalistic recovery.conf
andstart the streaming</span><p class="MsoNormal"><span lang="EN" style="font-size:12.0pt;font-family:"Times New
Roman","serif";color:black">>standbyin one go.</span><p class="MsoNormal"><span lang="EN"
style="font-size:12.0pt;font-family:"TimesNew Roman","serif";color:black"> </span><p class="MsoNormal"><span lang="EN"
style="font-size:12.0pt;font-family:"TimesNew Roman","serif";color:black">>Comments?</span><p
class="MsoNormal"><spanlang="EN" style="font-size:12.0pt;font-family:"Times New Roman","serif";color:black"> </span><p
class="MsoNormal"><b><spanlang="EN" style="font-size:12.0pt;font-family:"Times New Roman","serif";color:black">[Review
ofPatch]</span></b><p class="MsoNormal"><span lang="EN" style="font-size:12.0pt;font-family:"Times New
Roman","serif";color:black"> </span><pclass="MsoNormal"><b><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Basicstuff:</span></b><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">----------------------</span><br/><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">-Patch applies OK </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">-Compiles cleanly with no warnings</span><br /><br /><b><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Whatit does:</span></b><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">-------------------------</span><br/><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Thepg_basebackup tool does the backup of Cluster from server
tothe specified location. </span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">This new
functionalitywill also writes the recovery.conf in the database directory and start the standby server based on options
passedto pg_basebackup.</span><br /><br /><b><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Usability</span></b><br/><b><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">----------------</span></b><br/><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Forusability aspect, I am not very sure how many users would
liketo start the standby server using basebackup. </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Accordingto me it can be useful for users who have automated
scriptsto start server after backup can use this feature.</span><br /><br /><b><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">FeatureTesting:</span></b><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">-----------------------------</span><br/><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">1.Test pg_basebackup with option -R to check that the
recovery.conffile is written to data directory.</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   --recovery.conf file is created in data
directory.</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">    </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">2.Test pg_basebackup with option -R to check that the
recovery.conffile is not able to create because of disk full.</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   --Error is given as recovery.conf file is not able to
create.</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">     </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">3.Test pg_basebackup with option -S to check the standby
serverstart on the same/different machine.</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   --Starting standby server is success in if pg_basebackup
istaken in different machine.</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">   
</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">4. Test pg_basebackup with both options -S
and-R to check the standby server start on same/different machine.</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   --Starting standby server is success in if pg_basebackup
istaken in different machine.</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">   
</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">5. Test pg_basebackup with option -S
including-h, -U, -p, -w and -W to check the standy server start </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">  and verify the recovery.conf which is created in data
directory.</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">    --Except password, rest of
theprimary connection info parameters are working fine.</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">6.Test pg_basebackup with conflict options (-x or -X and -R
or-S).</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">    --Error is given when the
conflictoptions are provided to pg_basebackup.</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">7.Test pg_basebackup with option -S where pg_ctl/postmaster
binariesare not present in the path.</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">   
--Erroris given as not able to execute.</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">   
</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">8. Test pg_basebackup with option -S by
connectingto a standby server.</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">   
--standbyserver is started successfully when pg_basebackup is made from a standby server also.</span><br /><br
/><b><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">Code Review:</span></b><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">----------------------------</span><br/><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">1.In function WriteRecoveryConf(), un-initialized filename is
used.</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">    due to which it can print junk for
belowline in code</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">   printf("add password
toprimary_conninfo in %s if needed\n", filename);</span><br /><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">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.</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">    So now it can be confusing to user, in respect to can he
considerbackup as successfull and proceed. IMO, either error meesage or documentation</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">    can suggest the for such error user can proceed with
backupto write his own recovery.conf and start the standby.</span><br /><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">+       cf = fopen(filename, "w");</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">+       if (cf == NULL)</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">+       {</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">+               fprintf(stderr, _("cannot create %s"),
filename);</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">+              
 exit(1);</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">+        }</span><br /><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">3.In function main,</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   instead of the following code it can be changed in two
differentways,</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">    </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">           if (startstandby)</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">                   writerecoveryconf = true;</span><br
/><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">    </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   change1:</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">       case 'R':</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">                       writerecoveryconf = true;</span><br
/><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">                        break;</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">               case 'S':</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">                       startstandby = true;</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">                       writerecoveryconf = true;</span><br
/><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">                        break;</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">                       </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   change2:</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">               case 'S':</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">                       startstandby = true;</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">       case 'R':</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">                       writerecoveryconf = true;</span><br
/><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">                        break;</span><br /><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">4.The password is not written to primary_conninfo even if the
dbpasswordis present because of this reason</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">  connecting to the primary is failing because of
authenticationfailure.</span><br /><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">5. write the
functionheader for the newly added functions.</span><br /><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">6.execvp function is deprecated beginning in Visual C++ 2005.
whichis used to fork the pg_ctl process.</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif""> 
 <a
href="http://msdn.microsoft.com/en-us/library/ms235414.aspx">http://msdn.microsoft.com/en-us/library/ms235414.aspx</a></span><br
/><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">7. In StartStandby function, it is better to
freethe memory allocated for path (path = xstrdup(command);) </span><br /><br /><br /><b><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Defects:</span></b><br/><b><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">-------------</span></b><br/><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">1.If the pg_basebackup is used in the same machine with the
optionof -S, the standby server start</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif""> 
 willfail as the port already in use because of using the same postgresql.conf.</span><br /><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">2.If the hot_standby=off in master conf file, the same is
copiedto subscriber and starts the server. with that</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">  no client connections are allowed to the server.</span><br
/><br/><b><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">Documentation issues:</span></b><br
/><b><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">--------------------------------</span></b><br
/><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">1. For -R option, </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Conflictswith <option>--xlog</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Ithink it is better to explain the reason of
conflict.</span><br/><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">2. For -S option, </span><br
/><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">    "Start the standby database server. Implies -R
option."</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">    I think the above can be
improvedto </span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">    "Writes the recovery.conf
andstart the standby database server. There is no need for user to specify -R option explicitly."</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   or something similar.</span><br /><br /><span lang="EN"
style="font-size:12.0pt;font-family:"TimesNew Roman","serif";color:black"></span><p class="MsoNormal"><span lang="EN"
style="font-size:12.0pt;font-family:"TimesNew Roman","serif";color:black"> </span><p class="MsoNormal"><span lang="EN"
style="font-size:12.0pt;font-family:"TimesNew Roman","serif";color:black">With Regards,</span><p
class="MsoNormal"><spanlang="EN" style="font-size:12.0pt;font-family:"Times New Roman","serif";color:black">Amit
Kapila.</span><pclass="MsoNormal"><span lang="EN" style="font-size:13.0pt;font-family:"Courier
New";color:black"> </span><pclass="MsoNormal"> </div>