Re: Review: Extensions Patch

"David E. Wheeler" <david@kineticode.com> writes:
> Overall I think the docs could use a lot more fleshing out. Some of
> the stuff in the wiki would help a lot. At some point, though, I'll
> work over the docs myself and either send a patch to you or to the
> list (if it has been committed to core).

Ok, I'll leave it to you then.

> 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)?

> 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

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

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

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

> Yes, I think if you just mentioned that the comment is used for an
> implicit CREATE COMMENT that it would be useful. That's true, yes? The
> others are okay as-is.

Will do that too, ok. As soon as I'm able to git pull :)

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

>> 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? :)

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

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

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

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.

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

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

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

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

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

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 :)

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

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

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

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

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr