Re: 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

next version

pg_restore knows --if-exists option now

Regards

Pavel Stehule

2013/12/13 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>

> Hello
>
> I am sending a rebased patch.
>
> Now dump generated with --if-exists option is readable by pg_restore
>
> Regards
>
> Pavel
>

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

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
>
>

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(at)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(dot)stehule(at)gmail(dot)com>wrote:

> 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
>>
>>
>

--
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.

Tomorrow I'll send updated version

Regards

Pavel

2014/1/21 Jeevan Chalke <jeevan(dot)chalke(at)enterprisedb(dot)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(at)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(dot)stehule(at)gmail(dot)com>wrote:
>
>> 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
>>>
>>>
>>
>
>
> --
> 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(dot)chalke(at)enterprisedb(dot)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(at)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(dot)stehule(at)gmail(dot)com>wrote:
>
>> 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
>>>
>>>
>>
>
>
> --
> 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.
>

Sorry second update

fixed dump if-exists of DROP OPERATOR

Regards

Pavel

2014-01-25 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>

> 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(dot)chalke(at)enterprisedb(dot)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(at)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(dot)stehule(at)gmail(dot)com>wrote:
>>
>>> 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
>>>>
>>>>
>>>
>>
>>
>> --
>> 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

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 <jeevan(dot)chalke(at)enterprisedb(dot)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(at)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(dot)stehule(at)gmail(dot)com>wrote:
>
>> 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
>>>
>>>
>>
>
>
> --
> 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 the patch looks good to me. However when I try to restore your own sql
file's dump, I get following errors:

pg_restore: [archiver (db)] could not execute query: ERROR: relation
"public.emp" does not exist
Command was: DROP TRIGGER IF EXISTS emp_insert_trigger ON public.emp;

pg_restore: [archiver (db)] could not execute query: ERROR: schema
"myschema" does not exist
Command was: DROP FUNCTION IF EXISTS myschema.int_to_date(integer);

Is that expected after your patch ?

Also, I didn't quite understand these lines of comments:

/*
* 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.
*/

Will you please rephrase ?

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

2014-01-29 Jeevan Chalke <jeevan(dot)chalke(at)enterprisedb(dot)com>

> Hi Pavel,
>
> Now the patch looks good to me. However when I try to restore your own sql
> file's dump, I get following errors:
>
> pg_restore: [archiver (db)] could not execute query: ERROR: relation
> "public.emp" does not exist
> Command was: DROP TRIGGER IF EXISTS emp_insert_trigger ON public.emp;
>
> pg_restore: [archiver (db)] could not execute query: ERROR: schema
> "myschema" does not exist
> Command was: DROP FUNCTION IF EXISTS myschema.int_to_date(integer);
>
> Is that expected after your patch ?
>

it should be fixed by
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b152c6cd0de1827ba58756e24e18110cf902182acommit

>
> Also, I didn't quite understand these lines of comments:
>
> /*
> * 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.
> */
>
> Will you please rephrase ?
>

I can try it - .

A content of te->desc is usually substring of DROP STATEMENT with one
related exception - CONSTRAINT.
Independent to previous sentence - ALTER TABLE ALTER COLUMN DROP DEFAULT
doesn't support IF EXISTS - and therefore it should not be injected.

Regards

Pavel

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

2014-01-29 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>

>
>
>
> 2014-01-29 Jeevan Chalke <jeevan(dot)chalke(at)enterprisedb(dot)com>
>
> Hi Pavel,
>>
>> Now the patch looks good to me. However when I try to restore your own
>> sql file's dump, I get following errors:
>>
>> pg_restore: [archiver (db)] could not execute query: ERROR: relation
>> "public.emp" does not exist
>> Command was: DROP TRIGGER IF EXISTS emp_insert_trigger ON public.emp;
>>
>> pg_restore: [archiver (db)] could not execute query: ERROR: schema
>> "myschema" does not exist
>> Command was: DROP FUNCTION IF EXISTS myschema.int_to_date(integer);
>>
>> Is that expected after your patch ?
>>
>
> it should be fixed by
> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b152c6cd0de1827ba58756e24e18110cf902182acommit
>
>
>>
>> Also, I didn't quite understand these lines of comments:
>>
>> /*
>> * 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.
>> */
>>
>> Will you please rephrase ?
>>
>
> I can try it - .
>
> A content of te->desc is usually substring of DROP STATEMENT with one
> related exception - CONSTRAINT.
> Independent to previous sentence - ALTER TABLE ALTER COLUMN DROP DEFAULT
> doesn't support IF EXISTS - and therefore it should not be injected.
>

is it ok?

>
> Regards
>
> Pavel
>
>
>>
>> Thanks
>> --
>> Jeevan B Chalke
>> Principal Software Engineer, Product Development
>> EnterpriseDB Corporation
>> The Enterprise PostgreSQL Company
>>
>>
>

Hi Pavel,

it should be fixed by
>> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b152c6cd0de1827ba58756e24e18110cf902182acommit
>>
>
Ok. Good.
Sorry I didn't update my sources. Done now. Thanks

>
>>
>>>
>>> Also, I didn't quite understand these lines of comments:
>>>
>>> /*
>>> * 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.
>>> */
>>>
>>> Will you please rephrase ?
>>>
>>
>> I can try it - .
>>
>> A content of te->desc is usually substring of DROP STATEMENT with one
>> related exception - CONSTRAINT.
>> Independent to previous sentence - ALTER TABLE ALTER COLUMN DROP DEFAULT
>> doesn't support IF EXISTS - and therefore it should not be injected.
>>
>
> is it ok?
>

Fine with me.

Thanks

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

Hello

patch with updated comment

regards

Pavel

2014-01-30 Jeevan Chalke <jeevan(dot)chalke(at)enterprisedb(dot)com>:

> Hi Pavel,
>
> it should be fixed by
>>> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b152c6cd0de1827ba58756e24e18110cf902182acommit
>>>
>>
> Ok. Good.
> Sorry I didn't update my sources. Done now. Thanks
>
>
>>
>>>
>>>>
>>>> Also, I didn't quite understand these lines of comments:
>>>>
>>>> /*
>>>> * 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.
>>>> */
>>>>
>>>> Will you please rephrase ?
>>>>
>>>
>>> I can try it - .
>>>
>>> A content of te->desc is usually substring of DROP STATEMENT with one
>>> related exception - CONSTRAINT.
>>> Independent to previous sentence - ALTER TABLE ALTER COLUMN DROP DEFAULT
>>> doesn't support IF EXISTS - and therefore it should not be injected.
>>>
>>
>> is it ok?
>>
>
> Fine with me.
>
> Thanks
>
> --
> Jeevan B Chalke
> Principal Software Engineer, Product Development
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>
>

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

Hello

All is ok

Thank you

Pavel

2014-01-30 Jeevan Chalke <jeevan(dot)chalke(at)enterprisedb(dot)com>:

> 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
>
>

OK.

Assigned it to committer.

Thanks for the hard work.

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

> Hello
>
> All is ok
>
> Thank you
>
> Pavel
>

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

2014-01-30 Jeevan Chalke <jeevan(dot)chalke(at)enterprisedb(dot)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(dot)stehule(at)gmail(dot)com>wrote:
>
>> Hello
>>
>> All is ok
>>
>> Thank you
>>
>> Pavel
>>
>
> --
> Jeevan B Chalke
> Principal Software Engineer, Product Development
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>
>

Hi Peter,

I am not sure why you getting build unstable due to white-space errors.

Are you referring to these line ?

*11:25:01* src/bin/pg_dump/pg_backup_archiver.c:477: indent with
spaces.*11:25:01* + dropStmt,*11:25:01*
src/bin/pg_dump/pg_backup_archiver.c:478: indent with
spaces.*11:25:10* + buffer,*11:25:14*
src/bin/pg_dump/pg_backup_archiver.c:479: indent with
spaces.*11:25:15* + mark + l);*11:25:15* + echo
unstable

If yes, then in my latest attached patch, these lines are NOT AT ALL there.
I have informed on my comment that I have fixed these in my version of
patch,
but still you got unstable build. NOT sure how. Seems like you are applying
wrong patch.

Will you please let us know what's going wrong ?

Thanks

On Thu, Jan 30, 2014 at 6:56 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>wrote:

>
>
>
> 2014-01-30 Jeevan Chalke <jeevan(dot)chalke(at)enterprisedb(dot)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(dot)stehule(at)gmail(dot)com>wrote:
>>
>>> Hello
>>>
>>> All is ok
>>>
>>> Thank you
>>>
>>> Pavel
>>>
>>
>> --
>> 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.

Jeevan Chalke escribió:

> If yes, then in my latest attached patch, these lines are NOT AT ALL there.
> I have informed on my comment that I have fixed these in my version of
> patch,
> but still you got unstable build. NOT sure how. Seems like you are applying
> wrong patch.
>
> Will you please let us know what's going wrong ?

The commitfest app is not a chat area. When you add new versions of a
patch, please mark them as "patch" (not "comment") and make sure to
provide the message-id of the latest version.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Jeevan Chalke escribió:

I don't understand this code. (Well, it's pg_dump.) Or maybe I do
understand it, and it's not doing what you think it's doing. I mean, in
this part:

> diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
> index 7fc0288..c08a0d3 100644
> --- a/src/bin/pg_dump/pg_backup_archiver.c
> +++ b/src/bin/pg_dump/pg_backup_archiver.c
> @@ -413,8 +413,84 @@ 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 to DROP part when required. */
> + if (ropt->if_exists)

It does *not* modify te->dropStmt, it only sends ahprint() a different
version of what was stored (injected the wanted IF EXISTS clause). If
that is correct, then why are we, in this other part, trying to remove
the IF EXISTS clause?

> @@ -2942,9 +3018,39 @@ _getObjectDescription(PQExpBuffer buf, TocEntry *te, ArchiveHandle *AH)
> 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;
> + 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.
> + */

Maybe I am mistaken and the te->dropStmt already contains the IF EXISTS
bit for some reason; but if so I don't know why that is. Care to
explain?

I also think that _getObjectDescription() becomes overworked after this
patch. I wonder if we should be storing te->objIdentity so that we can
construct the ALTER OWNER command without going to as much trouble as
parsing the DROP command. Is there a way to do that? Maybe we can ask
the server for the object identity, for example. There is a new
function to do that in 9.3 which perhaps we can now use.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hello

2014-02-17 18:10 GMT+01:00 Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>:

> Jeevan Chalke escribió:
>
> I don't understand this code. (Well, it's pg_dump.) Or maybe I do
> understand it, and it's not doing what you think it's doing. I mean, in
> this part:
>
> > diff --git a/src/bin/pg_dump/pg_backup_archiver.c
> b/src/bin/pg_dump/pg_backup_archiver.c
> > index 7fc0288..c08a0d3 100644
> > --- a/src/bin/pg_dump/pg_backup_archiver.c
> > +++ b/src/bin/pg_dump/pg_backup_archiver.c
> > @@ -413,8 +413,84 @@ 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 to DROP
> part when required. */
> > + if (ropt->if_exists)
>
> It does *not* modify te->dropStmt, it only sends ahprint() a different
> version of what was stored (injected the wanted IF EXISTS clause). If
> that is correct, then why are we, in this other part, trying to remove
> the IF EXISTS clause?
>

we should not to modify te->dropStmt, because only in this fragment a DROP
STATEMENT is produced. This additional logic ensures correct syntax for all
variation of DROP.

When I wrote this patch I had a initial problem with understanding relation
between pg_dump and pg_restore. And I pushed IF EXISTS to all related DROP
statements producers. But I was wrong. All the drop statements are reparsed
and transformed and serialized in this fragment. So only this fragment
should be modified. IF EXISTS clause can be injected before, when you read
plain text dump (produced by pg_dump --if-exists) in pg_restore.

>
> > @@ -2942,9 +3018,39 @@ _getObjectDescription(PQExpBuffer buf, TocEntry
> *te, ArchiveHandle *AH)
> > 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;
> > + 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.
> > + */
>
> Maybe I am mistaken and the te->dropStmt already contains the IF EXISTS
> bit for some reason; but if so I don't know why that is. Care to
> explain?
>

pg_restore is available to read plain dump produced by pg_dump --if-exists.
It is way how IF EXISTS can infect te->dropStmt

>
> I also think that _getObjectDescription() becomes overworked after this
> patch. I wonder if we should be storing te->objIdentity so that we can
> construct the ALTER OWNER command without going to as much trouble as
> parsing the DROP command. Is there a way to do that? Maybe we can ask
> the server for the object identity, for example. There is a new
> function to do that in 9.3 which perhaps we can now use.
>
>
do you think a pg_describe_object function?

Probably it is possible, but its significantly much more invasive change,
you should to get objidentity, that is not trivial

Regards

Pavel

> --
> Álvaro Herrera http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>

Pavel Stehule escribió:

> 2014-02-17 18:10 GMT+01:00 Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>:

> > Maybe I am mistaken and the te->dropStmt already contains the IF EXISTS
> > bit for some reason; but if so I don't know why that is. Care to
> > explain?
>
> pg_restore is available to read plain dump produced by pg_dump --if-exists.
> It is way how IF EXISTS can infect te->dropStmt

Makes sense, I guess.

> > I also think that _getObjectDescription() becomes overworked after this
> > patch. I wonder if we should be storing te->objIdentity so that we can
> > construct the ALTER OWNER command without going to as much trouble as
> > parsing the DROP command. Is there a way to do that? Maybe we can ask
> > the server for the object identity, for example. There is a new
> > function to do that in 9.3 which perhaps we can now use.
>
> do you think a pg_describe_object function?
>
> Probably it is possible, but its significantly much more invasive change,
> you should to get objidentity, that is not trivial

I was thinking in pg_identify_object(). It can be given the values used
to construct the CatalogId of each tocEntry.

But yes, it is more invasive.

I'd guess that would be a project related to cleaning up the ALTER
OWNER. What we have now looks like an kludge.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Feb 17, 2014 at 7:43 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>wrote:

> Jeevan Chalke escribió:
>
> > If yes, then in my latest attached patch, these lines are NOT AT ALL
> there.
> > I have informed on my comment that I have fixed these in my version of
> > patch,
> > but still you got unstable build. NOT sure how. Seems like you are
> applying
> > wrong patch.
> >
> > Will you please let us know what's going wrong ?
>
> The commitfest app is not a chat area.

Hmm. Extremely sorry about that.

> When you add new versions of a
> patch, please mark them as "patch" (not "comment") and make sure to
> provide the message-id of the latest version.
>
>
Ohh, I was needed to mark it as patch and NOT comment (with message id).
And since I had marked it as comment, commitfest app was taking previous
patch
and not the latest one.
My bad. Will keep this in mind.

Thanks

> --
> Álvaro Herrera http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>

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

Hello

2014-02-17 22:14 GMT+01:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

> Hello
>
>
> 2014-02-17 18:10 GMT+01:00 Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>:
>
> Jeevan Chalke escribió:
>>
>> I don't understand this code. (Well, it's pg_dump.) Or maybe I do
>> understand it, and it's not doing what you think it's doing. I mean, in
>> this part:
>>
>> > diff --git a/src/bin/pg_dump/pg_backup_archiver.c
>> b/src/bin/pg_dump/pg_backup_archiver.c
>> > index 7fc0288..c08a0d3 100644
>> > --- a/src/bin/pg_dump/pg_backup_archiver.c
>> > +++ b/src/bin/pg_dump/pg_backup_archiver.c
>> > @@ -413,8 +413,84 @@ 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 to
>> DROP part when required. */
>> > + if (ropt->if_exists)
>>
>> It does *not* modify te->dropStmt, it only sends ahprint() a different
>> version of what was stored (injected the wanted IF EXISTS clause). If
>> that is correct, then why are we, in this other part, trying to remove
>> the IF EXISTS clause?
>>
>
> we should not to modify te->dropStmt, because only in this fragment a DROP
> STATEMENT is produced. This additional logic ensures correct syntax for all
> variation of DROP.
>
> When I wrote this patch I had a initial problem with understanding
> relation between pg_dump and pg_restore. And I pushed IF EXISTS to all
> related DROP statements producers. But I was wrong. All the drop statements
> are reparsed and transformed and serialized in this fragment. So only this
> fragment should be modified. IF EXISTS clause can be injected before, when
> you read plain text dump (produced by pg_dump --if-exists) in pg_restore.
>

I was wrong - "IF EXISTS" was there, because I generated DROP IF EXISTS
elsewhere in some very old stages of this patch. It is useless to check it
there now.

>
>
>>
>> > @@ -2942,9 +3018,39 @@ _getObjectDescription(PQExpBuffer buf, TocEntry
>> *te, ArchiveHandle *AH)
>> > 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;
>> > + 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.
>> > + */
>>
>> Maybe I am mistaken and the te->dropStmt already contains the IF EXISTS
>> bit for some reason; but if so I don't know why that is. Care to
>> explain?
>>
>
> pg_restore is available to read plain dump produced by pg_dump
> --if-exists. It is way how IF EXISTS can infect te->dropStmt
>
>
>>
>> I also think that _getObjectDescription() becomes overworked after this
>> patch. I wonder if we should be storing te->objIdentity so that we can
>> construct the ALTER OWNER command without going to as much trouble as
>> parsing the DROP command. Is there a way to do that? Maybe we can ask
>> the server for the object identity, for example. There is a new
>> function to do that in 9.3 which perhaps we can now use.
>>
>>
> do you think a pg_describe_object function?
>
> Probably it is possible, but its significantly much more invasive change,
> you should to get objidentity, that is not trivial
>
> Regards
>

It is irony, so this is death code - it is not used now. So I removed it
from patch.

Reduced, fixed patch attached + used tests

Regards

Pavel

>
> Pavel
>
>
>> --
>> Álvaro Herrera http://www.2ndQuadrant.com/
>> PostgreSQL Development, 24x7 Support, Training & Services
>>
>
>

Pavel Stehule escribió:

> It is irony, so this is death code - it is not used now. So I removed it
> from patch.
>
> Reduced, fixed patch attached + used tests

Nice, thanks.

Here's a new version in which I reworded some comments and docs, and
also inverted the sense of some if/else so that the oneliner case is
first, which makes it more readable IMHO.

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 still find the code to inject IF EXISTS to the DROP commands ugly as
sin. I would propose to stop storing the dropStmt in the archive
anymore; instead just store the object identity, which can later be used
to generate both DROP commands, with or without IF EXISTS, and the ALTER
OWNER commands. However, that's a larger project and I don't think we
need to burden this patch with that.

Another point is that we could argue about whether specifying
--if-exists ought to imply --clean instead of erroring out. There's no
backwards compatibility argument to be had; it's not like existing
scripts are going to suddenly start dropping objects that weren't
dropped before.

Other than the pg_dumpall issue, this patch seems ready.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

2014-02-28 19:31 GMT+01:00 Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>:

> Pavel Stehule escribió:
>
> > It is irony, so this is death code - it is not used now. So I removed it
> > from patch.
> >
> > Reduced, fixed patch attached + used tests
>
> Nice, thanks.
>
> Here's a new version in which I reworded some comments and docs, and
> also inverted the sense of some if/else so that the oneliner case is
> first, which makes it more readable IMHO.
>

ok

thank you

>
> 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'll look on it tomorrow

> I still find the code to inject IF EXISTS to the DROP commands ugly as
> sin. I would propose to stop storing the dropStmt in the archive
> anymore; instead just store the object identity, which can later be used
> to generate both DROP commands, with or without IF EXISTS, and the ALTER
> OWNER commands. However, that's a larger project and I don't think we
> need to burden this patch with that.
>

there are more similar parts - and I am not sure if it is little bit heroic
task.

>
> Another point is that we could argue about whether specifying
> --if-exists ought to imply --clean instead of erroring out. There's no
> backwards compatibility argument to be had; it's not like existing
> scripts are going to suddenly start dropping objects that weren't
> dropped before.
>

It is valid idea. I looked on any other options for and I don't known any
similar implication - so I prefer current implementation (no implication).
It is consistent with any other. I have not strong opinion about it - a
user comfort is against a clarity - but two "clean" option can be confusing
maybe.

Regards

Pavel

>
> Other than the pg_dumpall issue, this patch seems ready.
>
> --
> Álvaro Herrera http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>

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

do you agree?

Pavel

2014-02-28 23:13 GMT+01:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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
>

This patch has redesigned implementation --if-exists for pg_dumpall. Now it
is not propagated to pg_dump, but used on pg_dumpall level.

Regards

Pavel

2014-02-28 23:18 GMT+01:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

>
>
>
> 2014-02-28 23:13 GMT+01:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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
>>
>
>

Pavel Stehule escribió:
> This patch has redesigned implementation --if-exists for pg_dumpall. Now it
> is not propagated to pg_dump, but used on pg_dumpall level.

Seems sane, thanks.

BTW after this patch, I still don't see an error-free output from
restoring a database on top of itself. One problem is plpgsql, which is
now an extension, so pg_dump emits this error message:

ERROR: cannot drop language plpgsql because extension plpgsql requires it
SUGERENCIA: You can drop extension plpgsql instead.

Another problem is that some DROP commands don't work. For instance, if
the public schema in the target database contains objects that haven't
been dropped yet, the DROP command will fail:

ERROR: cannot drop schema public because other objects depend on it
DETALLE: function bt_metap(text) depends on schema public
function bt_page_items(text,integer) depends on schema public
function bt_page_stats(text,integer) depends on schema public
function f() depends on schema public
function get_raw_page(text,integer) depends on schema public
function heap_page_items(bytea) depends on schema public
function locate_tuple_corruption() depends on schema public
function page_header(bytea) depends on schema public
SUGERENCIA: Use DROP ... CASCADE to drop the dependent objects too.

(The way I got this was by using my 8.2 installation, on which I ran the
regression tests; then I dumped the resulting regression database. The
database on which I restored wasn't clean, as it contained unrelated
junk in the public schema.)

Not sure what's the right answer here to this problem, but it cannot be
attributed to this patch anyway.

I'm about to push this, since other than the above problems, this
functionality seems to be working as designed.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

2014-03-03 18:18 GMT+01:00 Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>:

> Pavel Stehule escribió:
> > This patch has redesigned implementation --if-exists for pg_dumpall. Now
> it
> > is not propagated to pg_dump, but used on pg_dumpall level.
>
> Seems sane, thanks.
>
>
> BTW after this patch, I still don't see an error-free output from
> restoring a database on top of itself. One problem is plpgsql, which is
> now an extension, so pg_dump emits this error message:
>
> ERROR: cannot drop language plpgsql because extension plpgsql requires it
> SUGERENCIA: You can drop extension plpgsql instead.
>
>
> Another problem is that some DROP commands don't work. For instance, if
> the public schema in the target database contains objects that haven't
> been dropped yet, the DROP command will fail:
>
> ERROR: cannot drop schema public because other objects depend on it
> DETALLE: function bt_metap(text) depends on schema public
> function bt_page_items(text,integer) depends on schema public
> function bt_page_stats(text,integer) depends on schema public
> function f() depends on schema public
> function get_raw_page(text,integer) depends on schema public
> function heap_page_items(bytea) depends on schema public
> function locate_tuple_corruption() depends on schema public
> function page_header(bytea) depends on schema public
> SUGERENCIA: Use DROP ... CASCADE to drop the dependent objects too.
>
>
> (The way I got this was by using my 8.2 installation, on which I ran the
> regression tests; then I dumped the resulting regression database. The
> database on which I restored wasn't clean, as it contained unrelated
> junk in the public schema.)
>
>
I'll recheck a behave of extensions.

On second hand - usually, preferred way is using a dump related to target
PostgreSQL release

> Not sure what's the right answer here to this problem, but it cannot be
> attributed to this patch anyway.
>
> I'm about to push this, since other than the above problems, this
> functionality seems to be working as designed.
>
>
Thank you very much

Regards

Pavel

> --
> Álvaro Herrera http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>

2014-03-04 8:55 GMT+01:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

>
>
>
> 2014-03-03 18:18 GMT+01:00 Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>:
>
> Pavel Stehule escribió:
>> > This patch has redesigned implementation --if-exists for pg_dumpall.
>> Now it
>> > is not propagated to pg_dump, but used on pg_dumpall level.
>>
>> Seems sane, thanks.
>>
>>
>> BTW after this patch, I still don't see an error-free output from
>> restoring a database on top of itself. One problem is plpgsql, which is
>> now an extension, so pg_dump emits this error message:
>>
>> ERROR: cannot drop language plpgsql because extension plpgsql requires it
>> SUGERENCIA: You can drop extension plpgsql instead.
>>
>>
>> Another problem is that some DROP commands don't work. For instance, if
>> the public schema in the target database contains objects that haven't
>> been dropped yet, the DROP command will fail:
>>
>> ERROR: cannot drop schema public because other objects depend on it
>> DETALLE: function bt_metap(text) depends on schema public
>> function bt_page_items(text,integer) depends on schema public
>> function bt_page_stats(text,integer) depends on schema public
>> function f() depends on schema public
>> function get_raw_page(text,integer) depends on schema public
>> function heap_page_items(bytea) depends on schema public
>> function locate_tuple_corruption() depends on schema public
>> function page_header(bytea) depends on schema public
>> SUGERENCIA: Use DROP ... CASCADE to drop the dependent objects too.
>>
>>
>> (The way I got this was by using my 8.2 installation, on which I ran the
>> regression tests; then I dumped the resulting regression database. The
>> database on which I restored wasn't clean, as it contained unrelated
>> junk in the public schema.)
>>
>>
> I'll recheck a behave of extensions.
>
>
I rechecked extensions and it works - so it can be full quiet when old dump
is imported, but import dump from fresh dumps should to work.

Regards

Pavel

> On second hand - usually, preferred way is using a dump related to target
> PostgreSQL release
>
>
>
>
>> Not sure what's the right answer here to this problem, but it cannot be
>> attributed to this patch anyway.
>>
>> I'm about to push this, since other than the above problems, this
>> functionality seems to be working as designed.
>>
>>
> Thank you very much
>
> Regards
>
> Pavel
>
>
>> --
>> Álvaro Herrera http://www.2ndQuadrant.com/
>> PostgreSQL Development, 24x7 Support, Training & Services
>>
>
>