Re: patch: option --if-exists for pg_dump

<div dir="ltr">Hi Pavel,<br /><br />I have reviewed the patch and here are my concerns and notes:<br /><br
/>POSITIVES:<br/>---<br />1. Patch applies with some white-space errors.<br />2. make / make install / make check is
smooth.No issues as such.<br /> 3. Feature looks good as well.<br />4. NO concern on overall design.<br />5. Good
work.<br/><br /><br />NEGATIVES:<br />---<br /><br />Here are the points which I see in the review and would like you
tohave your attention.<br /><br />1.<br /><font size="1"><span style="font-family:courier new,monospace">+        It
useconditional commands (with <literal>IF EXISTS</literal></span></font><br /><br />Grammar mistakes. use
=>uses<br /><br />2.<br /><font size="1"><span style="font-family:courier new,monospace">@@ -55,7 +55,8 @@ static
ArchiveHandle*_allocAH(const char *FileSpec, const ArchiveFormat fmt,<br />      const int compression, ArchiveMode
mode,SetupWorkerPtr setupWorkerPtr);<br /> static void _getObjectDescription(PQExpBuffer buf, TocEntry *te,<br
/>                     ArchiveHandle *AH);<br />-static void _printTocEntry(ArchiveHandle *AH, TocEntry *te,
RestoreOptions*ropt, bool isData, bool acl_pass);<br /> +static void _printTocEntry(ArchiveHandle *AH, TocEntry *te,
RestoreOptions*ropt,<br />+                                bool isData, bool acl_pass);<br /> static char
*replace_line_endings(constchar *str);<br /> static void _doSetFixedOutputState(ArchiveHandle *AH);<br />  static void
_doSetSessionAuth(ArchiveHandle*AH, const char *user);<br />@@ -234,6 +235,7 @@ RestoreArchive(Archive *AHX)<br />   
bool       parallel_mode;<br />    TocEntry   *te;<br />    OutputContext sav;<br />+   <br /><br />    AH->stage =
STAGE_INITIALIZING;<br/><br />@@ -2961,7 +3005,8 @@ _getObjectDescription(PQExpBuffer buf, TocEntry *te, ArchiveHandle
*AH)<br/> }<br /><br /> static void<br />-_printTocEntry(ArchiveHandle *AH, TocEntry *te, RestoreOptions *ropt, bool
isData,bool acl_pass)<br /> +_printTocEntry(ArchiveHandle *AH, TocEntry *te, RestoreOptions *ropt, bool isData,<br
/>+                                    bool acl_pass)<br /> {<br />    /* ACLs are dumped only during acl pass */<br
/>   if (acl_pass)<br /></span></font><br /> Above changes are NOT at all related to the patch. Please remove them
even<br/>though they are clean-up like changes. Don't mix them with actual changes.<br /><br />3.<br /><font
size="1"><spanstyle="font-family:courier new,monospace">+                       if (strncmp(te->dropStmt + desc_len
+5, " IF EXISTS", 9) != 0)<br /></span></font><br />" IF EXISTS" has 10 characters NOT 9.<br /><br />4.<br /><font
size="1"><spanstyle="font-family:courier new,monospace">+                       if (strncmp(te->dropStmt + desc_len
+5, " IF EXISTS", 9) != 0)<br /> +                           ahprintf(AH, "DROP %s IF EXISTS %s",<br
/>+                                            te->desc,<br />+                                        
te->dropStmt+ 6 + desc_len);<br /></span></font><br /> Here you have used strncmp, starting at te->dropStmt +
X,<br/>where X = desc_len + 5. While adding back you used X = 6 + desc_len.<br />First time you used 5 as you added
spacein comparison but for adding back we<br />want past space location and thus you have used 6. That's correct, but
little<br/> bit confusing. Why not you simply used<br /><font size="1"><span style="font-family:courier
new,monospace">+      if (strstr(te->dropStmt, "IF EXISTS") != NULL)<br /></span></font>to check whether drop
statementhas "IF EXISTS" or not like you did at some<br /> other place. This will remove my concern 3 and above
confusionas well.<br />What you think ?<br /><br />5.<br /><font size="1"><span style="font-family:courier
new,monospace">+      }<br />+<br />+       else<br /></span></font><br /> Extra line before else part. Please remove
itfor consistency.<br /><br />6.<br /><font size="1"><span style="font-family:courier new,monospace">+   printf(_(" 
--if-exists                 use IF EXISTS when dropping objects\n"));  (pg_dump)<br />+   printf(_(" 
--if-exists                 don't report error if cleaned object doesn't exist\n"));  (pg_dumpall)<br /> +  
printf(_(" --if-exists                  use IF EXISTS when dropping objects\n"));  (pg_restore)<br /></span></font><br
/>Pleasehave same message for all three.<br /><br />7.<br /><font size="1"><span style="font-family:courier
new,monospace">   printf(_("  --binary-upgrade             for use by upgrade utilities only\n"));<br />    
printf(_(" --column-inserts             dump data as INSERT commands with column names\n"));<br />+   printf(_(" 
--if-exists                 don't report error if cleaned object doesn't exist\n"));<br />     printf(_(" 
--disable-dollar-quoting    disable dollar quoting, use SQL standard quoting\n"));<br />    printf(_(" 
--disable-triggers          disable triggers during data-only restore\n"));<br /></span></font><br /> Please maintain
orderlike pg_dump and pg_restore. Also at variable declaration<br />and at options parsing mechanism.<br /><br />8.<br
/><fontsize="1"><span style="font-family:courier new,monospace">+   if (if_exists && !outputClean)<br />
+      exit_horribly(NULL, "option --if-exists requires -c/--clean option\n");<br /></span></font><br />Are we really
wantto exit when -c is not provided ? Can't we simply ignore<br />--if-exists in that case (or with emitting a warning)
?<br/><br />Marking "Waiting on author".<br /><br />Thanks<br /><div class="gmail_extra"><br clear="all" /><br />-- <br
/><divdir="ltr">Jeevan B Chalke<br />Principal Software Engineer, Product Development<br />EnterpriseDB Corporation<br
/>The Enterprise PostgreSQL Company<br /><br /></div></div></div>