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

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