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

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