Re: patch: option --if-exists for pg_dump - Mailing list pgsql-hackers

Hi Pavel,

I have reviewed the patch and here are my concerns and notes:

POSITIVES:
---
1. Patch applies with some white-space errors.
2. make / make install / make check is smooth. No issues as such.
3. Feature looks good as well.
4. NO concern on overall design.
5. Good work.


NEGATIVES:
---

Here are the points which I see in the review and would like you to have your attention.

1.
+        It use conditional commands (with <literal>IF EXISTS</literal>

Grammar mistakes. use => uses

2.
@@ -55,7 +55,8 @@ static ArchiveHandle *_allocAH(const char *FileSpec, const ArchiveFormat fmt,
     const int compression, ArchiveMode mode, SetupWorkerPtr setupWorkerPtr);
 static void _getObjectDescription(PQExpBuffer buf, TocEntry *te,
                      ArchiveHandle *AH);
-static void _printTocEntry(ArchiveHandle *AH, TocEntry *te, RestoreOptions *ropt, bool isData, bool acl_pass);
+static void _printTocEntry(ArchiveHandle *AH, TocEntry *te, RestoreOptions *ropt,
+                                bool isData, bool acl_pass);
 static char *replace_line_endings(const char *str);
 static void _doSetFixedOutputState(ArchiveHandle *AH);
 static void _doSetSessionAuth(ArchiveHandle *AH, const char *user);
@@ -234,6 +235,7 @@ RestoreArchive(Archive *AHX)
    bool        parallel_mode;
    TocEntry   *te;
    OutputContext sav;
+  

    AH->stage = STAGE_INITIALIZING;

@@ -2961,7 +3005,8 @@ _getObjectDescription(PQExpBuffer buf, TocEntry *te, ArchiveHandle *AH)
 }

 static void
-_printTocEntry(ArchiveHandle *AH, TocEntry *te, RestoreOptions *ropt, bool isData, bool acl_pass)
+_printTocEntry(ArchiveHandle *AH, TocEntry *te, RestoreOptions *ropt, bool isData,
+                                     bool acl_pass)
 {
    /* ACLs are dumped only during acl pass */
    if (acl_pass)

Above changes are NOT at all related to the patch. Please remove them even
though they are clean-up like changes. Don't mix them with actual changes.

3.
+                       if (strncmp(te->dropStmt + desc_len + 5, " IF EXISTS", 9) != 0)

" IF EXISTS" has 10 characters NOT 9.

4.
+                       if (strncmp(te->dropStmt + desc_len + 5, " IF EXISTS", 9) != 0)
+                           ahprintf(AH, "DROP %s IF EXISTS %s",
+                                             te->desc,
+                                         te->dropStmt + 6 + desc_len);

Here you have used strncmp, starting at te->dropStmt + X,
where X = desc_len + 5. While adding back you used X = 6 + desc_len.
First time you used 5 as you added space in comparison but for adding back we
want past space location and thus you have used 6. That's correct, but little
bit confusing. Why not you simply used
+       if (strstr(te->dropStmt, "IF EXISTS") != NULL)
to check whether drop statement has "IF EXISTS" or not like you did at some
other place. This will remove my concern 3 and above confusion as well.
What you think ?

5.
+       }
+
+       else

Extra line before else part. Please remove it for consistency.

6.
+   printf(_("  --if-exists                  use IF EXISTS when dropping objects\n"));  (pg_dump)
+   printf(_("  --if-exists                  don't report error if cleaned object doesn't exist\n"));  (pg_dumpall)
+   printf(_("  --if-exists                  use IF EXISTS when dropping objects\n"));  (pg_restore)

Please have same message for all three.

7.
    printf(_("  --binary-upgrade             for use by upgrade utilities only\n"));
    printf(_("  --column-inserts             dump data as INSERT commands with column names\n"));
+   printf(_("  --if-exists                  don't report error if cleaned object doesn't exist\n"));
    printf(_("  --disable-dollar-quoting     disable dollar quoting, use SQL standard quoting\n"));
    printf(_("  --disable-triggers           disable triggers during data-only restore\n"));

Please maintain order like pg_dump and pg_restore. Also at variable declaration
and at options parsing mechanism.

8.
+   if (if_exists && !outputClean)
+       exit_horribly(NULL, "option --if-exists requires -c/--clean option\n");

Are we really want to exit when -c is not provided ? Can't we simply ignore
--if-exists in that case (or with emitting a warning) ?

Marking "Waiting on author".

Thanks

--

Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company