Re: patch: option --if-exists for pg_dump - Mailing list pgsql-hackers
| From | Pavel Stehule |
|---|---|
| Subject | Re: patch: option --if-exists for pg_dump |
| Date | |
| Msg-id | CAFj8pRCiAS2nm+o4O=+GD17Pbo3JC0HohOiKi1oSfbneeQxa8w@mail.gmail.com Whole thread Raw |
| In response to | Re: patch: option --if-exists for pg_dump (Alvaro Herrera <alvherre@2ndquadrant.com>) |
| Responses |
Re: patch: option --if-exists for pg_dump
Re: patch: option --if-exists for pg_dump |
| List | pgsql-hackers |
Hello
2014-02-17 18:10 GMT+01:00 Alvaro Herrera <alvherre@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 */> + /* Inject IF EXISTS clause to DROP part when required. */
> _becomeOwner(AH, te);
> _selectOutputSchema(AH, te->namespace);
> - /* Drop it */
> - ahprintf(AH, "%s", te->dropStmt);
> +
> + if (*te->dropStmt != '\0')
> + {
> + 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
pgsql-hackers by date: