Re: patch: option --if-exists for pg_dump - Mailing list pgsql-hackers
From | Jeevan Chalke |
---|---|
Subject | Re: patch: option --if-exists for pg_dump |
Date | |
Msg-id | CAM2+6=U-TGoN-wk0Yf-S3DM46Lj4fbv_v15Qtq9KUhj2zOxGMg@mail.gmail.com Whole thread Raw |
In response to | Re: patch: option --if-exists for pg_dump (Pavel Stehule <pavel.stehule@gmail.com>) |
Responses |
Re: patch: option --if-exists for pg_dump
|
List | pgsql-hackers |
<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>
pgsql-hackers by date: