Re: Extensions, this time with a patch
От | Dimitri Fontaine |
---|---|
Тема | Re: Extensions, this time with a patch |
Дата | |
Msg-id | m28w1qekjr.fsf@2ndQuadrant.fr обсуждение исходный текст |
Ответ на | Re: Extensions, this time with a patch (Itagaki Takahiro <itagaki.takahiro@gmail.com>) |
Ответы |
Re: Extensions, this time with a patch
|
Список | pgsql-hackers |
Itagaki Takahiro <itagaki.takahiro@gmail.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
Вложения
В списке pgsql-hackers по дате отправления: