Re: Extensions, this time with a patch

Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> writes:
> Here are detailed report for v9 patch.

And here's v10 patch, to fix reported problems.

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

Fixed.

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

Fixed by having the CREATE EXTENSION command consider it's being given
the name of the extension as found in the control file, rather than the
control file base 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' ]

So after adding support for this option in the command, I realized it
might be useless. What I've done is implemented an 'encoding' parameter
in the control file, so that the extension's author / maintainer are
able to set that from there.

As I began by implementing the syntax, I didn't remove it yet, and maybe
there's a use case for it. Some strange encoding setting might require
the DBA to override what the author thinks the script encoding is?

The implementation of the encoding parameter is that the command takes
precedence over the control file, and the given encoding will be used in
a SetClientEncoding() call surrounding the call to pg_execute_from_file.

Now, I didn't change anything in this SQL callable function on the
grounds that when using it directly, you can always SET client_encoding
TO whatever you need. That's much less practical when executing an
extension's script because you're not in a position to know what is the
right value (well there's the encoding parameter set in the control file
now).

I didn't (yet) specified any encoding for the contrib control files, as
I'm not sure lots of thoughts have been given to them. Should I set
SQL_ASCII, following what file(1) tells me, or do we aim for another
encoding here, or is it useless (as I think) to set SQL_ASCII when the
default is the database encoding?

As a side effect, juggling with the syntax here allowed me to support
the full WITH [ NO ] USER DATA form that I had in mind before to find
opt_with_data ready to use. So pg_dump output had to change too.

> * Error messages in pg_execute_from_file()
> Many messages in extension.c are also to be adjusted.

I've been edited some of them, the ones that I didn't paste from
existing places, but I'm yet to stop by the error style guide...

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

Cleaning done in v10, attached.

> * malloc() in add_extension_custom_variable_classes().
> I think the README says nothing about malloc() except assign_hook
> cases; palloc would be better here.

Not sure about that, as I said, because all guc.c memory allocation is
done with malloc() rather than palloc().

> 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 when I get this email in the archives.

Thanks again, regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support