Re: Extensions, this time with a patch

Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> writes:
>> Fixed in v4, attached.
>
> It works as expected in basic functionality, so I pick up edge case issues.
> Some of them might be a problem for the patch; They might come from our
> non-standardized module design.

I see you noticed v5 later in another mail, but still pick this one to
answer. Detailed answer follow, in summary I attach a v6 of the patch
with fixes for most of your comments (and the ones from Robert and
Álvaro too).

> ==== Source code ====
> * It still has a warning.
> backend/utils/init/postinit.c needs #include "commands/extension.h".

Fixed in v6, attached.

> * It has duplicated oids.
> $ ./duplicate_oids

I'm discovering this tool, thanks for pointing it out. Fixed in v6, attached.

> * Some modules dumps noisy NOTICE or WARNING messages.
> We need to fix btree_gist, chkpass, cube, hstore, isn, ltree, pg_trgm,
> and seg. CREATE EXTENSION commands for them dumps the contents of installer
> script as CONTEXT. We can add SET client_min_messages in each script, but
> there is an issue to use SET command in scripts, discussed in below.

In v6 patch, should client_min_messages or log_min_messages be lower
than WARNING, they get set to WARNING for the script install context. We
still dump the extension's script at each WARNING, but you can set your
client_min_messages (and log_min_messages) to ERROR before hand.

That's following comments from Robert Haas and also includes rework
asked by Álvaro Herrera.

> * Modules that should be added to shared_preload_libraries.
> auto_explain, dummy_seclabel, passwordcheck, and pg_stat_statements are
> designed to be preloaded with shared_preload_libraries. If we support
> modifying GUC variables vis SQL, we could add some hooks to update
> conf files.

For this to work as custom_variable_classes, we need to have the setting
effects apply at two times: when loading the extension, and when
connecting to a database.

Now do we want to be able to have s_p_l SUSET and to change it
dynamically at connection time depending on what extensions are
installed? That sounds a lot like local_preload_libraries now.

Even then, it does not seem like local_preload_libraries is loaded after
you have access to the catalogs, but maybe we could have a second run of
process_local_preload_libraries().

All in all, it seems like custom_variable_classes is a GUC for extension
authors to deal with and it was practical to drive the implementation
there, but I'm less convinced of what we can do for the libs, because of
the backend init timing.

What I think we should do is to not produce a .control file for
extensions that have no .sql script. I've made that happen in the
attached version of the patch, v6.

> * Modules that only have *.sql, but not *.sql.in.
> There are no *.control files for intagg and intarray, maybe because they
> don't have *.sql.in. But we should support them, too.

Well in fact there is, but the .sql names are different from the contrib
directory names:

dim=# create extension int_aggregate;
NOTICE: Installing extension 'int_aggregate' from '/Users/dim/pgsql/exts/share/contrib/int_aggregate.sql', with user data
CREATE EXTENSION
dim=# select * from pg_extension_objects('int_aggregate');
class | classid | objid | objdesc
--------------+---------+-------+------------------------------------------
pg_extension | 3996 | 16855 | extension int_aggregate
pg_proc | 1255 | 16856 | function int_agg_state(internal,integer)
pg_proc | 1255 | 16857 | function int_agg_final_array(internal)
pg_proc | 1255 | 16858 | function int_array_aggregate(integer)
pg_proc | 1255 | 16859 | function int_array_enum(integer[])
(5 rows)

dim=# create extension _int;
NOTICE: Installing extension '_int' from '/Users/dim/pgsql/exts/share/contrib/_int.sql', with user data
CREATE EXTENSION
dim=# select count(*) from pg_extension_objects('_int');
count
-------
111
(1 row)

Note: this new function will be mostly useful to help extension authors
find their existing object ids at upgrade time, but is nice for users
too.

> * Hyphen (-) in module name
> 'uuid-ossp' has a hyphen in the name. Do we need double-quotes to install
> such extensions? (CREATE EXTENSION "uuid-ossp" works.)
> Also, custom_variable_classes doesn't support hyphens; only [A-Za-z0-9_]
> are accepted for now, but will we also support hyphens?

Well I think requiring to quote extension names containing hyphens is a
good idea... that's consistent with any other SQL object name, isn't it?

> * xml2 module has a different extension name from the directory name.
> The directory name is 'xml2', but the extension name is 'pgxml'.
> I'm not sure whether we should change the names. Or, merging xml2 module
> into core and deprecating xml2 might be the best solution.

Same with intagg and intarray. Do we want to fix all this?

There's \dx+ in the patch so that you can list available extensions, or
you can SELECT * FROM pg_extensions; too.

> * spi module has multiple *.so, but only one *.control.
> 'spi' module generates multiple libraries: 'autoinc', 'insert_username',
> 'moddatetime', 'refint', and 'timetravel'. But it has only autoinc.control.
> We might need control files for each library.

Yes I think the best answer here will be to edit the
contrib/spi/Makefile and to manually provide a list of the control files
wanted. I've done that in the attached patch:

+CONTROL = $(addsuffix .control, $(MODULES))
+EXTVERSION = $(MAJORVERSION)

Now we properly have one extension per MODULES in contrib/spi.

> ==== CREATE EXTENSION command ====
> * Environment could be modified by the installer script.
> =# SHOW search_path; => "$user",public
> =# CREATE EXTENSION dblink;
> =# SHOW search_path; => public
> because almost all of the modules have SET search_path in the scripts:
> -- Adjust this setting to control where the objects get created.
> SET search_path = public;
>
> Is is an intended behavior? Personally, I want the installer to run in sandbox.
> One idea is to rewrite module scripts to use BEGIN - SET LOCAL - COMMIT,
> but we cannot execute CREATE EXTENSION in transaction if do so.

Using SPI to execute the extension's script already means that it can
not contain explicit BEGIN and COMMIT commands. Now, is it possible to
force a Reset of all GUCs after script's execution?

> * Non-ASCII characters in script
> User-defined module could have non-ascii characters in their install script.
> They often have "SET client_encoding" at the beginning, and it works as
> expected if executed by psql. However, it raises encoding errors if executed
> by CREATE EXTENSION because they are executed as one multi-command.
>
> Are there any solution to solve the issue? I think it's a bit difficult
> because encodings in database, script, and client might be different.
> If encodings in script and client are different, the server might
> need to handle two different client encodings in the same time.

No idea here, at least yet.

Thanks again for your detailed reviewing!

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