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

Hello

2014/1/16 Jeevan Chalke <jeevan(dot)chalke(at)enterprisedb(dot)com>

> 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.
>
>
I fixed a previous issue, see a attachment please

> 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) ?
>
>
This behave is based on a talk related to proposal of this feature - and I
am thinking, this behave is little bit safer - ignoring requested
functionality is not what I like. And a error message is simple and clean
in this case - is not difficult to use it and it is not difficult to fix
missing option for user

Regards

Pavel

> Marking "Waiting on author".
>
> Thanks
>
>
> --
> Jeevan B Chalke
> Principal Software Engineer, Product Development
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>
>