Review: Dumping an Extension's Script

I have done a high level review of the patch, and here are my comments:

1) I don't know if community is really looking for this functionality, I
followed the thread a bit and appraently there is a disagreement over it. I
will not comments on that. Let the big guys decide if they really want this
feature.

2) I think comments are very limited, I would be really great if you can
add thorough comments of whatever new that you have added. Couple of
examples are given below:
i) You have added 'script' variable in 'ExtensionControlFile' structure,
but no comments. All other variables have one, you should follow the suite
and we can understand what's going on at the first look.
ii) What is the need of "require" option?
It would be great if you can go through all the comments and try to add the
missing comments and improve the old ones.
3) There are spelling mistakes in the existing comments, for example:
i) File:pg_dump.c, line:850
ii) File: pg_dump.h line: 136
It would be good idea to run a spell check on your old and new comments.

4) 'if_no_exits' flag of new grammar rule is set to false, even though the
rule states that the extension is created as 'IF NOT EXISTS'.

5) Why 'relocatable' option is added to the new grammar rule of create
extension? In a regular 'create extension' statement, when 'schema' option
is specified, it means that the extension is relocatable. So why is option
needed? If there is a specific reason why 'scheme' option wasn't used, then
please explain.

6) I am unable to figure out why new 'require' option is needed(comments
would have helped)? Is it because when we will dump the extension with -X
flag, it will first dump that referenced extension?
I created the following extension(hstore was already created):
create extension testex with version '1' requires 'hstore' as
$testex$ create type testtype as(a char) $testex$;

I then dumped the extension using -X option, only 'testex' extension was
dumped and not 'hstore'. Apparently by looking at query being used to fetch
extensions in dump, the extensions that it depends on are also being
fetched. But it's not working like that.

7) You have removed an old PG comments in pg_dump.c line:7526 above the
following check:
if (!extinfo->dobj.dump || dataOnly)
return;
I guess it's been done in mistake?

8) In extension.c the following check is added:
/*
* If the extension script is not in the command, then the user is not
* the extension packager and we want to read about relocatable and
* requires in the control file, not in the SQL command.
*/
if (d_relocatable || d_requires)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("parameter \"%s\" is only expected when using CREATE EXTENSION AS",
d_relocatable ? "relocatable" : "requires")));

Is the above code really reachable? If the user has not specified a
script(old create extension syntax is used), then 'd_relocatable' and
'd_requires' will never be true, because the old 'create extension' syntax
doesn't allow it.

9) For every extension being dumped, and then for every object inside that
extension. The code traverses all the fetched items of the database to
compare if that item belong to that specific extension. Because of that
'DumpableObject **dobjs' is traversed fully, every time a single extension
is dumped. This seems to be big big performance hit for a huge database.
And considering if many many extensions are dumped in a huge database, dump
might become very slow. Can you think of any other scheme? Maybe you should
follow PG's design, and fetch the extension objects in dumpExtension() and
dump them.

10) pg_dumpall is not handled at all. User can't use -X switch if he wants
to dump all the databases with extensions.

Regards,
Ali Dar