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

Hello

2014-02-17 22:14 GMT+01:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

> 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.
>

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
>>
>
>