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