Обсуждение: patch: option --if-exists for pg_dump

Hello

I am sending a rebased patch.

Now dump generated with --if-exists option is readable by pg_restore

Regards

Pavel

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

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.

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.

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.

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.

Hi Pavel,

Now patch looks good to me and I think it is in good shape to pass it on to
the committer as well.

However, I have
- Tweaked few comments

- Removed white-space errors

- Fixed typos

- Fixed indentation

Attached patch with my changes. However entire design and code logic is
untouched.

Please have a quick look and pass it on to committor if you have no issues OR
ask me to assign it to the committor, NO issues either way.

Feel free to reject my changes if you didn't like them.

Thanks

--

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

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

OK.

Assigned it to committer.

Thanks for the hard work.

Thank you for review

Pavel

 

On Thu, Jan 30, 2014 at 6:16 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hello

All is ok

Thank you

Pavel

 
--

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

2014-02-28 23:13 GMT+01:00 Pavel Stehule <pavel.stehule@gmail.com>:

Hi

However, I don't think this is behaving sanely in pg_dumpall.  AFAICT,
pg_dumpall does not pass --clean to pg_dump (in other words it only
emits DROP for the global objects, not the objects contained inside
databases), so passing --if-exists results in failures.  Therefore I
think the solution is to not pass --if-exists to pg_dump at all, i.e.
keep it internal to pg_dumpall.  But maybe I'm missing something.

I am looking to pg_dumpall code, and I am inclined to don't pass --if-exists to pg_dump too.

-c, --clean for pg_dumpall means "drop databases"

<<<<<

Usage:
  pg_dumpall [OPTION]...

General options:
  -f, --file=FILENAME          output file name
  -V, --version                output version information, then exit
  --lock-wait-timeout=TIMEOUT  fail after waiting TIMEOUT for a table lock
  -?, --help                   show this help, then exit

Options controlling the output content:
  -a, --data-only              dump only the data, not the schema
  -c, --clean                  clean (drop) databases before recreating
>>>>>

so --if-exists should to mean

DROP DATABASE IF EXISTS dbname

+ DROP ROLE and DROP TABLESPACE

 

do you agree?

Pavel