Re: Extensions, this time with a patch

Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> writes:
> * extension.v9.patch.gz seems to contain other changes in the repo.
> Did you use old master to get the diff?

Sorry about that, I've been using git diff master.. but this time, I did
merge pgmaster (upstream) into extension directly rather than into my
local master then merge into extension. So that the patch contained all
the accumulated diffs between previous git pull and this one.

Please find attached a clean v9 patch, that is easy to get from the git
repository too.

http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=shortlog;h=refs/heads/extension

> * Typo in doc/xfunc.sgml. They are to be "replaceable" probably.
> openjade:xfunc.sgml:2510:32:E: element "REPLACABLE" undefined
> openjade:xfunc.sgml:2523:46:E: element "REPLACABLE" undefined

Fixing now, will be ok in v10 later today.

> * There are some inconsistency between extension names in \dx+ view
> and actual name used by CREATE EXTENSION.
> - auto_username => insert_username
> - intarray => _int
> - xml2 => pgxml
> We might need to rename them, or add 'installer'/'uninstaller' entries
> into control files to support different extension names from .so name.

The problem here is that the design and the implementation differ. The
design is to have no flexibility on the SQL script name, it has to share
the .control base name. Now, the implementation of \dx+ will report the
name field read into the control file, which can be different from the
file name.

Please note that given DROP EXTENSION name [ RESTRICT | CASCADE ]; I
think that uninstall script are deprecated.

So the fix is to either change the design so that the script file to use
is to be found in the control file, or change the implementation to
remove the name field into the control file: the name of the extension
would then be the control file name sans extension.

I lean toward adding support for a script variable into the control file
which defaults to script = '${name}.sql' and will have to be edited only
in those 3 cases you're reporting about. I'm going to work on that this
morning, it looks simple enough to get reworked if necessary.

(yes it means we have to scan all control files to find the one where
the name property is the one given in the CREATE EXTENSION command,
but the code already exists --- it still has to be refactored)

> * pg_execute_from_file() and encoding
> It expects the file is in server encoding, but it is not always true
> because we support multiple encodings in the same installation.
> How about adding encoding parameter to the function?
> => pg_execute_from_file( file, encoding )
> CREATE EXTENSION could have optional ENCODING option.
> => CREATE EXTENSION name [ ENCODING 'which' ]
>
> I strongly hope the multi-encoding support for my Japanese textsearch
> extension. Comments in the extension is written in UTF-8, but both
> UTF-8 and EUC_JP are equally used for database encodings.

That's just a part of the problem that is yet to be addressed, be
assured that I also want to fix it. Will go discover some more code and
find the APIs to make your proposal happen, should get implemented in
v10.

You're not saying it, but I think the encoding argument of the function
should be optional and default to database encoding, as the check is
already implemented:

pg_verifymbstr(query_string, nbytes, false);

> * Error messages in pg_execute_from_file()
> - "must be superuser to get file information" would be
> "must be superuser to execute file" .
> - "File '%s' could not be executed" would be
> "could not execute file: '%s'". Our message style guide is here:
> http://www.postgresql.org/docs/9.0/static/error-style-guide.html
> Many messages in extension.c are also to be adjusted.

Will do.

> commands/extension.c needs to be cleaned up a bit more:
> * fsize in read_extension_control_file() is not used.
> * ferror() test just after AllocateFile() is not needed;
> NULL checking is enough.
> * malloc() in add_extension_custom_variable_classes().
> I think the README says nothing about malloc() except assign_hook
> cases; palloc would be better here.

I didn't read it as assign_hook only, because at least in firsts
incarnations of the patch it looked like I was creating guc entries by
myself. So I compared to guc_malloc, guc_realloc, guc_strdup. Will
happily switch to memory context dependent allocations if that's what to
do here.

> BTW, did you register your patch to the next commitfest?
> It would be better to do so for tracking the activities.
> https://commitfest.postgresql.org/action/commitfest_view?id=8

Will do starting with v10, but until about v8 the patch was missing
features: I wanted to make sure the direction taken was ok, but it
didn't feel ready to submit. It's taking shape now ;)

Regards, and thanks again for your reviewing time!
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support