Re: Review: Extensions Patch

On Dec 7, 2010, at 8:00 AM, Dimitri Fontaine wrote:

>> You write a very simple contrib module exclusively for testing. It
>> doesn't have to actually do anything other than create a couple of
>> objects. A domain perhaps.
>
> What about unaccent? Or lo (1 domain, 2 functions)?

Sure. Doesn't have to actually do anything.

>> Yeah, I haven't tried the dump and restore stuff. Are there no
>> dump/restore tests in core? If there are, I expect all you'd have to
>> do is add an extension or two to be dumped and restored.
>
> Not that I know of. Grep finished with no matches found.
>  grep -nH -re pg_dump src/test

Pity.

>> I think the catalog should be pg_created_extensions (or
>> pg_installed_extensions) and the view should be
>> pg_available_extensions.
>
> Well I don't see the point into changing both of them, so I'll change
> only the view name.

Just to make it less ambiguous

>> Is there no way to have a catalog table visible to *all* databases
>> that contains the list of available extensions?
>
> That's called a shared catalog. I don't see any benefit of having to
> maintain that when we do already have a directory containing the files
> and the restriction that extensions names are the file names.

Because then you don't need control files at all.

> Again, if you really want to have that, you have to first detail how and
> you fill in the shared catalog, and update it.

`make install` should do it. From variables in the Makefile.

>> Okay. I think I see the need to differentiate between a variable in
>> .control.in replaced by `make` and one replaced by `CREATE
>> EXTENSION`. Something explaining that might be good.
>
> That's documenting the building process and I guess should go into the
> PGXS section of the manual, right?

Or in the "putting it all together" section.

>>> Updates on the manual style paragraph to explain that are welcome. It
>>> could be that what's needed is a better overview of the whole thing
>>> somewhere, but I though what's written would be enough. It seems to be
>>> only enough when you already know what it's all about --- and that's
>>> typical of Unix style man pages. Do we want to include an extension
>>> author tutorial?
>>
>> Yes!
>
> Would you include that in the documentation rework and patch you're
> talking about or expect me to care about it? :)

Possibly. I'm not going to do it this week; seems like there are some issues that still need shaking out in the
implementation,to judge from the "pg_execute_from_file review" thread. 

>>> MODULES = autoinc insert_username moddatetime refint timetravel
>>> EXTENSION = autoinc auto_username moddatetime refint timetravel
>>> EXTVERSION = $(VERSION)
>>>
>> No, wait, you have five *extensions* above (but only one version
>> number). Why not just make it *on* extension in five scripts? That
>> would be less confusing to me, given that you can have only one
>> version number, it appears.
>
> One script, one extension. If you want to manage things otherwise
> internally, it's easy enough, either with a Make rule using cat, or
> using pg_execute_from_file() from your main script.
>
> Now, the contrib/spi directory really does contain 5 different
> extensions. I don't see a good reason not to support that.

Each would get a separate control file. The mapping of one version number to multiple extensions is potentially
confusing.

>> Yes. Change the error message to "This function can only be called from SQL
>> files executed by CREATE EXTENSION".
>
> Thanks for the wording, I'd only change files to script (singular).

Sure, that works.

>> It's probably fine, given the precedent of MODULE_PATHNAME. I'd rather
>> see the .control file killed from the distribution altogether (that
>> is, generated by `make`).
>
> It only works fine when you have a single Makefile for a single
> extension. As soon as you want more than one extension managed by a
> single Makefile, it's an horror story.

Why is that? We currently manage multiple script files, test files, etc. in a single Makefile. Wildcard operators are
veryuseful for this sort of thing. 

> So we need a way to indicate in the Makefile that we want the building
> process to create the control file from the Make variables. I'm not sure
> if the non-existence of a .control.in or a .control file is enough a
> clue, and I remember about contorted Make rules. Will try again.

Why wouldn't that be enough of a clue?

>> Oh, so they're not defined placeholders. I can do anything I want
>> supported by variadic replace(). Seems kind of weird, though. Erm,
>> except that @extschema@ is explicitly supported by CREATE EXTENSION,
>> right?
>
> Yes, this very name is hard-coded.
>
>>    SET search_path = @extschema@;
>>
>> I'd rather not have to.
> [...]
>> IOW, if I install extension "foo" and it does *not* have the above
>> magic line, then this command will *not* do what I expect:
>>
>>    CREATE EXTENSION foo WITH SCHEMA bar;
>>
>> Extension "foo" will be in the public schema (usually) rather than "bar".
>
> Well, before that you had to explicitly write public in there, which IMO
> is so much worse. Then again, I now think that the right way to approach
> that is to remove this feature. The user would have a 2-steps operation
> instead, but that works right always.

Yes, that would be preferable, but a one-step operation would of course be ideal.

> In this idea, CREATE EXTENSION will always create objects in the schema
> that is hard-coded in the script file, but once this is done, it's quite
> easy to check about them all being in the same place.

If there is no hard-coded schema in the extension script, what would it do? First schema in search_path, I presume.

>  BEGIN;
>   CREATE EXTENSION citext;
>   ALTER EXTENSION citext SET SCHEMA utils;
>  COMMIT;
>
> In the future we might want to extend this command to support for moving
> things in more than one schema at a time. I'm just thinking the need is
> not there though.
>
>  ALTER EXTENSION name SET SCHEMA foo TO bar, baz TO quux;

Perhaps. v2, eh? ;-P

>> Yeah, was just a thought. I expect that everything in contrib can work
>> even if it's not an extension, and could be converted in a later
>> commit. But it's not a big deal to do it up-front.
>
> Things that are not extensions in contrib are things that don't have any
> SQL script, so that they're not creating objects in a database. That's it.

Sure, not the point I was making, but it's not important.

>> Yes, shared object. Don't you need to restart the server to have it
>> pick up a new dso now?
>
> No, that's only when you want to preload the module (that's how a .so
> file is called) in shared_preload_libraries. Other than that the module
> is loaded on-demand, and per-session.

Some do require shared_preload_libraries, no?

>> Oh, agreed. Better still would be some way to have a catalog (heh) of
>> available extensions readable from all databases in a cluster. Then,
>> amusingly enough, one wouldn't need control files, either. But maybe
>> that's not possible in PostgreSQL? I'm not sure.
>
> You have to think about the whole install process of extensions,
> here. CREATE EXTENSION is only the last step, it requires you have
> already installed the files in the system. The control file allows us to
> get meta-data about the extension for registering it into the system (to
> get an OID to depend upon), then you run the SQL script that has to be
> there in $PGDATA/contrib, then when you use the objects installed, you
> might need to load a module (.so) that has to be found in $libdir.

Sure. Just s/control file/shared catalog/.

> Now you're saying that we could bypass the control file. That would mean
> that to do the extension registering, the extension author would have to
> provide for something else. Maybe another SQL command. That was the idea
> I had a long time ago, until Heikki proposed the current scheme over a
> bear at the Royal Oak, and I don't want to make a step backward :)

Why? If the metadata is in the Makefile (or META.json), then `make install` could use it to populate the shared
catalog.

>> I think so. That function is used by the view, right?
>
> From psql \d
>
> View definition:
> SELECT e.name, e.version, e.custom_variable_classes, e.comment, e.installed
>   FROM pg_extensions() e(name, version, custom_variable_classes, comment, installed);
>
>> Ah. SPI isn't able to be more precise about it, eh? Pity. Maybe for
>> hstore we could silence the warning in the install script, eh?
>
> Mmm, set client_encoding to ERROR around creating the operator would
> work, but that would still fill the logs. Do we want to suppress this
> warning here?
   SET client_min_messages TO warning;   SET log_min_messages    TO warning;

Though I think I'd rather that the warning still went to the log.

>> It seems to me that if you can put an extension in a schema, then you
>> can put another extension with the same name in another schema. That's
>> how it works for all other schema-residing objects, AFAIK.
>
> Extensions create objects that are schema-qualified, but are not
> themselves. You don't put an extension in a schema, you change the
> schema where the extension's objects live.

Yes. That's rather confusing IMHO. Not sure of a better way, though.

>> Ah! Yep, that was the problem. Still, it would be great if I could get
>> better feedback as to what the error actually is. I'm not sure I ever
>> would have figured it out myself, given that it works just fine if I
>> install it the old-fashioned way (psql -f).
>
> I'd need some input on how to fix SPI to support that.

Yeah, that would be really helpful.

>>>> * I could omit the @extschema@ business altogether and just let CREATE
>>>> EXTENSION set the path for the duration of its execution.
>>>
>>> That won't fly.
>>
>> Why not?
>
> What you want is to know that the script did create its objects into the
> schema you're giving, but there's no reason the script is not
> hard-coding the schema, or using more than one. Back to the 2-steps idea.

And it may be that the extension needs to be in more than one schema, as Tom pointed out. Better for the common case,
though,would be to limit the extension objects to a single schema. Then extension scripts wouldn't need the set
search_pathstatement, or they would be ignored by CREATE EXTENSION. 

Best,

David