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 Herrera http://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>
+ <term><option>--if-exists</option></term>
+ <listitem>
+ <para>
+ It uses conditional commands (with <literal>IF EXISTS</literal>
+ clause) for cleaning database schema.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>--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>
+ <term><option>--if-exists</option></term>
+ <listitem>
+ <para>
+ It uses conditional commands (with <literal>IF EXISTS</literal>
+ clause) for cleaning database schema.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>--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>
+ <term><option>--if-exists</option></term>
+ <listitem>
+ <para>
+ It uses conditional commands (with <literal>IF EXISTS</literal>
+ clause) for cleaning database schema.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>--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 2b36e45..f36caf2 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -413,8 +413,64 @@ 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)
+ {
+ 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);
+ Assert(mark != NULL);
+
+ *mark = '\0';
+ appendPQExpBuffer(ftStmt, "%s%s IF EXISTS%s",
+ dropStmt, buffer, mark + strlen(buffer));
+ }
+ else
+ appendPQExpBuffer(ftStmt, "%s", dropStmt);
+
+ ahprintf(AH, "%s", ftStmt->data);
+
+ destroyPQExpBuffer(ftStmt);
+ }
+ else
+ ahprintf(AH, "%s", te->dropStmt);
+ }
}
}
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 458a118..88ea556 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -136,6 +136,7 @@ static int binary_upgrade = 0;
static int disable_dollar_quoting = 0;
static int dump_inserts = 0;
static int column_inserts = 0;
+static int if_exists = 0;
static int no_security_labels = 0;
static int no_synchronized_snapshots = 0;
static int no_unlogged_table_data = 0;
@@ -349,6 +350,7 @@ main(int argc, char **argv)
{"disable-dollar-quoting", no_argument, &disable_dollar_quoting, 1},
{"disable-triggers", no_argument, &disable_triggers, 1},
{"exclude-table-data", required_argument, NULL, 4},
+ {"if-exists", no_argument, &if_exists, 1},
{"inserts", no_argument, &dump_inserts, 1},
{"lock-wait-timeout", required_argument, NULL, 2},
{"no-tablespaces", no_argument, &outputNoTablespaces, 1},
@@ -577,6 +579,9 @@ main(int argc, char **argv)
exit_nicely(1);
}
+ if (if_exists && !outputClean)
+ exit_horribly(NULL, "option --if-exists requires -c/--clean option\n");
+
/* Identify archive format to emit */
archiveFormat = parseArchiveFormat(format, &archiveMode);
@@ -809,6 +814,7 @@ main(int argc, char **argv)
ropt->dropSchema = outputClean;
ropt->dataOnly = dataOnly;
ropt->schemaOnly = schemaOnly;
+ ropt->if_exists = if_exists;
ropt->dumpSections = dumpSections;
ropt->aclsSkip = aclsSkip;
ropt->superuser = outputSuperuser;
@@ -890,6 +896,7 @@ help(const char *progname)
printf(_(" --disable-dollar-quoting disable dollar quoting, use SQL standard quoting\n"));
printf(_(" --disable-triggers disable triggers during data-only restore\n"));
printf(_(" --exclude-table-data=TABLE do NOT dump data for the named table(s)\n"));
+ printf(_(" --if-exists use IF EXISTS when dropping objects\n"));
printf(_(" --inserts dump data as INSERT commands, rather than COPY\n"));
printf(_(" --no-security-labels do not dump security label assignments\n"));
printf(_(" --no-synchronized-snapshots do not use synchronized snapshots in parallel jobs\n"));
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 193c1a0..335fdde 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -73,6 +73,7 @@ static int binary_upgrade = 0;
static int column_inserts = 0;
static int disable_dollar_quoting = 0;
static int disable_triggers = 0;
+static int if_exists = 0;
static int inserts = 0;
static int no_tablespaces = 0;
static int use_setsessauth = 0;
@@ -119,6 +120,7 @@ main(int argc, char *argv[])
{"column-inserts", no_argument, &column_inserts, 1},
{"disable-dollar-quoting", no_argument, &disable_dollar_quoting, 1},
{"disable-triggers", no_argument, &disable_triggers, 1},
+ {"if-exists", no_argument, &if_exists, 1},
{"inserts", no_argument, &inserts, 1},
{"lock-wait-timeout", required_argument, NULL, 2},
{"no-tablespaces", no_argument, &no_tablespaces, 1},
@@ -352,6 +354,8 @@ main(int argc, char *argv[])
appendPQExpBufferStr(pgdumpopts, " --disable-dollar-quoting");
if (disable_triggers)
appendPQExpBufferStr(pgdumpopts, " --disable-triggers");
+ if (if_exists)
+ appendPQExpBufferStr(pgdumpopts, " --if-exists");
if (inserts)
appendPQExpBufferStr(pgdumpopts, " --inserts");
if (no_tablespaces)
@@ -564,6 +568,7 @@ help(void)
printf(_(" --column-inserts dump data as INSERT commands with column names\n"));
printf(_(" --disable-dollar-quoting disable dollar quoting, use SQL standard quoting\n"));
printf(_(" --disable-triggers disable triggers during data-only restore\n"));
+ printf(_(" --if-exists use IF EXISTS when dropping objects\n"));
printf(_(" --inserts dump data as INSERT commands, rather than COPY\n"));
printf(_(" --no-security-labels do not dump security label assignments\n"));
printf(_(" --no-tablespaces do not dump tablespace assignments\n"));
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index bb65253..f732027 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -76,6 +76,7 @@ main(int argc, char **argv)
Archive *AH;
char *inputFileSpec;
static int disable_triggers = 0;
+ static int if_exists = 0;
static int no_data_for_failed_tables = 0;
static int outputNoTablespaces = 0;
static int use_setsessauth = 0;
@@ -116,6 +117,7 @@ main(int argc, char **argv)
* the following options don't have an equivalent short option letter
*/
{"disable-triggers", no_argument, &disable_triggers, 1},
+ {"if-exists", no_argument, &if_exists, 1},
{"no-data-for-failed-tables", no_argument, &no_data_for_failed_tables, 1},
{"no-tablespaces", no_argument, &outputNoTablespaces, 1},
{"role", required_argument, NULL, 2},
@@ -342,6 +344,14 @@ main(int argc, char **argv)
opts->use_setsessauth = use_setsessauth;
opts->no_security_labels = no_security_labels;
+ if (if_exists && !opts->dropSchema)
+ {
+ fprintf(stderr, _("%s: option --if-exists requires -c/--clean option\n"),
+ progname);
+ exit_nicely(1);
+ }
+ opts->if_exists = if_exists;
+
if (opts->formatName)
{
switch (opts->formatName[0])
@@ -456,6 +466,7 @@ usage(const char *progname)
printf(_(" -x, --no-privileges skip restoration of access privileges (grant/revoke)\n"));
printf(_(" -1, --single-transaction restore as a single transaction\n"));
printf(_(" --disable-triggers disable triggers during data-only restore\n"));
+ printf(_(" --if-exists use IF EXISTS when dropping objects\n"));
printf(_(" --no-data-for-failed-tables do not restore data of tables that could not be\n"
" created\n"));
printf(_(" --no-security-labels do not restore security labels\n"));
dumptest.sql
Description: application/sql
Makefile
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
