Re: [HACKERS] patch: option --if-exists for pg_dump
2014-03-04 8:55 GMT+01:00 Pavel Stehule pavel.steh...@gmail.com: 2014-03-03 18:18 GMT+01:00 Alvaro Herrera alvhe...@2ndquadrant.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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
Re: [HACKERS] patch: option --if-exists for pg_dump
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch: option --if-exists for pg_dump
2014-03-03 18:18 GMT+01:00 Alvaro Herrera alvhe...@2ndquadrant.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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
Re: [HACKERS] patch: option --if-exists for pg_dump
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml index 40c69f0..1f0d4de 100644 --- a/doc/src/sgml/ref/pg_dump.sgml +++ b/doc/src/sgml/ref/pg_dump.sgml @@ -145,7 +145,8 @@ PostgreSQL documentation para Output commands to clean (drop) database objects prior to outputting the commands for creating them. -(Restore might generate some harmless error messages, if any objects +(Unless option--if-exists/ is also specified, +restore might generate some harmless error messages, if any objects were not present in the destination database.) /para @@ -650,6 +651,17 @@ PostgreSQL documentation /varlistentry varlistentry + termoption--if-exists/option/term + listitem + para +Use conditional commands (i.e. add an literalIF EXISTS/literal +clause) when cleaning database objects. This option is not valid +unless option--clean/ is also specified. + /para + /listitem + /varlistentry + + varlistentry termoption--disable-dollar-quoting//term listitem para diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml index f337939..fcf5f77 100644 --- a/doc/src/sgml/ref/pg_dumpall.sgml +++ b/doc/src/sgml/ref/pg_dumpall.sgml @@ -301,6 +301,17 @@ PostgreSQL documentation /varlistentry varlistentry + termoption--if-exists/option/term + listitem + para +Use conditional commands (i.e. add an literalIF EXISTS/literal +clause) to clean databases and other objects. This option is not valid +unless option--clean/ is also specified. + /para + /listitem + /varlistentry + + varlistentry termoption--inserts/option/term listitem para diff --git a/doc/src/sgml/ref/pg_restore.sgml b/doc/src/sgml/ref/pg_restore.sgml index cd60b25..4bc30ce 100644 --- a/doc/src/sgml/ref/pg_restore.sgml +++ b/doc/src/sgml/ref/pg_restore.sgml @@ -109,7 +109,8 @@ listitem para Clean (drop) database objects before recreating them. -(This might generate some harmless error messages, if any objects +(Unless option--if-exists/ is used, +this might generate some harmless error messages, if any objects were not present in the destination database.) /para /listitem @@ -490,6 +491,17 @@ /varlistentry varlistentry + termoption--if-exists/option/term + listitem + para +Use conditional commands (i.e. add an literalIF EXISTS/literal +clause) when cleaning database objects. This option is not valid +unless option--clean/ is also specified. + /para + /listitem + /varlistentry + + varlistentry termoption--no-data-for-failed-tables/option/term listitem para diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h index 6927968..83f7216 100644 --- a/src/bin/pg_dump/pg_backup.h +++ b/src/bin/pg_dump/pg_backup.h @@ -113,6 +113,7 @@ typedef struct _restoreOptions char *superuser; /* Username to use as superuser */ char *use_role; /* Issue SET ROLE to this */ int dropSchema; + int if_exists; const char *filename; int dataOnly; int schemaOnly; diff --git a/src/bin/pg_dump/pg_backup_archiver.c
Re: [HACKERS] patch: option --if-exists for pg_dump
2014-02-28 19:31 GMT+01:00 Alvaro Herrera alvhe...@2ndquadrant.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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
Re: [HACKERS] patch: option --if-exists for pg_dump
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, --versionoutput 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
Re: [HACKERS] patch: option --if-exists for pg_dump
2014-02-28 23:13 GMT+01:00 Pavel Stehule pavel.steh...@gmail.com: Hi However, I don't think this is behaving sanely in pg_dumpall. AFAICT, pg_dumpall does not pass --clean to pg_dump (in other words it only emits DROP for the global objects, not the objects contained inside databases), so passing --if-exists results in failures. Therefore I think the solution is to not pass --if-exists to pg_dump at all, i.e. keep it internal to pg_dumpall. But maybe I'm missing something. I am looking to pg_dumpall code, and I am inclined to don't pass --if-exists to pg_dump too. -c, --clean for pg_dumpall means drop databases Usage: pg_dumpall [OPTION]... General options: -f, --file=FILENAME output file name -V, --versionoutput 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
Re: [HACKERS] patch: option --if-exists for pg_dump
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.steh...@gmail.com: 2014-02-28 23:13 GMT+01:00 Pavel Stehule pavel.steh...@gmail.com: Hi However, I don't think this is behaving sanely in pg_dumpall. AFAICT, pg_dumpall does not pass --clean to pg_dump (in other words it only emits DROP for the global objects, not the objects contained inside databases), so passing --if-exists results in failures. Therefore I think the solution is to not pass --if-exists to pg_dump at all, i.e. keep it internal to pg_dumpall. But maybe I'm missing something. I am looking to pg_dumpall code, and I am inclined to don't pass --if-exists to pg_dump too. -c, --clean for pg_dumpall means drop databases Usage: pg_dumpall [OPTION]... General options: -f, --file=FILENAME output file name -V, --versionoutput 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 commit 3e2e9baa3b5e708b4014b18cbedd2e6b1fc095a5 Author: root root@nemesis.(none) Date: Fri Feb 28 23:31:32 2014 +0100 pg_dumpall --if-exists fix diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml index 40c69f0..1f0d4de 100644 --- a/doc/src/sgml/ref/pg_dump.sgml +++ b/doc/src/sgml/ref/pg_dump.sgml @@ -145,7 +145,8 @@ PostgreSQL documentation para Output commands to clean (drop) database objects prior to outputting the commands for creating them. -(Restore might generate some harmless error messages, if any objects +(Unless option--if-exists/ is also specified, +restore might generate some harmless error messages, if any objects were not present in the destination database.) /para @@ -650,6 +651,17 @@ PostgreSQL documentation /varlistentry varlistentry + termoption--if-exists/option/term + listitem + para +Use conditional commands (i.e. add an literalIF EXISTS/literal +clause) when cleaning database objects. This option is not valid +unless option--clean/ is also specified. + /para + /listitem + /varlistentry + + varlistentry termoption--disable-dollar-quoting//term listitem para diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml index f337939..fcf5f77 100644 --- a/doc/src/sgml/ref/pg_dumpall.sgml +++ b/doc/src/sgml/ref/pg_dumpall.sgml @@ -301,6 +301,17 @@ PostgreSQL documentation /varlistentry varlistentry + termoption--if-exists/option/term + listitem + para +Use conditional commands (i.e. add an literalIF EXISTS/literal +clause) to clean databases and other objects. This option is not valid +unless option--clean/ is also specified. + /para + /listitem + /varlistentry + + varlistentry termoption--inserts/option/term listitem para diff --git a/doc/src/sgml/ref/pg_restore.sgml b/doc/src/sgml/ref/pg_restore.sgml index cd60b25..4bc30ce 100644 --- a/doc/src/sgml/ref/pg_restore.sgml +++ b/doc/src/sgml/ref/pg_restore.sgml @@ -109,7 +109,8 @@ listitem para Clean (drop) database objects before recreating them. -(This might generate some harmless error messages, if any objects +(Unless option--if-exists/ is used, +this might generate some harmless error messages, if any objects were not present in the destination database.) /para /listitem @@ -490,6 +491,17 @@ /varlistentry varlistentry + termoption--if-exists/option/term + listitem + para +Use conditional commands (i.e. add an literalIF EXISTS/literal +clause) when cleaning database objects. This option is not valid +unless option--clean/ is also specified. + /para + /listitem + /varlistentry + + varlistentry termoption--no-data-for-failed-tables/option/term listitem para diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h index 6927968..83f7216 100644 --- a/src/bin/pg_dump/pg_backup.h +++ b/src/bin/pg_dump/pg_backup.h @@ -113,6 +113,7 @@ typedef struct _restoreOptions char *superuser; /* Username to use as superuser */ char *use_role; /* Issue SET ROLE to this */ int dropSchema; + int if_exists; const char *filename; int dataOnly; int schemaOnly; diff --git
Re: [HACKERS] patch: option --if-exists for pg_dump
Hello
2014-02-17 22:14 GMT+01:00 Pavel Stehule pavel.steh...@gmail.com:
Hello
2014-02-17 18:10 GMT+01:00 Alvaro Herrera alvhe...@2ndquadrant.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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
commit 937830b60920a8fac84cd2adb24c3d1b5280b036
Author: Pavel Stehule pavel.steh...@gooddata.com
Date: Tue Feb 18 14:25:40 2014 +0100
reduced2
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 8d45f24..c39767b 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -650,6 +650,16 @@ PostgreSQL documentation
/varlistentry
varlistentry
+ termoption--if-exists/option/term
+ listitem
+ para
+It uses conditional commands (with literalIF EXISTS/literal
+clause) for cleaning database schema.
+ /para
+ /listitem
+ /varlistentry
+
+ varlistentry
termoption--disable-dollar-quoting//term
listitem
para
diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml
index 5c6a101..ba6583d 100644
--- a/doc/src/sgml/ref/pg_dumpall.sgml
+++
Re: [HACKERS] patch: option --if-exists for pg_dump
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch: option --if-exists for pg_dump
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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch: option --if-exists for pg_dump
Hello
2014-02-17 18:10 GMT+01:00 Alvaro Herrera alvhe...@2ndquadrant.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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
Re: [HACKERS] patch: option --if-exists for pg_dump
Pavel Stehule escribió: 2014-02-17 18:10 GMT+01:00 Alvaro Herrera alvhe...@2ndquadrant.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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch: option --if-exists for pg_dump
On Mon, Feb 17, 2014 at 7:43 PM, Alvaro Herrera alvhe...@2ndquadrant.comwrote: 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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Jeevan B Chalke Principal Software Engineer, Product Development EnterpriseDB Corporation The Enterprise PostgreSQL Company
Re: [HACKERS] patch: option --if-exists for pg_dump
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.steh...@gmail.comwrote: 2014-01-30 Jeevan Chalke jeevan.cha...@enterprisedb.com: OK. Assigned it to committer. Thanks for the hard work. Thank you for review Pavel On Thu, Jan 30, 2014 at 6:16 PM, Pavel Stehule pavel.steh...@gmail.comwrote: 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.
Re: [HACKERS] patch: option --if-exists for pg_dump
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
Re: [HACKERS] patch: option --if-exists for pg_dump
Hello
patch with updated comment
regards
Pavel
2014-01-30 Jeevan Chalke jeevan.cha...@enterprisedb.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
commit e72e3ae9003d2c8eea0e687122a2f31d21674b81
Author: Pavel Stehule pavel.steh...@gooddata.com
Date: Thu Jan 30 11:28:40 2014 +0100
fix comment
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
new file mode 100644
index 8d45f24..c39767b
*** a/doc/src/sgml/ref/pg_dump.sgml
--- b/doc/src/sgml/ref/pg_dump.sgml
*** PostgreSQL documentation
*** 650,655
--- 650,665
/varlistentry
varlistentry
+ termoption--if-exists/option/term
+ listitem
+para
+ It uses conditional commands (with literalIF EXISTS/literal
+ clause) for cleaning database schema.
+/para
+ /listitem
+ /varlistentry
+
+ varlistentry
termoption--disable-dollar-quoting//term
listitem
para
diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml
new file mode 100644
index 5c6a101..ba6583d
*** a/doc/src/sgml/ref/pg_dumpall.sgml
--- b/doc/src/sgml/ref/pg_dumpall.sgml
*** PostgreSQL documentation
*** 301,306
--- 301,316
/varlistentry
varlistentry
+ termoption--if-exists/option/term
+ listitem
+para
+ It uses conditional commands (with literalIF EXISTS/literal
+ clause) for cleaning database schema.
+/para
+ /listitem
+ /varlistentry
+
+ varlistentry
termoption--inserts/option/term
listitem
para
diff --git a/doc/src/sgml/ref/pg_restore.sgml b/doc/src/sgml/ref/pg_restore.sgml
new file mode 100644
index 717da42..55326d5
*** a/doc/src/sgml/ref/pg_restore.sgml
--- b/doc/src/sgml/ref/pg_restore.sgml
***
*** 490,495
--- 490,505
/varlistentry
varlistentry
+ termoption--if-exists/option/term
+ listitem
+para
+ It uses conditional commands (with literalIF EXISTS/literal
+ clause) for cleaning database schema.
+/para
+ /listitem
+ /varlistentry
+
+ varlistentry
termoption--no-data-for-failed-tables/option/term
listitem
para
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
new file mode 100644
index 6927968..83f7216
*** a/src/bin/pg_dump/pg_backup.h
--- b/src/bin/pg_dump/pg_backup.h
*** typedef struct _restoreOptions
*** 113,118
--- 113,119
char *superuser; /* Username to use as superuser */
char *use_role; /* Issue SET ROLE to this */
int dropSchema;
+ int if_exists;
const char *filename;
int dataOnly;
int schemaOnly;
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
new file mode 100644
index 7fc0288..a7061d3
*** a/src/bin/pg_dump/pg_backup_archiver.c
--- b/src/bin/pg_dump/pg_backup_archiver.c
*** RestoreArchive(Archive *AHX)
*** 413,420
/* Select owner and schema as necessary */
_becomeOwner(AH, te);
_selectOutputSchema(AH, te-namespace);
! /* Drop it */
! ahprintf(AH, %s, te-dropStmt);
}
}
--- 413,496
/* Select owner and schema as necessary */
_becomeOwner(AH, te);
_selectOutputSchema(AH, te-namespace);
!
! if (*te-dropStmt != '\0')
! {
! /* Inject IF EXISTS clause to DROP part when it is required. */
! if (ropt-if_exists)
! {
! char buffer[40];
! char *mark;
! char *dropStmt = te-dropStmt;
! PQExpBuffer ftStmt = createPQExpBuffer();
!
! /* ALTER TABLE should be enahnced to ALTER TABLE IF EXISTS */
! if (strncmp(dropStmt, ALTER TABLE, 11) == 0)
! {
! /*
! * Prepare fault tolerant statement, but
! * ensure unique IF EXISTS option.
! */
! if (strncmp(dropStmt + 11, IF EXISTS, 10) != 0)
! {
!
Re: [HACKERS] patch: option --if-exists for pg_dump
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
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 8d45f24..c39767b 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -650,6 +650,16 @@ PostgreSQL documentation
/varlistentry
varlistentry
+ termoption--if-exists/option/term
+ listitem
+ para
+It uses conditional commands (with literalIF EXISTS/literal
+clause) for cleaning database schema.
+ /para
+ /listitem
+ /varlistentry
+
+ varlistentry
termoption--disable-dollar-quoting//term
listitem
para
diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml
index 5c6a101..ba6583d 100644
--- a/doc/src/sgml/ref/pg_dumpall.sgml
+++ b/doc/src/sgml/ref/pg_dumpall.sgml
@@ -301,6 +301,16 @@ PostgreSQL documentation
/varlistentry
varlistentry
+ termoption--if-exists/option/term
+ listitem
+ para
+It uses conditional commands (with literalIF EXISTS/literal
+clause) for cleaning database schema.
+ /para
+ /listitem
+ /varlistentry
+
+ varlistentry
termoption--inserts/option/term
listitem
para
diff --git a/doc/src/sgml/ref/pg_restore.sgml b/doc/src/sgml/ref/pg_restore.sgml
index 717da42..55326d5 100644
--- a/doc/src/sgml/ref/pg_restore.sgml
+++ b/doc/src/sgml/ref/pg_restore.sgml
@@ -490,6 +490,16 @@
/varlistentry
varlistentry
+ termoption--if-exists/option/term
+ listitem
+ para
+It uses conditional commands (with literalIF EXISTS/literal
+clause) for cleaning database schema.
+ /para
+ /listitem
+ /varlistentry
+
+ varlistentry
termoption--no-data-for-failed-tables/option/term
listitem
para
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index 6927968..83f7216 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -113,6 +113,7 @@ typedef struct _restoreOptions
char *superuser; /* Username to use as superuser */
char *use_role; /* Issue SET ROLE to this */
int dropSchema;
+ int if_exists;
const char *filename;
int dataOnly;
int schemaOnly;
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)
+ {
+ char buffer[40];
+ char *mark;
+ char *dropStmt = te-dropStmt;
+ PQExpBuffer ftStmt = createPQExpBuffer();
+
+ /*
+ * Need to inject IF EXISTS clause after ALTER TABLE
+ * part in ALTER TABLE .. DROP statement
+ */
+ if (strncmp(dropStmt, ALTER TABLE, 11) == 0)
+ {
+ /*
+ * Prepare fault tolerant statement, but ensure
+ * unique IF EXISTS option.
+ */
+ if (strncmp(dropStmt + 11, IF EXISTS, 10) != 0)
+ {
+appendPQExpBuffer(ftStmt,
+ ALTER TABLE IF EXISTS);
+dropStmt = dropStmt + 11;
+ }
+ }
+
+ /*
+ * A content of te-desc is usually substring of a DROP
+ * statement with one related exception - CONSTRAINTs.
+ *
+ * Independent to previous sentence, ALTER TABLE ..
+ * ALTER COLUMN .. DROP DEFAULT statement does not
+ * support IF EXISTS clause and therefore it should not
+ * be injected.
+ */
+ if (strcmp(te-desc, DEFAULT) != 0)
+ {
+ if (strcmp(te-desc, CONSTRAINT) == 0 ||
+strcmp(te-desc, CHECK CONSTRAINT) == 0 ||
+strcmp(te-desc, FK CONSTRAINT) == 0)
+strcpy(buffer, DROP CONSTRAINT);
+ else
+snprintf(buffer, sizeof(buffer), DROP %s,
+ te-desc);
+
+ mark = strstr(dropStmt, buffer);
+ }
+ else
+ mark = NULL;
+
+ if (mark != NULL)
+ {
+ size_t l = strlen(buffer);
+
+ /* Inject IF EXISTS clause if not alredy present */
+ if (strncmp(mark + l, IF
Re: [HACKERS] patch: option --if-exists for pg_dump
Hello All is ok Thank you Pavel 2014-01-30 Jeevan Chalke jeevan.cha...@enterprisedb.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
Re: [HACKERS] patch: option --if-exists for pg_dump
OK. Assigned it to committer. Thanks for the hard work. On Thu, Jan 30, 2014 at 6:16 PM, Pavel Stehule pavel.steh...@gmail.comwrote: Hello All is ok Thank you Pavel -- Jeevan B Chalke Principal Software Engineer, Product Development EnterpriseDB Corporation The Enterprise PostgreSQL Company
Re: [HACKERS] patch: option --if-exists for pg_dump
2014-01-30 Jeevan Chalke jeevan.cha...@enterprisedb.com: OK. Assigned it to committer. Thanks for the hard work. Thank you for review Pavel On Thu, Jan 30, 2014 at 6:16 PM, Pavel Stehule pavel.steh...@gmail.comwrote: Hello All is ok Thank you Pavel -- Jeevan B Chalke Principal Software Engineer, Product Development EnterpriseDB Corporation The Enterprise PostgreSQL Company
Re: [HACKERS] patch: option --if-exists for pg_dump
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
Re: [HACKERS] patch: option --if-exists for pg_dump
2014-01-29 Jeevan Chalke jeevan.cha...@enterprisedb.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
Re: [HACKERS] patch: option --if-exists for pg_dump
2014-01-29 Pavel Stehule pavel.steh...@gmail.com 2014-01-29 Jeevan Chalke jeevan.cha...@enterprisedb.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
Re: [HACKERS] patch: option --if-exists for pg_dump
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.cha...@enterprisedb.com
Hi Pavel,
Consider following test scenario:
mydb=# \d emp
Table public.emp
Column | Type | Modifiers
+-+---
empno | integer | not null
deptno | integer |
ename | text|
Indexes:
emp_pkey PRIMARY KEY, btree (empno)
Foreign-key constraints:
emp_deptno_fkey FOREIGN KEY (deptno) REFERENCES dept(deptno)
mydb=# \d dept
Table public.dept
Column | Type | Modifiers
+-+---
deptno | integer | not null
dname | text|
Indexes:
dept_pkey PRIMARY KEY, btree (deptno)
Referenced by:
TABLE emp CONSTRAINT emp_deptno_fkey FOREIGN KEY (deptno)
REFERENCES dept(deptno)
mydb=# \q
jeevan@ubuntu:~/pg_master$ ./install/bin/pg_dump -d mydb --if-exists -c
mydb_ic.dmp
I see following lines in dump which looks certainly wrong:
===
DROP FK CONSTRAINT IF EXISTS ublic.emp DROP CONSTRAINT emp_deptno_fkey;
DROP CONSTRAINT IF EXISTS Y public.emp DROP CONSTRAINT emp_pkey;
DROP CONSTRAINT IF EXISTS Y public.dept DROP CONSTRAINT dept_pkey;
When try to restore, as expected it is throwing an error:
===
psql:mydb_ic.dmp:14: ERROR: syntax error at or near FK
LINE 1: DROP FK CONSTRAINT IF EXISTS ublic.emp DROP CONSTRAINT emp_d...
^
psql:mydb_ic.dmp:15: ERROR: syntax error at or near CONSTRAINT
LINE 1: DROP CONSTRAINT IF EXISTS Y public.emp DROP CONSTRAINT emp_p...
^
psql:mydb_ic.dmp:16: ERROR: syntax error at or near CONSTRAINT
LINE 1: DROP CONSTRAINT IF EXISTS Y public.dept DROP CONSTRAINT dept...
^
Note:
===
Commands which are in form of ALTER TABLE ... DROP are failing.
You need to test each and every object with DROP .. IF EXISTS command.
Better write small test-case with all objects included.
Following logic has flaw:
===
diff --git a/src/bin/pg_dump/pg_backup_archiver.c
b/src/bin/pg_dump/pg_backup_archiver.c
index 7fc0288..0677712 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -413,8 +413,30 @@ RestoreArchive(Archive *AHX)
/* Select owner and schema as necessary */
_becomeOwner(AH, te);
_selectOutputSchema(AH, te-namespace);
-/* Drop it */
-ahprintf(AH, %s, te-dropStmt);
+
+if (*te-dropStmt != '\0')
+{
+/* Inject IF EXISTS clause when it is required. */
+if (ropt-if_exists)
+{
+char buffer[40];
+size_t l;
+
+/* But do it only, when it is not there yet. */
+snprintf(buffer, sizeof(buffer), DROP %s IF
EXISTS,
+ te-desc);
+l = strlen(buffer);
+
+if (strncmp(te-dropStmt, buffer, l) != 0)
+{
+ahprintf(AH, DROP %s IF EXISTS %s,
+te-desc,
+te-dropStmt + l);
+}
+else
+ahprintf(AH, %s, te-dropStmt);
+}
+}
}
}
Also:
===
1.
This is still out of sync.
@@ -348,6 +350,8 @@ main(int argc, char *argv[])
appendPQExpBufferStr(pgdumpopts, --binary-upgrade);
if (column_inserts)
appendPQExpBufferStr(pgdumpopts, --column-inserts);
+if (if_exists)
+appendPQExpBufferStr(pgdumpopts, --if-exists);
if (disable_dollar_quoting)
appendPQExpBufferStr(pgdumpopts, --disable-dollar-quoting);
if (disable_triggers)
2.
Spell check required:
+/* skip first n chars, and create a modifieble copy */
modifieble = modifiable
+/* DROP IF EXISTS pattern is not appliable on dropStmt */
appliable = applicable
3.
+/*
+ * Object description is based on dropStmt statement. But
+ * a drop statements can be enhanced about IF EXISTS clause.
+ * We have to increase a offset in this case, IF EXISTS
+ * should not be included on object description.
+ */
Looks like you need to re-phrase these comments line. Something like:
/*
* Object description is based on dropStmt statement which may have
* IF EXISTS clause. Thus we need to update an offset such that it
* won't be included in the object description.
*/
Or as per
Re: [HACKERS] patch: option --if-exists for pg_dump
Tomorrow I'll send updated version
Regards
Pavel
2014/1/21 Jeevan Chalke jeevan.cha...@enterprisedb.com
Hi Pavel,
Consider following test scenario:
mydb=# \d emp
Table public.emp
Column | Type | Modifiers
+-+---
empno | integer | not null
deptno | integer |
ename | text|
Indexes:
emp_pkey PRIMARY KEY, btree (empno)
Foreign-key constraints:
emp_deptno_fkey FOREIGN KEY (deptno) REFERENCES dept(deptno)
mydb=# \d dept
Table public.dept
Column | Type | Modifiers
+-+---
deptno | integer | not null
dname | text|
Indexes:
dept_pkey PRIMARY KEY, btree (deptno)
Referenced by:
TABLE emp CONSTRAINT emp_deptno_fkey FOREIGN KEY (deptno)
REFERENCES dept(deptno)
mydb=# \q
jeevan@ubuntu:~/pg_master$ ./install/bin/pg_dump -d mydb --if-exists -c
mydb_ic.dmp
I see following lines in dump which looks certainly wrong:
===
DROP FK CONSTRAINT IF EXISTS ublic.emp DROP CONSTRAINT emp_deptno_fkey;
DROP CONSTRAINT IF EXISTS Y public.emp DROP CONSTRAINT emp_pkey;
DROP CONSTRAINT IF EXISTS Y public.dept DROP CONSTRAINT dept_pkey;
When try to restore, as expected it is throwing an error:
===
psql:mydb_ic.dmp:14: ERROR: syntax error at or near FK
LINE 1: DROP FK CONSTRAINT IF EXISTS ublic.emp DROP CONSTRAINT emp_d...
^
psql:mydb_ic.dmp:15: ERROR: syntax error at or near CONSTRAINT
LINE 1: DROP CONSTRAINT IF EXISTS Y public.emp DROP CONSTRAINT emp_p...
^
psql:mydb_ic.dmp:16: ERROR: syntax error at or near CONSTRAINT
LINE 1: DROP CONSTRAINT IF EXISTS Y public.dept DROP CONSTRAINT dept...
^
Note:
===
Commands which are in form of ALTER TABLE ... DROP are failing.
You need to test each and every object with DROP .. IF EXISTS command.
Better write small test-case with all objects included.
Following logic has flaw:
===
diff --git a/src/bin/pg_dump/pg_backup_archiver.c
b/src/bin/pg_dump/pg_backup_archiver.c
index 7fc0288..0677712 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -413,8 +413,30 @@ RestoreArchive(Archive *AHX)
/* Select owner and schema as necessary */
_becomeOwner(AH, te);
_selectOutputSchema(AH, te-namespace);
-/* Drop it */
-ahprintf(AH, %s, te-dropStmt);
+
+if (*te-dropStmt != '\0')
+{
+/* Inject IF EXISTS clause when it is required. */
+if (ropt-if_exists)
+{
+char buffer[40];
+size_t l;
+
+/* But do it only, when it is not there yet. */
+snprintf(buffer, sizeof(buffer), DROP %s IF
EXISTS,
+ te-desc);
+l = strlen(buffer);
+
+if (strncmp(te-dropStmt, buffer, l) != 0)
+{
+ahprintf(AH, DROP %s IF EXISTS %s,
+te-desc,
+te-dropStmt + l);
+}
+else
+ahprintf(AH, %s, te-dropStmt);
+}
+}
}
}
Also:
===
1.
This is still out of sync.
@@ -348,6 +350,8 @@ main(int argc, char *argv[])
appendPQExpBufferStr(pgdumpopts, --binary-upgrade);
if (column_inserts)
appendPQExpBufferStr(pgdumpopts, --column-inserts);
+if (if_exists)
+appendPQExpBufferStr(pgdumpopts, --if-exists);
if (disable_dollar_quoting)
appendPQExpBufferStr(pgdumpopts, --disable-dollar-quoting);
if (disable_triggers)
2.
Spell check required:
+/* skip first n chars, and create a modifieble copy */
modifieble = modifiable
+/* DROP IF EXISTS pattern is not appliable on dropStmt */
appliable = applicable
3.
+/*
+ * Object description is based on dropStmt statement. But
+ * a drop statements can be enhanced about IF EXISTS clause.
+ * We have to increase a offset in this case, IF EXISTS
+ * should not be included on object description.
+ */
Looks like you need to re-phrase these comments line. Something like:
/*
* Object description is based on dropStmt statement which may have
* IF EXISTS clause. Thus we need to update an offset such that it
* won't be included in the object description.
*/
Or as per your choice.
Need to have careful thought on a bug mentioned above.
Thanks
On Fri, Jan 17, 2014 at 6:23 PM, Pavel Stehule pavel.steh...@gmail.comwrote:
Hello
2014/1/16 Jeevan Chalke jeevan.cha...@enterprisedb.com
Hi Pavel,
I have
Re: [HACKERS] 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@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.steh...@gmail.comwrote:
Hello
2014/1/16 Jeevan Chalke jeevan.cha...@enterprisedb.com
Hi Pavel,
I have reviewed the patch and here are my concerns and notes:
POSITIVES:
---
1. Patch applies with some white-space errors.
2. make / make install / make check is smooth. No issues as such.
3. Feature looks good as well.
4. NO
Re: [HACKERS] patch: option --if-exists for pg_dump
Hello
2014/1/16 Jeevan Chalke jeevan.cha...@enterprisedb.com
Hi Pavel,
I have reviewed the patch and here are my concerns and notes:
POSITIVES:
---
1. Patch applies with some white-space errors.
2. make / make install / make check is smooth. No issues as such.
3. Feature looks good as well.
4. NO concern on overall design.
5. Good work.
NEGATIVES:
---
Here are the points which I see in the review and would like you to have
your attention.
1.
+It use conditional commands (with literalIF 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)
boolparallel_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
commit 3fecb7805261e96db764fffcae2bb912538903f5
Author: Pavel Stehule pavel.steh...@gmail.com
Date: Fri
Re: [HACKERS] patch: option --if-exists for pg_dump
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 literalIF 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)
boolparallel_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
Re: [HACKERS] patch: option --if-exists for pg_dump
Hello
next version
pg_restore knows --if-exists option now
Regards
Pavel Stehule
2013/12/13 Pavel Stehule pavel.steh...@gmail.com
Hello
I am sending a rebased patch.
Now dump generated with --if-exists option is readable by pg_restore
Regards
Pavel
commit 7c79aa77ccf53252bac8ce2302a7a0f7a1942e9b
Author: Pavel Stehule pavel.steh...@gooddata.com
Date: Fri Dec 20 20:13:00 2013 +0100
inital
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 8d45f24..18f7346 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -650,6 +650,16 @@ PostgreSQL documentation
/varlistentry
varlistentry
+ termoption--if-exists/option/term
+ listitem
+ para
+It use conditional commands (with literalIF EXISTS/literal
+clause) for cleaning database schema.
+ /para
+ /listitem
+ /varlistentry
+
+ varlistentry
termoption--disable-dollar-quoting//term
listitem
para
diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml
index 5c6a101..670539e 100644
--- a/doc/src/sgml/ref/pg_dumpall.sgml
+++ b/doc/src/sgml/ref/pg_dumpall.sgml
@@ -301,6 +301,16 @@ PostgreSQL documentation
/varlistentry
varlistentry
+ termoption--if-exists/option/term
+ listitem
+ para
+It use conditional commands (with literalIF EXISTS/literal
+clause) for cleaning database schema.
+ /para
+ /listitem
+ /varlistentry
+
+ varlistentry
termoption--inserts/option/term
listitem
para
diff --git a/doc/src/sgml/ref/pg_restore.sgml b/doc/src/sgml/ref/pg_restore.sgml
index 717da42..dd8e990 100644
--- a/doc/src/sgml/ref/pg_restore.sgml
+++ b/doc/src/sgml/ref/pg_restore.sgml
@@ -490,6 +490,16 @@
/varlistentry
varlistentry
+ termoption--if-exists/option/term
+ listitem
+ para
+It use conditional commands (with literalIF EXISTS/literal
+clause) for cleaning database schema.
+ /para
+ /listitem
+ /varlistentry
+
+ varlistentry
termoption--no-data-for-failed-tables/option/term
listitem
para
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index 6927968..83f7216 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -113,6 +113,7 @@ typedef struct _restoreOptions
char *superuser; /* Username to use as superuser */
char *use_role; /* Issue SET ROLE to this */
int dropSchema;
+ int if_exists;
const char *filename;
int dataOnly;
int schemaOnly;
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 63a8009..327ff03 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -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;
@@ -409,8 +411,24 @@ 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')
+{
+ if (ropt-if_exists)
+ {
+ int desc_len = strlen(te-desc);
+
+ /* Clause IF EXISTS can be used yet (by pg_dump) */
+ 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);
+ else
+ ahprintf(AH, %s, te-dropStmt);
+ }
+ else
+ ahprintf(AH, %s, te-dropStmt);
+}
}
}
@@ -2938,9 +2956,35 @@ _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;
+
+ /* try to search IF EXISTS in DROP command */
+ if (strstr(te-dropStmt, IF EXISTS) != NULL)
+ {
+ char buffer[40];
+ size_t l;
+
+ /* IF EXISTS clause should be optional, check it*/
+ snprintf(buffer, sizeof(buffer), DROP %s IF EXISTS, type);
+ l =
[HACKERS] 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
commit 19f21165343b1aaa6cc21d381b84e3c0ce6e3b46
Author: Pavel Stehule pavel.steh...@gooddata.com
Date: Fri Dec 13 14:02:46 2013 +0100
--if-exists option for pg_dump
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 8d45f24..18f7346 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -650,6 +650,16 @@ PostgreSQL documentation
/varlistentry
varlistentry
+ termoption--if-exists/option/term
+ listitem
+ para
+It use conditional commands (with literalIF EXISTS/literal
+clause) for cleaning database schema.
+ /para
+ /listitem
+ /varlistentry
+
+ varlistentry
termoption--disable-dollar-quoting//term
listitem
para
diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml
index 5c6a101..7e5134e 100644
--- a/doc/src/sgml/ref/pg_dumpall.sgml
+++ b/doc/src/sgml/ref/pg_dumpall.sgml
@@ -270,6 +270,16 @@ PostgreSQL documentation
/varlistentry
varlistentry
+ termoption--if-exists/option/term
+ listitem
+ para
+It use conditional commands (with literalIF EXISTS/literal
+clause) for cleaning database schema.
+ /para
+ /listitem
+ /varlistentry
+
+ varlistentry
termoption--disable-dollar-quoting//term
listitem
para
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index 6927968..83f7216 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -113,6 +113,7 @@ typedef struct _restoreOptions
char *superuser; /* Username to use as superuser */
char *use_role; /* Issue SET ROLE to this */
int dropSchema;
+ int if_exists;
const char *filename;
int dataOnly;
int schemaOnly;
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 63a8009..23dc77e 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -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;
@@ -2938,9 +2940,35 @@ _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;
+
+ /* try to search IF EXISTS in DROP command */
+ if (strstr(te-dropStmt, IF EXISTS) != NULL)
+ {
+ char buffer[40];
+ size_t l;
+
+ /* IF EXISTS clause should be optional, check it*/
+ snprintf(buffer, sizeof(buffer), DROP %s IF EXISTS, type);
+ l = strlen(buffer);
+
+ if (strncmp(te-dropStmt, buffer, l) == 0)
+ {
+/* append command type to target type */
+appendPQExpBufferStr(buf, type);
+
+/* skip first n chars, and create a modifieble copy */
+first = pg_strdup(te-dropStmt + l);
+ }
+ else
+/* DROP IF EXISTS pattern is not appliable on dropStmt */
+first = pg_strdup(te-dropStmt + 5);
+ }
+
+ else
+ /* IF EXISTS clause was not used, simple solution */
+ first = pg_strdup(te-dropStmt + 5);
/* point to last character in string */
last = first + strlen(first) - 1;
@@ -2961,7 +2989,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)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 224e8cb..0f602c9 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -140,6 +140,7 @@ static int no_security_labels = 0;
static int no_synchronized_snapshots = 0;
static int no_unlogged_table_data = 0;
static int serializable_deferrable = 0;
+static int if_exists = 0;
static void help(const char *progname);
@@ -346,6 +347,7 @@ main(int argc, char **argv)
