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

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.