Re: Extensions, this time with a patch

On Fri, Oct 22, 2010 at 1:31 AM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:
> Of course, you what that means? Yes, another version of the patch, that
> will build the control file out of the control.in at build time rather
> than install time, and that's back to using EXTVERSION both in the
> Makefile and in the .control.in file.

Here are detailed report for v9 patch.

* extension.v9.patch.gz seems to contain other changes in the repo.
Did you use old master to get the diff?

* 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

* 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.

* 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.

* 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.

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.


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

-- 
Itagaki Takahiro