Hello Second update - I reduced patch by removing not necessary changes.
Attached tests and Makefile Now --if-exists option is fully consistent with -c option With some free time I plan to enhance test script about more object types - now It contains almost all usual types. Regards Pavel 2014-01-21 Jeevan Chalke <[email protected]> > Hi Pavel, > > Consider following test scenario: > > mydb=# \d emp > Table "public.emp" > Column | Type | Modifiers > --------+---------+----------- > empno | integer | not null > deptno | integer | > ename | text | > Indexes: > "emp_pkey" PRIMARY KEY, btree (empno) > Foreign-key constraints: > "emp_deptno_fkey" FOREIGN KEY (deptno) REFERENCES dept(deptno) > > mydb=# \d dept > Table "public.dept" > Column | Type | Modifiers > --------+---------+----------- > deptno | integer | not null > dname | text | > Indexes: > "dept_pkey" PRIMARY KEY, btree (deptno) > Referenced by: > TABLE "emp" CONSTRAINT "emp_deptno_fkey" FOREIGN KEY (deptno) > REFERENCES dept(deptno) > > mydb=# \q > jeevan@ubuntu:~/pg_master$ ./install/bin/pg_dump -d mydb --if-exists -c > > mydb_ic.dmp > > I see following lines in dump which looks certainly wrong: > === > > DROP FK CONSTRAINT IF EXISTS ublic.emp DROP CONSTRAINT emp_deptno_fkey; > DROP CONSTRAINT IF EXISTS Y public.emp DROP CONSTRAINT emp_pkey; > DROP CONSTRAINT IF EXISTS Y public.dept DROP CONSTRAINT dept_pkey; > > When try to restore, as expected it is throwing an error: > === > > psql:mydb_ic.dmp:14: ERROR: syntax error at or near "FK" > LINE 1: DROP FK CONSTRAINT IF EXISTS ublic.emp DROP CONSTRAINT emp_d... > ^ > psql:mydb_ic.dmp:15: ERROR: syntax error at or near "CONSTRAINT" > LINE 1: DROP CONSTRAINT IF EXISTS Y public.emp DROP CONSTRAINT emp_p... > ^ > psql:mydb_ic.dmp:16: ERROR: syntax error at or near "CONSTRAINT" > LINE 1: DROP CONSTRAINT IF EXISTS Y public.dept DROP CONSTRAINT dept... > ^ > > Note: > === > Commands which are in form of ALTER TABLE ... DROP are failing. > You need to test each and every object with DROP .. IF EXISTS command. > Better write small test-case with all objects included. > > Following logic has flaw: > === > diff --git a/src/bin/pg_dump/pg_backup_archiver.c > b/src/bin/pg_dump/pg_backup_archiver.c > index 7fc0288..0677712 100644 > --- a/src/bin/pg_dump/pg_backup_archiver.c > +++ b/src/bin/pg_dump/pg_backup_archiver.c > @@ -413,8 +413,30 @@ RestoreArchive(Archive *AHX) > /* Select owner and schema as necessary */ > _becomeOwner(AH, te); > _selectOutputSchema(AH, te->namespace); > - /* Drop it */ > - ahprintf(AH, "%s", te->dropStmt); > + > + if (*te->dropStmt != '\0') > + { > + /* Inject IF EXISTS clause when it is required. */ > + if (ropt->if_exists) > + { > + char buffer[40]; > + size_t l; > + > + /* But do it only, when it is not there yet. */ > + snprintf(buffer, sizeof(buffer), "DROP %s IF > EXISTS", > + te->desc); > + l = strlen(buffer); > + > + if (strncmp(te->dropStmt, buffer, l) != 0) > + { > > + ahprintf(AH, "DROP %s IF EXISTS %s", > + te->desc, > + te->dropStmt + l); > + } > + else > + ahprintf(AH, "%s", te->dropStmt); > + } > + } > } > } > > > Also: > === > > 1. > This is still out of sync. > > @@ -348,6 +350,8 @@ main(int argc, char *argv[]) > appendPQExpBufferStr(pgdumpopts, " --binary-upgrade"); > if (column_inserts) > appendPQExpBufferStr(pgdumpopts, " --column-inserts"); > + if (if_exists) > + appendPQExpBufferStr(pgdumpopts, " --if-exists"); > if (disable_dollar_quoting) > appendPQExpBufferStr(pgdumpopts, " --disable-dollar-quoting"); > if (disable_triggers) > > 2. > Spell check required: > > + /* skip first n chars, and create a modifieble copy */ > > modifieble => modifiable > > + /* DROP IF EXISTS pattern is not appliable on dropStmt */ > > appliable => applicable > > 3. > > + /* > + * Object description is based on dropStmt statement. But > + * a drop statements can be enhanced about IF EXISTS clause. > + * We have to increase a offset in this case, "IF EXISTS" > + * should not be included on object description. > + */ > > Looks like you need to re-phrase these comments line. Something like: > > /* > * Object description is based on dropStmt statement which may have > * IF EXISTS clause. Thus we need to update an offset such that it > * won't be included in the object description. > */ > Or as per your choice. > > > Need to have careful thought on a bug mentioned above. > > Thanks > > > On Fri, Jan 17, 2014 at 6:23 PM, Pavel Stehule <[email protected]>wrote: > >> Hello >> >> >> 2014/1/16 Jeevan Chalke <[email protected]> >> >>> 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 >>> >>> >> > > > -- > Jeevan B Chalke > Principal Software Engineer, Product Development > EnterpriseDB Corporation > The Enterprise PostgreSQL Company > > Phone: +91 20 30589500 > > Website: www.enterprisedb.com > EnterpriseDB Blog: http://blogs.enterprisedb.com/ > Follow us on Twitter: http://www.twitter.com/enterprisedb > > This e-mail message (and any attachment) is intended for the use of the > individual or entity to whom it is addressed. This message contains > information from EnterpriseDB Corporation that may be privileged, > confidential, or exempt from disclosure under applicable law. If you are > not the intended recipient or authorized to receive this for the intended > recipient, any use, dissemination, distribution, retention, archiving, or > copying of this communication is strictly prohibited. If you have received > this e-mail in error, please notify the sender immediately by reply e-mail > and delete this message. >
*** a/doc/src/sgml/ref/pg_dump.sgml
--- b/doc/src/sgml/ref/pg_dump.sgml
***************
*** 650,655 ****
--- 650,665 ----
</varlistentry>
<varlistentry>
+ <term><option>--if-exists</option></term>
+ <listitem>
+ <para>
+ It uses conditional commands (with <literal>IF EXISTS</literal>
+ clause) for cleaning database schema.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>--disable-dollar-quoting</></term>
<listitem>
<para>
*** a/doc/src/sgml/ref/pg_dumpall.sgml
--- b/doc/src/sgml/ref/pg_dumpall.sgml
***************
*** 301,306 ****
--- 301,316 ----
</varlistentry>
<varlistentry>
+ <term><option>--if-exists</option></term>
+ <listitem>
+ <para>
+ It uses conditional commands (with <literal>IF EXISTS</literal>
+ clause) for cleaning database schema.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>--inserts</option></term>
<listitem>
<para>
*** a/doc/src/sgml/ref/pg_restore.sgml
--- b/doc/src/sgml/ref/pg_restore.sgml
***************
*** 490,495 ****
--- 490,505 ----
</varlistentry>
<varlistentry>
+ <term><option>--if-exists</option></term>
+ <listitem>
+ <para>
+ It uses conditional commands (with <literal>IF EXISTS</literal>
+ clause) for cleaning database schema.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>--no-data-for-failed-tables</option></term>
<listitem>
<para>
*** a/src/bin/pg_dump/pg_backup.h
--- b/src/bin/pg_dump/pg_backup.h
***************
*** 113,118 ****
--- 113,119 ----
char *superuser; /* Username to use as superuser */
char *use_role; /* Issue SET ROLE to this */
int dropSchema;
+ int if_exists;
const char *filename;
int dataOnly;
int schemaOnly;
*** a/src/bin/pg_dump/pg_backup_archiver.c
--- b/src/bin/pg_dump/pg_backup_archiver.c
***************
*** 413,420 ****
/* Select owner and schema as necessary */
_becomeOwner(AH, te);
_selectOutputSchema(AH, te->namespace);
! /* Drop it */
! ahprintf(AH, "%s", te->dropStmt);
}
}
--- 413,493 ----
/* Select owner and schema as necessary */
_becomeOwner(AH, te);
_selectOutputSchema(AH, te->namespace);
!
! if (*te->dropStmt != '\0')
! {
! /* Inject IF EXISTS clause to DROP part when it is required. */
! if (ropt->if_exists)
! {
! char buffer[40];
! char *mark;
! char *dropStmt = te->dropStmt;
! PQExpBuffer ftStmt = createPQExpBuffer();
!
! /* ALTER TABLE should be enahnced to ALTER TABLE IF EXISTS */
! if (strncmp(dropStmt, "ALTER TABLE", 11) == 0)
! {
! /*
! * Prepare fault tolerant statement, but
! * ensure unique IF EXISTS option.
! */
! if (strncmp(dropStmt + 11, " IF EXISTS", 10) != 0)
! {
! appendPQExpBuffer(ftStmt, "ALTER TABLE IF EXISTS");
! dropStmt = dropStmt + 11;
! }
! }
!
! /*
! * Descriptor string (te-desc) should not be same as object
! * specifier for DROP STATEMENT. The DROP DEFAULT has not
! * IF EXISTS clause - has not sense.
! */
! if (strcmp(te->desc, "DEFAULT") != 0)
! {
! if (strcmp(te->desc, "CONSTRAINT") == 0 ||
! strcmp(te->desc, "CHECK CONSTRAINT") == 0 ||
! strcmp(te->desc, "FK CONSTRAINT") == 0)
! strcpy(buffer, "DROP CONSTRAINT");
! else
! snprintf(buffer, sizeof(buffer), "DROP %s",
! te->desc);
!
! mark = strstr(dropStmt, buffer);
! }
! else
! mark = NULL;
!
! if (mark != NULL)
! {
! size_t l = strlen(buffer);
!
! /*
! * Insert IF EXISTS clause when it is not
! * used yet.
! */
! if (strncmp(mark + l, " IF EXISTS", 10) != 0)
! {
! *mark = '\0';
!
! appendPQExpBuffer(ftStmt, "%s%s IF EXISTS%s",
! dropStmt,
! buffer,
! mark + l);
! }
! else
! appendPQExpBuffer(ftStmt, "%s", dropStmt);
! }
! else
! appendPQExpBuffer(ftStmt, "%s", dropStmt);
!
! ahprintf(AH, "%s", ftStmt->data);
!
! destroyPQExpBuffer(ftStmt);
! }
! else
! ahprintf(AH, "%s", te->dropStmt);
! }
}
}
***************
*** 2942,2950 ****
strcmp(type, "OPERATOR CLASS") == 0 ||
strcmp(type, "OPERATOR FAMILY") == 0)
{
! /* Chop "DROP " off the front and make a modifiable copy */
! char *first = pg_strdup(te->dropStmt + 5);
! char *last;
/* point to last character in string */
last = first + strlen(first) - 1;
--- 3015,3053 ----
strcmp(type, "OPERATOR CLASS") == 0 ||
strcmp(type, "OPERATOR FAMILY") == 0)
{
! char *first;
! char *last;
!
! /*
! * Object description is based on dropStmt statement which may have
! * IF EXISTS clause. Thus we need to update an offset such that it
! * won't be included in the object description.
! */
! if (strstr(te->dropStmt, "IF EXISTS") != NULL)
! {
! char buffer[40];
! size_t l;
!
! /* Be sure, so IF EXISTS is used as DROP stmt option. */
! snprintf(buffer, sizeof(buffer), "DROP %s IF EXISTS", type);
!
! l = strlen(buffer);
!
! if (strncmp(te->dropStmt, buffer, l) == 0)
! {
! /* append command type to target type */
! appendPQExpBufferStr(buf, type);
!
! /* skip first n chars, and create a modifiable copy */
! first = pg_strdup(te->dropStmt + l);
! }
! else
! /* DROP IF EXISTS pattern is not applicable on dropStmt */
! first = pg_strdup(te->dropStmt + 5);
! }
! else
! /* IF EXISTS clause was not used, simple solution */
! first = pg_strdup(te->dropStmt + 5);
/* point to last character in string */
last = first + strlen(first) - 1;
*** a/src/bin/pg_dump/pg_dump.c
--- b/src/bin/pg_dump/pg_dump.c
***************
*** 136,141 ****
--- 136,142 ----
static int disable_dollar_quoting = 0;
static int dump_inserts = 0;
static int column_inserts = 0;
+ static int if_exists = 0;
static int no_security_labels = 0;
static int no_synchronized_snapshots = 0;
static int no_unlogged_table_data = 0;
***************
*** 349,354 ****
--- 350,356 ----
{"disable-dollar-quoting", no_argument, &disable_dollar_quoting, 1},
{"disable-triggers", no_argument, &disable_triggers, 1},
{"exclude-table-data", required_argument, NULL, 4},
+ {"if-exists", no_argument, &if_exists, 1},
{"inserts", no_argument, &dump_inserts, 1},
{"lock-wait-timeout", required_argument, NULL, 2},
{"no-tablespaces", no_argument, &outputNoTablespaces, 1},
***************
*** 577,582 ****
--- 579,587 ----
exit_nicely(1);
}
+ if (if_exists && !outputClean)
+ exit_horribly(NULL, "option --if-exists requires -c/--clean option\n");
+
/* Identify archive format to emit */
archiveFormat = parseArchiveFormat(format, &archiveMode);
***************
*** 809,814 ****
--- 814,820 ----
ropt->dropSchema = outputClean;
ropt->dataOnly = dataOnly;
ropt->schemaOnly = schemaOnly;
+ ropt->if_exists = if_exists;
ropt->dumpSections = dumpSections;
ropt->aclsSkip = aclsSkip;
ropt->superuser = outputSuperuser;
***************
*** 890,895 ****
--- 896,902 ----
printf(_(" --disable-dollar-quoting disable dollar quoting, use SQL standard quoting\n"));
printf(_(" --disable-triggers disable triggers during data-only restore\n"));
printf(_(" --exclude-table-data=TABLE do NOT dump data for the named table(s)\n"));
+ printf(_(" --if-exists use IF EXISTS when dropping objects\n"));
printf(_(" --inserts dump data as INSERT commands, rather than COPY\n"));
printf(_(" --no-security-labels do not dump security label assignments\n"));
printf(_(" --no-synchronized-snapshots do not use synchronized snapshots in parallel jobs\n"));
*** a/src/bin/pg_dump/pg_dumpall.c
--- b/src/bin/pg_dump/pg_dumpall.c
***************
*** 73,78 ****
--- 73,79 ----
static int column_inserts = 0;
static int disable_dollar_quoting = 0;
static int disable_triggers = 0;
+ static int if_exists = 0;
static int inserts = 0;
static int no_tablespaces = 0;
static int use_setsessauth = 0;
***************
*** 119,124 ****
--- 120,126 ----
{"column-inserts", no_argument, &column_inserts, 1},
{"disable-dollar-quoting", no_argument, &disable_dollar_quoting, 1},
{"disable-triggers", no_argument, &disable_triggers, 1},
+ {"if-exists", no_argument, &if_exists, 1},
{"inserts", no_argument, &inserts, 1},
{"lock-wait-timeout", required_argument, NULL, 2},
{"no-tablespaces", no_argument, &no_tablespaces, 1},
***************
*** 352,357 ****
--- 354,361 ----
appendPQExpBufferStr(pgdumpopts, " --disable-dollar-quoting");
if (disable_triggers)
appendPQExpBufferStr(pgdumpopts, " --disable-triggers");
+ if (if_exists)
+ appendPQExpBufferStr(pgdumpopts, " --if-exists");
if (inserts)
appendPQExpBufferStr(pgdumpopts, " --inserts");
if (no_tablespaces)
***************
*** 564,569 ****
--- 568,574 ----
printf(_(" --column-inserts dump data as INSERT commands with column names\n"));
printf(_(" --disable-dollar-quoting disable dollar quoting, use SQL standard quoting\n"));
printf(_(" --disable-triggers disable triggers during data-only restore\n"));
+ printf(_(" --if-exists use IF EXISTS when dropping objects\n"));
printf(_(" --inserts dump data as INSERT commands, rather than COPY\n"));
printf(_(" --no-security-labels do not dump security label assignments\n"));
printf(_(" --no-tablespaces do not dump tablespace assignments\n"));
*** a/src/bin/pg_dump/pg_restore.c
--- b/src/bin/pg_dump/pg_restore.c
***************
*** 76,81 ****
--- 76,82 ----
Archive *AH;
char *inputFileSpec;
static int disable_triggers = 0;
+ static int if_exists = 0;
static int no_data_for_failed_tables = 0;
static int outputNoTablespaces = 0;
static int use_setsessauth = 0;
***************
*** 116,121 ****
--- 117,123 ----
* the following options don't have an equivalent short option letter
*/
{"disable-triggers", no_argument, &disable_triggers, 1},
+ {"if-exists", no_argument, &if_exists, 1},
{"no-data-for-failed-tables", no_argument, &no_data_for_failed_tables, 1},
{"no-tablespaces", no_argument, &outputNoTablespaces, 1},
{"role", required_argument, NULL, 2},
***************
*** 342,347 ****
--- 344,357 ----
opts->use_setsessauth = use_setsessauth;
opts->no_security_labels = no_security_labels;
+ if (if_exists && !opts->dropSchema)
+ {
+ fprintf(stderr, _("%s: option --if-exists requires -c/--clean option\n"),
+ progname);
+ exit_nicely(1);
+ }
+ opts->if_exists = if_exists;
+
if (opts->formatName)
{
switch (opts->formatName[0])
***************
*** 456,461 ****
--- 466,472 ----
printf(_(" -x, --no-privileges skip restoration of access privileges (grant/revoke)\n"));
printf(_(" -1, --single-transaction restore as a single transaction\n"));
printf(_(" --disable-triggers disable triggers during data-only restore\n"));
+ printf(_(" --if-exists use IF EXISTS when dropping objects\n"));
printf(_(" --no-data-for-failed-tables do not restore data of tables that could not be\n"
" created\n"));
printf(_(" --no-security-labels do not restore security labels\n"));
dumptest.sql
Description: application/sql
Makefile
Description: Binary data
-- Sent via pgsql-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
