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

Hello

I fixed all described issues. There was a more mistakes, that I fixed - main mistake was in work with te->desc variable.

You propose a regress tests for pg_dump? I searched in mailing lists and there was some proposals about it. I am not against, but I have to do some research and better be this as separate patch.

This patch requires b152c6cd0de1827ba58756e24e18110cf902182a commit.

Regards

Pavel

2014-01-21 Jeevan Chalke <jeevan.chalke@enterprisedb.com>

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 <pavel.stehule@gmail.com> wrote:

Hello

2014/1/16 Jeevan Chalke <jeevan.chalke@enterprisedb.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

--

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.