Re: patch: option --if-exists for pg_dump - Mailing list pgsql-hackers

From Alvaro Herrera
Subject Re: patch: option --if-exists for pg_dump
Date
Msg-id 20140217171016.GQ6342@eldon.alvh.no-ip.org
Whole thread Raw
In response to Re: patch: option --if-exists for pg_dump  (Jeevan Chalke <jeevan.chalke@enterprisedb.com>)
Responses Re: patch: option --if-exists for pg_dump  (Pavel Stehule <pavel.stehule@gmail.com>)
List pgsql-hackers
Jeevan Chalke escribió:

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

> diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
> index 7fc0288..c08a0d3 100644
> --- a/src/bin/pg_dump/pg_backup_archiver.c
> +++ b/src/bin/pg_dump/pg_backup_archiver.c
> @@ -413,8 +413,84 @@ RestoreArchive(Archive *AHX)
>                  /* Select owner and schema as necessary */
>                  _becomeOwner(AH, te);
>                  _selectOutputSchema(AH, te->namespace);
> -                /* Drop it */
> -                ahprintf(AH, "%s", te->dropStmt);
> +
> +                if (*te->dropStmt != '\0')
> +                {
> +                    /* Inject IF EXISTS clause to DROP part when required. */
> +                    if (ropt->if_exists)

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

> @@ -2942,9 +3018,39 @@ _getObjectDescription(PQExpBuffer buf, TocEntry *te, ArchiveHandle *AH)
>          strcmp(type, "OPERATOR CLASS") == 0 ||
>          strcmp(type, "OPERATOR FAMILY") == 0)
>      {
> -        /* Chop "DROP " off the front and make a modifiable copy */
> -        char       *first = pg_strdup(te->dropStmt + 5);
> -        char       *last;
> +        char        *first;
> +        char        *last;
> +
> +        /*
> +         * Object description is based on dropStmt statement which may have
> +         * IF EXISTS clause.  Thus we need to update an offset such that it
> +         * won't be included in the object description.
> +         */

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

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

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



pgsql-hackers by date:

Previous
From: Bruce Momjian
Date:
Subject: Re: Ctrl+C from sh can shut down daemonized PostgreSQL cluster
Next
From: Alvaro Herrera
Date:
Subject: Re: [bug fix] "pg_ctl stop" times out when it should respond quickly