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

Hello

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

> Jeevan Chalke escribió:
>
> I don't understand this code. (Well, it's pg_dump.) Or maybe I do
> understand it, and it's not doing what you think it's doing. I mean, in
> this part:
>
> > diff --git a/src/bin/pg_dump/pg_backup_archiver.c
> b/src/bin/pg_dump/pg_backup_archiver.c
> > index 7fc0288..c08a0d3 100644
> > --- a/src/bin/pg_dump/pg_backup_archiver.c
> > +++ b/src/bin/pg_dump/pg_backup_archiver.c
> > @@ -413,8 +413,84 @@ RestoreArchive(Archive *AHX)
> > /* Select owner and schema as necessary */
> > _becomeOwner(AH, te);
> > _selectOutputSchema(AH, te->namespace);
> > - /* Drop it */
> > - ahprintf(AH, "%s", te->dropStmt);
> > +
> > + if (*te->dropStmt != '\0')
> > + {
> > + /* Inject IF EXISTS clause to DROP
> part when required. */
> > + if (ropt->if_exists)
>
> It does *not* modify te->dropStmt, it only sends ahprint() a different
> version of what was stored (injected the wanted IF EXISTS clause). If
> that is correct, then why are we, in this other part, trying to remove
> the IF EXISTS clause?
>

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

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

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

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

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

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

Regards

Pavel

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