Extensions, this time with a patch

Hi,

Please find attached a WIP patch for extension's pg_dump support in
PostgreSQL, following design decisions that we've outlined earlier at
this year's and last year's PGCon developer meetings.

What's in the patch?

An extension is a new SQL object with a catalog and two commands to
manage them (reserved to superuser):

CREATE EXTENSION <extension> ;
DROP EXTENSION [IF EXISTS] <extension> [ RESTRICT | CASCADE ];

The first command (create) will parse the "control" file from a fixed
place (`pg_control --sharedir`/contrib/<extension>.control) and insert
an entry in the pg_extension catalog. That gives us an Oid which we can
use in pg_depend. Once we have it, the command will execute the SQL
script at same/place/<extension>.sql and recordDependencyOn lots of
objects created here (not all of them, because it does not appear
necessary to).

The drop command will happily remove the extension's object and all its
"direct" dependencies. That's what's been installed by the script. If
there exists some object not created by the script and that depends on
the extension, CASCADE is needed (e.g. create table foo(kv hstore);).

With that tracking in place, pg_dump is now able to issue a single
command per extension, the CREATE command. All dependent objects are
filtered out of the dump in the pg_dump queries. There's a nasty corner
case here with schema, see pg_dump support.

Rough support for a new \dx command in psql is implemented, too.

PGXS has been adapted so that it produces automatically the control file
should it be missing, using $(MAJORVERSION) as the contrib's
version. That means that right after installing contrib, it's possible
to 'create extension <any of them>' automatically (thanks to a 2 lines
file).

Open Items :

- cfparser

To parse the control/registry file, I've piggybacked on the recovery
configuration file parsing. The function to parse each line was not
exported from xlog, I've made a new src/backend/utils/misc/cfparser.c
file and placed parseRecoveryCommandFileLine() in there.

Please find attached a separate patch implementing just that, in case
you want to review/apply that on its own. The main patch do contains
the change too.

- User Documentation. Where in the manual do I write it?

- Naming of the "control" file, <extension>.{control,registry,install}

Currently, inspired by debian/control, the file is called .control,
but maybe we want to choose something that the user can guess about
the purpose before reading the fine manual. I don't like .install
because that's not what it is. .registry?

- Handling of custom_variable_classes

The attached patch has a column to store the information but makes no
use whatsoever of it, the goal would be to append classes from the
extension's registry file and possibly setup the default values
there.

As I don't think this part has been agreed before, I send the patch
without the code for that, even if I suspect it would be a rather
short addition, and very worthwile too.

- User data tables

An extension can install plain relations, and that's fine. The
problem is when the data in there are changed after installing.
Because the name of the game here is to exclude the table from the
dumps, of course the data will not be in there.

The solution would be to offer extension's author a way to 'flag'
some tables as worthy of dumping, I think marking the dependency as
DEPENDENCY_NORMAL rather then DEPENDENCY_INTERNAL will do the trick:

SELECT pg_extension_flag_dump(oid);

- Extension Upgrading

Should this be done by means of 'create extension' or some other
command, like 'alter extension foo upgrade'? The command would run
the SQL script again, which would be responsible for any steps the
extension author might find necessary to run.

- Bugs to fix

There's at least one where drop extension leaves things behind,
although it uses performDeletion(). The dependencies are fine enough
so that the leftover objects are not part of the dump done before to
drop, though. I didn't investigate it at all, this mail is in the
"discuss early" tradition of the project.

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

On Tue, Oct 12, 2010 at 08:57:09PM +0200, Dimitri Fontaine wrote:
> Hi,
>
> Please find attached a WIP patch for extension's pg_dump support in
> PostgreSQL, following design decisions that we've outlined earlier at
> this year's and last year's PGCon developer meetings.
>
> What's in the patch?
>
> An extension is a new SQL object with a catalog and two commands to
> manage them (reserved to superuser):
>
> CREATE EXTENSION <extension> ;
> DROP EXTENSION [IF EXISTS] <extension> [ RESTRICT | CASCADE ];

Kudos!

> - User Documentation. Where in the manual do I write it?

Parts belong in Server Administration, others in Server Programming.

> - Extension Upgrading
>
> Should this be done by means of 'create extension' or some other
> command, like 'alter extension foo upgrade'? The command would
> run the SQL script again, which would be responsible for any
> steps the extension author might find necessary to run.

As people will want to up- or downgrade extensions to a particular
version, this should probably be something more like ALTER EXTENSION
... SET VERSION [version number | LATEST | PREVIOUS ]... or something
like that.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Excerpts from David Fetter's message of mié oct 13 11:27:56 -0300 2010:
> On Tue, Oct 12, 2010 at 08:57:09PM +0200, Dimitri Fontaine wrote:

> > - Extension Upgrading
> >
> > Should this be done by means of 'create extension' or some other
> > command, like 'alter extension foo upgrade'? The command would
> > run the SQL script again, which would be responsible for any
> > steps the extension author might find necessary to run.
>
> As people will want to up- or downgrade extensions to a particular
> version, this should probably be something more like ALTER EXTENSION
> ... SET VERSION [version number | LATEST | PREVIOUS ]... or something
> like that.

Does this mean that the control file should contain a version number in
the filename? Otherwise, I don't see how you'd have more than one
version of the control file.

Also, if upgrading is necessary, there will need to be one "upgrade"
control file that says how to upgrade from version N to N+1.

I don't think we should really support the downgrade case. It has the
potential to get too messy -- and for what gain?

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Also, if upgrading is necessary, there will need to be one "upgrade"
> control file that says how to upgrade from version N to N+1.

> I don't think we should really support the downgrade case. It has the
> potential to get too messy -- and for what gain?

I think we could leave that to the extension author to decide. Basically,
what is needed is a script file that says how to replace version M by
version N. If the author cares to supply a script for going from
M to M-1, it's no extra logic in the extension manager to be able to
apply that one. In many cases it wouldn't be very practical to do,
but then you just don't offer the script.

regards, tom lane

On Wed, Oct 13, 2010 at 11:36:02AM -0300, Alvaro Herrera wrote:
> Excerpts from David Fetter's message of mié oct 13 11:27:56 -0300 2010:
> > On Tue, Oct 12, 2010 at 08:57:09PM +0200, Dimitri Fontaine wrote:
>
> > > - Extension Upgrading
> > >
> > > Should this be done by means of 'create extension' or some other
> > > command, like 'alter extension foo upgrade'? The command would
> > > run the SQL script again, which would be responsible for any
> > > steps the extension author might find necessary to run.
> >
> > As people will want to up- or downgrade extensions to a particular
> > version, this should probably be something more like ALTER EXTENSION
> > ... SET VERSION [version number | LATEST | PREVIOUS ]... or something
> > like that.
>
> Does this mean that the control file should contain a version number in
> the filename? Otherwise, I don't see how you'd have more than one
> version of the control file.

Excellent idea!

> Also, if upgrading is necessary, there will need to be one "upgrade"
> control file that says how to upgrade from version N to N+1.

This, too, is an excellent idea.

> I don't think we should really support the downgrade case. It has
> the potential to get too messy -- and for what gain?

I think there should be something extension authors should be able to
provide for the downgrade case, given that an upgrade could cause a
system-wide failure.

Unfortunately, the extensions not provided with a downgrade option are
the ones likeliest to need it. Authors who write and test downgrade
code are much less likely to have their upgrades cause such failures
in the first place, but there's not much to do about that.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Does this mean that the control file should contain a version number in
> the filename? Otherwise, I don't see how you'd have more than one
> version of the control file.
>
> Also, if upgrading is necessary, there will need to be one "upgrade"
> control file that says how to upgrade from version N to N+1.

I like both ideas. I'd like to propose that we get back to this part of
the feature later, after the first patch is in. After all, the main goal
is to support dump&restore of extensions. Let's do that first.

If some of you are interested, the development happens here:
http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=shortlog;h=refs/heads/extension

I've written some basic documentation in the "Extending SQL"
chapter (and command man pages too). There's not so much to tell about
the new features, as the goal is for it to fix something quite basic.

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

Excerpts from Dimitri Fontaine's message of mié oct 13 18:11:21 -0300 2010:

> I like both ideas. I'd like to propose that we get back to this part of
> the feature later, after the first patch is in. After all, the main goal
> is to support dump&restore of extensions. Let's do that first.

Okay. I looked at the code and I have to admit that it seems awkward to
have pg_dump left-joining everything against pg_depend and checking for
NULLs. I wondered if there was a simpler way to go about it, perhaps
using EXCEPT? No specific proposal though.

> If some of you are interested, the development happens here:
> http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=shortlog;h=refs/heads/extension

Thanks. I managed to retrieve into an already-checked-out copy of HEAD
and it worked pretty well:
git remote add extensions git://git.postgresql.org/git/postgresql-extension.git
git fetch extensions extension:extension

then I could run "git diff master...extension" and see the complete
diff. Of course, I can also see each commit individually. Or
"git checkout extension".

Maybe it would be worthwhile to split the parts that parse a file and
execute from a file, and submit separately. It is obviously
self-contained and serves a useful purpose on its own. It also forces
you to think harder about renaming the parse function :-)

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Okay. I looked at the code and I have to admit that it seems awkward to
> have pg_dump left-joining everything against pg_depend and checking for
> NULLs. I wondered if there was a simpler way to go about it, perhaps
> using EXCEPT? No specific proposal though.

Thanks for your time and review!

The LEFT JOIN WHERE right IS NULL is the first thing that I though
about, if it looks ugly I'll rework the queries, ok.

> Maybe it would be worthwhile to split the parts that parse a file and
> execute from a file, and submit separately. It is obviously
> self-contained and serves a useful purpose on its own. It also forces
> you to think harder about renaming the parse function :-)

The cfparser code is extracted from xlog.c and I left it as-is, so the
function is till named parseRecoveryCommandFileLine. If cfparser has a
file name is ok, I could rename the function cfParseOneLine?

I'll send separate patches for that and pg_execute_from_file() then.

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

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> Open Items :
>
> - cfparser

Still in attached v1 patch, but will repost separately, as proposed by
Álvaro.

> - User Documentation. Where in the manual do I write it?

Chapter 35. Extending SQL looked like a good choice, there it is. Needs
to get expanded with latest additions.

> - Naming of the "control" file,
> <extension>.{control,registry,install}

Issue still to discuss. I'm happy with the current .control name.

> - Handling of custom_variable_classes

This version of the patch address the point. The problem I wanted to
solve is that 'create extension' has no chance to edit the user
configuration, such as custom_variable_classes. Also, in the debian
world it is a policy violation for a package to modify another package's
configuration file.

So I wanted the extension mechanism to be able to append some classes to
the custom_variable_classes without touching the configuration
file. That's why pg_extension has this custom_class column.

The v1 patch, attached, rework this GUC so that it's SUSET. Invalid
variables are now kept aside while processing the configuration, so that
it's possible to scan them again when custom_variable_classes changes,
and to make them available.

Based on this change, postinit now will scan the pg_extension catalog
for the connected database and add all classes defined there. So that if
an extension register its own class, and the user defines some variables
in the configuration file, it's not necessary for him to edit the global
custom_variable_classes any more.

Also, at CREATE EXTENSION time, should an extension specific classes be
given, custom_variable_classes is changed on the fly, and default values
from the control file can get loaded too.

Current code is to be read as a proposal, open to discussion of course:
it seemed to me preferable to write it (having not heard back from my
earlier mail) as an opening.

> - User data tables
>
> An extension can install plain relations, and that's fine. The
> problem is when the data in there are changed after installing.
> Because the name of the game here is to exclude the table from the
> dumps, of course the data will not be in there.
>
> The solution would be to offer extension's author a way to 'flag'
> some tables as worthy of dumping, I think marking the dependency as
> DEPENDENCY_NORMAL rather then DEPENDENCY_INTERNAL will do the trick:
>
> SELECT pg_extension_flag_dump(oid);

That's in the attached patch too.

> - Extension Upgrading

That was a mistake to raise this subject this early, it seems wise to
defer that to a later patch. Much remains to be designed here.

> - Bugs to fix
>
> There's at least one where drop extension leaves things behind,
> although it uses performDeletion(). The dependencies are fine enough
> so that the leftover objects are not part of the dump done before to
> drop, though.

That well seems to me to be an existing bug that I'm just putting in the
light. The reading of comments from findDependentObjects() makes me
think that following nested DEPENDENCY_INTERNAL levels is broken, but I
didn't have time to figure out how exactly, nor to try to fix.

So, v1 patch attached, and repository available too:

http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=shortlog;h=refs/heads/extension

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

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Maybe it would be worthwhile to split the parts that parse a file and
> execute from a file, and submit separately. It is obviously
> self-contained and serves a useful purpose on its own. It also forces
> you to think harder about renaming the parse function :-)

So, you will find two new branches for those purposes at the repository,
named cfparser and pg_execute_from_file:

http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=summary

Please find attached the patches extracted from those branches. Note
that currently, the main "extension" branch still contains the
modifications, I intend to merge/rebase that against the master after
the commits.

I've also merged the master repository into my feature branch, and git
just did it all by itself. I like it when the tools are helping! :)

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

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> So, you will find two new branches for those purposes at the repository,
> named cfparser and pg_execute_from_file:
>
> http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=summary

I updated the pg_execute_from_file branch with documentation for the
function, and added documentation (catalog, functions, custom classes)
to the main feature branch too.

The cfparser patch didn't change, the current version is still v1.

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

Excerpts from Dimitri Fontaine's message of vie oct 15 16:15:23 -0300 2010:
> Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> > So, you will find two new branches for those purposes at the repository,
> > named cfparser and pg_execute_from_file:
> >
> > http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=summary
>
> I updated the pg_execute_from_file branch with documentation for the
> function, and added documentation (catalog, functions, custom classes)
> to the main feature branch too.
>
> The cfparser patch didn't change, the current version is still v1.

Hmm, did you try "make install" in contrib? It fails for me in intagg:

make[1]: Entering directory `/home/alvherre/Code/CVS/pgsql/build/HEAD/contrib/intagg'
/bin/mkdir -p '/pgsql/install/HEAD/share/contrib'
touch .control
test ! -f .control && echo "name = '' \nversion = '9.1'" > .control
make[1]: *** [.control] Error 1
make[1]: *** Deleting file `.control'
make[1]: Leaving directory `/home/alvherre/Code/CVS/pgsql/build/HEAD/contrib/intagg'
make: *** [install] Error 2

I also note that the .control file generation is not working as intended
for me -- the \n ends up verbatim in the generated files, not as a
newline. It's probably easier to call echo two times.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Alvaro Herrera's message of sáb oct 16 00:30:41 -0300 2010:

> Hmm, did you try "make install" in contrib? It fails for me in intagg:
>
> make[1]: Entering directory `/home/alvherre/Code/CVS/pgsql/build/HEAD/contrib/intagg'
> /bin/mkdir -p '/pgsql/install/HEAD/share/contrib'
> touch .control
> test ! -f .control && echo "name = '' \nversion = '9.1'" > .control
> make[1]: *** [.control] Error 1
> make[1]: *** Deleting file `.control'
> make[1]: Leaving directory
> `/home/alvherre/Code/CVS/pgsql/build/HEAD/contrib/intagg'
> make: *** [install] Error 2

Oh, I see what's going on here ... you have this bit in pgxs.mk:

# create extension support
ifndef CONTROL
ifdef MODULE_big
CONTROL = $(MODULE_big).control
EXTENSION = $(MODULE_big)
else
CONTROL = $(MODULES).control
EXTENSION = $(MODULES)
endif
endif

The reason it fails for intagg is that it doesn't define MODULE_big, so
it takes the other path. But since MODULES is not defined either, this
ends up defining CONTROL as ".control"; and then "test" returns a
failure because a file with the same name has been created in the
previous line.

I don't think the MODULES bit works either; see the spi contrib for an
example. What it ends up doing is probably not what you want.

Maybe what you should be doing here is that modules should provide
another definition, say EXTENSION, and they have to explicitely define
it in their Makefile (maybe require EXTENSION_VERSION too or something
like that). I think the idea that modules should continue to work as
extensions without any modification is doomed.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Dimitri Fontaine's message of vie oct 15 16:15:23 -0300 2010:

> I updated the pg_execute_from_file branch with documentation for the
> function, and added documentation (catalog, functions, custom classes)
> to the main feature branch too.

Hmm. To be honest I don't like the direction that pg_execute_from_file
has taken. (Now that I look, it's been like this since inception). I
have two problems with it: one is that it is #including half the world
into genfile.c. This already smells trouble in itself. I got worried
when I saw the CommandDest declaration. Really, I think having the guts
of postgres.c into that file is not a good idea from a modularisation
point of view.

The other problem is that it's slurping the whole file and executing it
as a single query. This is really two problems: one is that you
shouldn't be trusting that the file is going to be small enough to be
read that way. The other one is that I don't think it's a good idea to
execute it in a fell swoop; seems to be it would be better to split it
into queries, and rewrite/parse/plan/execute them one by one.

I think a better way to go about this is to have another entry point in
postgres.c that executes a query the way you want; and somehow read the
file in small chunks, find where each query ends, and execute them one
by one. (To be honest, I have no idea how to find out where each query
ends. psql knows how to do it, but I'm not sure how trustworthy it is.)

As far as #include lines go, please keep them in alphabetical order. As
a matter of style we always have "postgres.h" alone, then a blank line,
then system includes, another blank, then the rest of the includes.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Maybe what you should be doing here is that modules should provide
> another definition, say EXTENSION, and they have to explicitely define
> it in their Makefile (maybe require EXTENSION_VERSION too or something
> like that). I think the idea that modules should continue to work as
> extensions without any modification is doomed.

In fact there's ifndef CONTROL that protects the black magic failing
part, so that we could edit any contrib's Makefile to give the
information we're trying to guess. I just had another try at it that
seems to work much better, based on DATA and DATA_built:

# create extension support
ifndef CONTROL
ifdef DATA_built
EXTENSION = $(basename $(notdir $(firstword $(DATA_built))))
else ifdef DATA
EXTENSION = $(basename $(notdir $(firstword $(DATA))))
endif
ifdef EXTENSION
CONTROL = $(EXTENSION).control
endif
endif

Also, I've switched to using echo twice as you recommended, that's much
better too.

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

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Hmm. To be honest I don't like the direction that pg_execute_from_file
> has taken. (Now that I look, it's been like this since inception). I
> have two problems with it: one is that it is #including half the world
> into genfile.c. This already smells trouble in itself. I got worried
> when I saw the CommandDest declaration. Really, I think having the guts
> of postgres.c into that file is not a good idea from a modularisation
> point of view.

Understood. The thinking back when I worked on that part was to minimize
the diff and remain localized, which is known to be a bad idea... I'll
rework that part as soon as we agree on the other one:

> The other problem is that it's slurping the whole file and executing it
> as a single query. This is really two problems: one is that you
> shouldn't be trusting that the file is going to be small enough to be
> read that way. The other one is that I don't think it's a good idea to
> execute it in a fell swoop; seems to be it would be better to split it
> into queries, and rewrite/parse/plan/execute them one by one.
>
> I think a better way to go about this is to have another entry point in
> postgres.c that executes a query the way you want; and somehow read the
> file in small chunks, find where each query ends, and execute them one
> by one. (To be honest, I have no idea how to find out where each query
> ends. psql knows how to do it, but I'm not sure how trustworthy it
> is.)

Well, that's the reason why it's done this way now, relying on a multiple
queries portal. The only trustworthy way to split the queries apart in
the SQL install script would be to rely on gram.y, and I didn't find the
API to explicitly loop over each query parsed.

Given some advice, I'll rework that part too. The good news is that it's
well separated from the rest of the extension's work.

We need a way to do the same as \i in psql, but from the backend, and we
won't be returning anything from there, so we don't need to handle more
than one portal definition in a single fe/be communication.

> As far as #include lines go, please keep them in alphabetical order. As
> a matter of style we always have "postgres.h" alone, then a blank line,
> then system includes, another blank, then the rest of the includes.

Will do.

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

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>> The other problem is that it's slurping the whole file and executing it
>> as a single query.

> Given some advice, I'll rework that part too. The good news is that it's
> well separated from the rest of the extension's work.

I think that's something that could be left for later, if not never.
I find it hard to imagine extension modules with more than a few
thousand commands (the largest one in contrib is isn, with about 500).
No modern machine is going to have the slightest difficulty with that.
If it ever does get to be a problem in practice, we could rework the
implementation at that time.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> I think that's something that could be left for later, if not never.

That's very great news. I'm left with moving the bulk of the code away
from genfile.c and into postgres.c, and have the former be a user
callable shell around the later, I suppose. Right?

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

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> That's very great news. I'm left with moving the bulk of the code away
> from genfile.c and into postgres.c, and have the former be a user
> callable shell around the later, I suppose. Right?

Here it is, looks much better this way.

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

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> I think that's something that could be left for later, if not never.

> That's very great news. I'm left with moving the bulk of the code away
> from genfile.c and into postgres.c, and have the former be a user
> callable shell around the later, I suppose. Right?

Umm ... I fail to see why an extensions patch should be touching
postgres.c at all, let alone injecting a large amount of code there.
Whatever you're doing there probably requires some rethinking.

regards, tom lane

Excerpts from Tom Lane's message of sáb oct 16 19:52:27 -0300 2010:
> Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> > Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> >> I think that's something that could be left for later, if not never.
>
> > That's very great news. I'm left with moving the bulk of the code away
> > from genfile.c and into postgres.c, and have the former be a user
> > callable shell around the later, I suppose. Right?
>
> Umm ... I fail to see why an extensions patch should be touching
> postgres.c at all, let alone injecting a large amount of code there.
> Whatever you're doing there probably requires some rethinking.

Hm, it was me that led him in that direction. The original patch was
just copying a bunch of code from postgres.c into genfile.c, which
struck me as a worse proposition.

The intent here is to execute some code from the file directly inside
the server.

Eh, I realize now that the right way to go about this is to use SPI.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> The intent here is to execute some code from the file directly inside
> the server.

> Eh, I realize now that the right way to go about this is to use SPI.

Yeah, that would be one way to go about it. But IMO postgres.c should
be solely concerned with interactions with the client.

regards, tom lane

Excerpts from Tom Lane's message of sáb oct 16 23:32:49 -0300 2010:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > The intent here is to execute some code from the file directly inside
> > the server.
>
> > Eh, I realize now that the right way to go about this is to use SPI.
>
> Yeah, that would be one way to go about it. But IMO postgres.c should
> be solely concerned with interactions with the client.

Duly noted.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Sun, Oct 17, 2010 at 12:09:51AM -0300, Alvaro Herrera wrote:
> Excerpts from Tom Lane's message of sáb oct 16 23:32:49 -0300 2010:
> > Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > > The intent here is to execute some code from the file directly inside
> > > the server.
> >
> > > Eh, I realize now that the right way to go about this is to use SPI.
> >
> > Yeah, that would be one way to go about it. But IMO postgres.c should
> > be solely concerned with interactions with the client.
>
> Duly noted.

Should this be noted in a README? Source code comments? I'm thinking
if Alvaro didn't know it, it's not clear enough from context.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>> Eh, I realize now that the right way to go about this is to use SPI.
>
> Yeah, that would be one way to go about it. But IMO postgres.c should
> be solely concerned with interactions with the client.

I didn't notice it's "possible" to use SPI from within the backend core
code, and now see precedent in xml.c where the user can give a query
string. I've used SPI_execute() in the new (attached) version of the
patch, that's not touching postgres.c at all anymore.

The bulk of it is now short enough to be inlined in the mail, and if you
have more comments I guess they'll be directed at this portion of the
patch, so let's make it easy:

/*
* We abuse some internal knowledge from spi.h here. As we don't know
* which queries are going to get executed, we don't know what to expect
* as an OK return code from SPI_execute(). We assume that
* SPI_OK_CONNECT, SPI_OK_FINISH and SPI_OK_FETCH are quite improbable,
* though, and the errors are negatives. So a valid return code is
* considered to be SPI_OK_UTILITY or anything from there.
*/
SPI_connect();
if (SPI_execute(query_string, false, 0) < SPI_OK_UTILITY)
ereport(ERROR,
(errcode(ERRCODE_DATA_EXCEPTION),
errmsg("File '%s' contains invalid query", filename)));
SPI_finish();

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

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
>> - Bugs to fix
>>
>> There's at least one where drop extension leaves things behind,
>> although it uses performDeletion(). The dependencies are fine enough
>> so that the leftover objects are not part of the dump done before to
>> drop, though.
>
> That well seems to me to be an existing bug that I'm just putting in the
> light. The reading of comments from findDependentObjects() makes me
> think that following nested DEPENDENCY_INTERNAL levels is broken, but I
> didn't have time to figure out how exactly, nor to try to fix.
>
> http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=shortlog;h=refs/heads/extension

Here's another version of the patch, v3. Changes:

- mentioned bug is fixed, that indeed was a shortcoming in following
dependencies, due to a new way of using DEPENDENCY_INTERNAL in the
extension code.

- system view pg_extensions, using a function of the same name, lists
all available and installed extensions. That should make the life of
pgAdmin developers easier, among other users :)

- pg_dump now issues CREATE EXTENSION foo WITH NO DATA; variant, and
extension install script can use pg_extension_with_user_data() which
returns true only when they have to create user data objects (often,
fact table with pre-loaded data that the user is free to change)

- pgxs.mk now will use given $(EXTENSION), $(EXTVERSION) and
$(EXTCOMMENT) variables to produce the control file, or use an
existing one. When $(EXTENSION) is not given, it will try to guess it
from $(DATA) and $(DATA_built).

- more documentation, I think it's all covered now

- merges of the cfparser and pg_execute_from_file latest versions, so
that the patch is self-contained and you can test it should you want
to. I intend to merge from the official branch (pgmaster here) then
produce another version of the patch without that code once it's
separately committed, as that's the outlined plan so far.

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

On Mon, Oct 18, 2010 at 8:37 PM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Here's another version of the patch, v3. Changes:

CREATE EXTENSION is interesting feature!
I just compiled it and tested it via SQL commands. Here is a quick report.

* There are some compiler warnings. You might be missing something in
copyfuncs and equalfuncs.
----
copyfuncs.c:3119: warning: ‘_copyCreateExtensionStmt’ defined but not used
copyfuncs.c:3130: warning: ‘_copyDropExtensionStmt’ defined but not used
equalfuncs.c:1593: warning: ‘_equalCreateExtensionStmt’ defined but not used
equalfuncs.c:1602: warning: ‘_equalDropExtensionStmt’ defined but not used
postinit.c: In function ‘CheckMyDatabase’:
postinit.c:341: warning: implicit declaration of function ‘ExtensionSetCVC’
----

* There might be some bugs in pg_dump:
----
postgres=# CREATE EXTENSION dblink;
NOTICE: Installing extension 'dblink' from
'$PGHOME/share/contrib/dblink.sql', with user data
CREATE EXTENSION
postgres=# \q
$ pg_dump
pg_dump: schema with OID 2200 does not exist, but is needed for object 16411
----

* The example in the doc "CREATE EXTENSION hstore" dumps surprising
warning messages,
We would be better to avoid such messages, though it's not an issue
for EXTENSION.
----
WARNING: => is deprecated as an operator name
DETAIL: This name may be disallowed altogether in future versions of
PostgreSQL.
CONTEXT: SQL statement "/* contrib/hstore/hstore.sql.in */
(followed by dumped script)
----

* Docs sql-createextension.html has two odd links:
----
See Also
DROP EXTENSION, Table 9-61, Appendix F
----

--
Itagaki Takahiro

Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> writes:
> CREATE EXTENSION is interesting feature!
> I just compiled it and tested it via SQL commands. Here is a quick
> report.

Thanks for you time and interest!

> * There are some compiler warnings. You might be missing something in
> copyfuncs and equalfuncs.
> ----
> copyfuncs.c:3119: warning: ‘_copyCreateExtensionStmt’ defined but not used
> copyfuncs.c:3130: warning: ‘_copyDropExtensionStmt’ defined but not used
> equalfuncs.c:1593: warning: ‘_equalCreateExtensionStmt’ defined but not used
> equalfuncs.c:1602: warning: ‘_equalDropExtensionStmt’ defined but not used
> postinit.c: In function ‘CheckMyDatabase’:
> postinit.c:341: warning: implicit declaration of function ‘ExtensionSetCVC’
> ----

Ouch, sorry about that, I didn't spot them. Will fix and post a v4 patch soon.

> * There might be some bugs in pg_dump:
> ----
> postgres=# CREATE EXTENSION dblink;
> NOTICE: Installing extension 'dblink' from
> '$PGHOME/share/contrib/dblink.sql', with user data
> CREATE EXTENSION
> postgres=# \q
> $ pg_dump
> pg_dump: schema with OID 2200 does not exist, but is needed for object 16411
> ----

I've hit that sometime but though that were tied to the dependency bug
fixed in the v3 patch. I can reproduce here, will fix too.

> * The example in the doc "CREATE EXTENSION hstore" dumps surprising
> warning messages,
> We would be better to avoid such messages, though it's not an issue
> for EXTENSION.
> ----
> WARNING: => is deprecated as an operator name
> DETAIL: This name may be disallowed altogether in future versions of
> PostgreSQL.
> CONTEXT: SQL statement "/* contrib/hstore/hstore.sql.in */
> (followed by dumped script)
> ----

I don't have a dumped script here, that maybe depends on verbosity
options?

> * Docs sql-createextension.html has two odd links:
> ----
> See Also
> DROP EXTENSION, Table 9-61, Appendix F
> ----

I didn't know if using xref would do, but if you find that odd, I will
replace with linkend and a custom label there.

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

Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> writes:
> * There are some compiler warnings. You might be missing something in
> copyfuncs and equalfuncs.

Fixed in v4, attached.

> * There might be some bugs in pg_dump:
> pg_dump: schema with OID 2200 does not exist, but is needed for object
> 16411

Fixed in v4, attached.

The problem was with the query used in pg_dump to filter out relations
that are part of an extension, in getTables(). A composite type will
create an underlying relation of relkind 'c', but there was no direct
dependency to the extension, thus the filter failed to bypass it.

It's fixed by adding a direct internal dependency between the relation
and the extension, as that's so much easier than doing a recursive scan
of pg_depend in pg_dump SQL queries.

I will try to find out if other cases are forgotten like this one.

Note that as a result you need to drop extension dblink then create it
again so that you benefit from the fix. Also note that drop extension is
recursively following the dependencies so is not concerned by this bug.

> * The example in the doc "CREATE EXTENSION hstore" dumps surprising
> warning messages,
> We would be better to avoid such messages, though it's not an issue
> for EXTENSION.
> ----
> WARNING: => is deprecated as an operator name
> DETAIL: This name may be disallowed altogether in future versions of
> PostgreSQL.
> CONTEXT: SQL statement "/* contrib/hstore/hstore.sql.in */
> (followed by dumped script)
> ----

I didn't realise that using SPI would mean dumping the all script in
case of even NOTICEs. May be we want to protect against that in the
CREATE EXTENSION case, but I didn't have a look at how to do it. Do we
want CREATE EXTENSION to be more quiet than SPI usually is?

> * Docs sql-createextension.html has two odd links:
> ----
> See Also
> DROP EXTENSION, Table 9-61, Appendix F
> ----

Fixed in v4, attached.

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

On Oct 19, 2010, at 8:33 AM, Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> wrote:
> I didn't realise that using SPI would mean dumping the all script in
> case of even NOTICEs. May be we want to protect against that in the
> CREATE EXTENSION case, but I didn't have a look at how to do it. Do we
> want CREATE EXTENSION to be more quiet than SPI usually is?

I don't see why. I think the real action item here is to remove => from hstore.

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I don't see why. I think the real action item here is to remove =>
> from hstore.

As input, consider that lots of extensions will create types that are
only a shell at the moment of the CREATE TYPE, and for each of those
types you will see the (potentially > 1000 lines long) whole SQL script
dumped on the screen.

In the following script, I've filtered out the scripts, but it's written
out for each NOTICE message that you see:

dim ~/dev/PostgreSQL/postgresql-extension psql -c "create extension citext;" 2>&1 | egrep 'NOTICE|ERROR'
NOTICE: Installing extension 'citext' from '/Users/dim/pgsql/exts/share/contrib/citext.sql', with user data
NOTICE: return type citext is only a shell
NOTICE: argument type citext is only a shell
NOTICE: return type citext is only a shell
NOTICE: argument type citext is only a shell
dim ~/dev/PostgreSQL/postgresql-extension psql -c "create extension cube;" 2>&1 | egrep 'NOTICE|ERROR'
NOTICE: Installing extension 'cube' from '/Users/dim/pgsql/exts/share/contrib/cube.sql', with user data
NOTICE: type "cube" is not yet defined
NOTICE: return type cube is only a shell
NOTICE: return type cube is only a shell
NOTICE: argument type cube is only a shell
dim ~/dev/PostgreSQL/postgresql-extension psql -c "create extension earthdistance;" 2>&1 | egrep 'NOTICE|ERROR'
NOTICE: Installing extension 'earthdistance' from '/Users/dim/pgsql/exts/share/contrib/earthdistance.sql', with user data
dim ~/dev/PostgreSQL/postgresql-extension psql -c "create extension fuzzystrmatch;" 2>&1 | egrep 'NOTICE|ERROR'
NOTICE: Installing extension 'fuzzystrmatch' from '/Users/dim/pgsql/exts/share/contrib/fuzzystrmatch.sql', with user data
dim ~/dev/PostgreSQL/postgresql-extension psql -c "create extension hstore;" 2>&1 | egrep 'NOTICE|ERROR'
NOTICE: Installing extension 'hstore' from '/Users/dim/pgsql/exts/share/contrib/hstore.sql', with user data
NOTICE: return type hstore is only a shell
NOTICE: argument type hstore is only a shell
NOTICE: return type hstore is only a shell
NOTICE: argument type hstore is only a shell
NOTICE: return type ghstore is only a shell
NOTICE: argument type ghstore is only a shell
dim ~/dev/PostgreSQL/postgresql-extension psql -c "create extension isn;" 2>&1 | egrep 'NOTICE|ERROR'
NOTICE: Installing extension 'isn' from '/Users/dim/pgsql/exts/share/contrib/isn.sql', with user data
NOTICE: type "ean13" is not yet defined
NOTICE: argument type ean13 is only a shell
NOTICE: type "isbn13" is not yet defined
NOTICE: argument type isbn13 is only a shell
NOTICE: type "ismn13" is not yet defined
NOTICE: argument type ismn13 is only a shell
NOTICE: type "issn13" is not yet defined
NOTICE: argument type issn13 is only a shell
NOTICE: type "isbn" is not yet defined
NOTICE: argument type isbn is only a shell
NOTICE: type "ismn" is not yet defined
NOTICE: argument type ismn is only a shell
NOTICE: type "issn" is not yet defined
NOTICE: argument type issn is only a shell
NOTICE: type "upc" is not yet defined
NOTICE: argument type upc is only a shell
dim ~/dev/PostgreSQL/postgresql-extension psql -c "create extension ltree;" 2>&1 | egrep 'NOTICE|ERROR'
NOTICE: Installing extension 'ltree' from '/Users/dim/pgsql/exts/share/contrib/ltree.sql', with user data
NOTICE: type "ltree" is not yet defined
NOTICE: argument type ltree is only a shell
NOTICE: type "lquery" is not yet defined
NOTICE: argument type lquery is only a shell
NOTICE: type "ltxtquery" is not yet defined
NOTICE: argument type ltxtquery is only a shell
NOTICE: type "ltree_gist" is not yet defined
NOTICE: argument type ltree_gist is only a shell

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

PS: Oh, a repalloc() bug now. Will fix later in the afternoon, \dx or
select * from pg_extensions(); crashes with more than 10 extensions
installed in the v4 patch. That's what I get for doing that on a
Saturday evening.

On Tue, Oct 19, 2010 at 9:07 AM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I don't see why.  I think the real action item here is to remove =>
>> from hstore.
>
> As input, consider that lots of extensions will create types that are
> only a shell at the moment of the CREATE TYPE, and for each of those
> types you will see the (potentially > 1000 lines long) whole SQL script
> dumped on the screen.
>
> In the following script, I've filtered out the scripts, but it's written
> out for each NOTICE message that you see:
>
> dim ~/dev/PostgreSQL/postgresql-extension psql -c "create extension citext;" 2>&1 | egrep 'NOTICE|ERROR'
> NOTICE:  Installing extension 'citext' from '/Users/dim/pgsql/exts/share/contrib/citext.sql', with user data
> NOTICE:  return type citext is only a shell
> NOTICE:  argument type citext is only a shell
> NOTICE:  return type citext is only a shell
> NOTICE:  argument type citext is only a shell

Well, perhaps it's reasonable to suppress NOTICE messages then. That
particular message could perhaps be quieted, but we probably don't
want to do that with every NOTICE that might occur (e.g. those that
you might get on table creation for sequences, indices, etc.).

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I don't see why. I think the real action item here is to remove =>
>> from hstore.

> As input, consider that lots of extensions will create types that are
> only a shell at the moment of the CREATE TYPE, and for each of those
> types you will see the (potentially > 1000 lines long) whole SQL script
> dumped on the screen.

Only if the script is intentionally noisy. The right fix here is
probably to bump up the min message level while running the script.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> Only if the script is intentionally noisy. The right fix here is
> probably to bump up the min message level while running the script.

You mean doing that from the SQL script itself (using SET) or in the
pg_execute_from_file() code? My guess is the former, but...

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

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> Only if the script is intentionally noisy. The right fix here is
>> probably to bump up the min message level while running the script.

> You mean doing that from the SQL script itself (using SET) or in the
> pg_execute_from_file() code? My guess is the former, but...

You could argue that either way I guess. The script knows what it
needs, but OTOH just about every extension there is will probably
be generating useless NOTICEs unless something is done, so maybe
the extension management code should take care of it for them.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> You could argue that either way I guess. The script knows what it
> needs, but OTOH just about every extension there is will probably
> be generating useless NOTICEs unless something is done, so maybe
> the extension management code should take care of it for them.

Either way is the key here too, so please find attached a revised (v5)
patch which will force log_min_messages and client_min_messages to
WARNING while the script is run.

v5 also contains the \dx bug fix about repalloc.

Please note that I didn't touch any contrib yet, so that hstore will
still dump its full script here:

dim=# create extension isn;
NOTICE: Installing extension 'isn' from '/Users/dim/pgsql/exts/share/contrib/isn.sql', with user data
CREATE EXTENSION
dim=# create extension hstore;
NOTICE: Installing extension 'hstore' from '/Users/dim/pgsql/exts/share/contrib/hstore.sql', with user data
WARNING: => is deprecated as an operator name
DETAIL: This name may be disallowed altogether in future versions of PostgreSQL.
CONTEXT: SQL statement "/* contrib/hstore/hstore.sql.in */

The script follows here. Maybe 9.1 is when to deprecate => as an
operator name in the hstore official extension? :)

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

Of course the git repo is still maintained for those prefering it:
http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=shortlog;h=refs/heads/extension

Excerpts from Dimitri Fontaine's message of dom oct 17 16:30:47 -0300 2010:

> The bulk of it is now short enough to be inlined in the mail, and if you
> have more comments I guess they'll be directed at this portion of the
> patch, so let's make it easy:
>
> /*
> * We abuse some internal knowledge from spi.h here. As we don't know
> * which queries are going to get executed, we don't know what to expect
> * as an OK return code from SPI_execute(). We assume that
> * SPI_OK_CONNECT, SPI_OK_FINISH and SPI_OK_FETCH are quite improbable,
> * though, and the errors are negatives. So a valid return code is
> * considered to be SPI_OK_UTILITY or anything from there.
> */
> SPI_connect();
> if (SPI_execute(query_string, false, 0) < SPI_OK_UTILITY)
> ereport(ERROR,
> (errcode(ERRCODE_DATA_EXCEPTION),
> errmsg("File '%s' contains invalid query", filename)));
> SPI_finish();

SPI_OK_FETCH is indeed improbable -- it's not used anywhere in the SPI
routines, and hasn't been for ages. SPI_OK_CONNECT and SPI_OK_FINNISH
are only issued by SPI_connect and SPI_finish, so presumably they can't
be returned by a script.

The error message wording needs some work; maybe
errmsg("file \"%s\" could not be executed", filename)

SPI_execute sets the query as errcontext, which is good; but apparently
there is no error position, which is bad. I'm not sure how feasible is
to fix this. (Not this patch's responsibility anyway.)

I happened to notice that there are several pieces of code that are
calling SPI_connect and SPI_finish without checking the return value,
notably the FTS code and xml.c.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Dimitri Fontaine's message of vie oct 15 16:15:23 -0300 2010:

> The cfparser patch didn't change, the current version is still v1.

Hmm, this needs some cleanup; the comments still talk about the old
function name; and about just the recovery.conf file.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> The error message wording needs some work; maybe
> errmsg("file \"%s\" could not be executed", filename)
[...]
> I happened to notice that there are several pieces of code that are
> calling SPI_connect and SPI_finish without checking the return value,
> notably the FTS code and xml.c.

Please find attached pg_execute_from_file.v4.patch with fixes. I've used
the usual error messages for SPI_connect() and finish this time.

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

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Hmm, this needs some cleanup; the comments still talk about the old
> function name; and about just the recovery.conf file.

Ah yes, thinking it's an easy patch is not helping. Please find attached
a revised version of it.

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

On Tue, Oct 19, 2010 at 12:09 PM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> You could argue that either way I guess.  The script knows what it
>> needs, but OTOH just about every extension there is will probably
>> be generating useless NOTICEs unless something is done, so maybe
>> the extension management code should take care of it for them.
>
> Either way is the key here too, so please find attached a revised (v5)
> patch which will force log_min_messages and client_min_messages to
> WARNING while the script is run.

It seems good to do this in the normal case, but (1) if
client_min_messages was already set higher than WARNING, we probably
should not lower it and (2) we might want to have a way to lower it
for troubleshooting purposes.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, Oct 19, 2010 at 12:09 PM, Dimitri Fontaine
> <dimitri(at)2ndquadrant(dot)fr> wrote:
>> Either way is the key here too, so please find attached a revised (v5)
>> patch which will force log_min_messages and client_min_messages to
>> WARNING while the script is run.

> It seems good to do this in the normal case, but (1) if
> client_min_messages was already set higher than WARNING, we probably
> should not lower it and (2) we might want to have a way to lower it
> for troubleshooting purposes.

I think the standard way of troubleshooting would be to run the
extension's script by hand, no? So while I agree with (1),
I'm not sure we need to sweat about (2).

regards, tom lane

Excerpts from Dimitri Fontaine's message of mar oct 19 13:09:47 -0300 2010:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> > You could argue that either way I guess. The script knows what it
> > needs, but OTOH just about every extension there is will probably
> > be generating useless NOTICEs unless something is done, so maybe
> > the extension management code should take care of it for them.
>
> Either way is the key here too, so please find attached a revised (v5)
> patch which will force log_min_messages and client_min_messages to
> WARNING while the script is run.

I think you should pstrdup the return value from GetConfigOption.
(As a matter of style, I'd name the local vars differently, so that they
don't show up when you search the code for the global vars; perhaps
"old_cmsgs" and "old_lmsgs" would do, for example.)

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Tue, Oct 19, 2010 at 9:33 PM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> 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.

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

* It has duplicated oids.
$ ./duplicate_oids
3627
3695
3696
3697
3698
3699

==== Tests for each contrib module ====
* 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.

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

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

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

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

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

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

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

--
Itagaki Takahiro

Excerpts from Itagaki Takahiro's message of mié oct 20 00:19:44 -0300 2010:

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

Lets rename the directory. We used to have a problem with that in CVS,
in Git it's no longer an issue.

Merging xml2 into core would be nice, but it's been attempted many
times and it's obvious that it's going to require a lot of work.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Wed, Oct 20, 2010 at 12:58 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
>> * xml2 module has a different extension name from the directory name.
>> The directory name is 'xml2', but the extension name is 'pgxml'.
>
> Lets rename the directory.

Hmmm, but we call it 'xml2' in the doc. There is no 'pgxml' at all in it.
http://developer.postgresql.org/pgdocs/postgres/xml2.html

However, I don't think we can change the module name because pg_upgrade
will fail if the module (.so) name was changed. So, it might be the
point of compromise to keep two names until we deprecate it completely.

--
Itagaki Takahiro

Sorry. I missed v5 patch and other new ones.
Some of the issues might have been already fixed in the new version.

On Wed, Oct 20, 2010 at 12:19 PM, Itagaki Takahiro
<itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> On Tue, Oct 19, 2010 at 9:33 PM, Dimitri Fontaine
>> Fixed in v4, attached.
>
> ==== Source code ====
> * It still has a warning.
> * It has duplicated oids.
>
> ==== Tests for each contrib module ====
> * Some modules dumps noisy NOTICE or WARNING messages.
> * Modules that should be added to shared_preload_libraries.
> * Modules that only have *.sql, but not *.sql.in.
> * Hyphen (-) in module name
> * xml2 module has a different extension name from the directory name.
> * spi module has multiple *.so, but only one *.control.
>
> ==== CREATE EXTENSION command ====
> * Environment could be modified by the installer script.
> * Non-ASCII characters in script

--
Itagaki Takahiro

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> It seems good to do this in the normal case, but (1) if
>> client_min_messages was already set higher than WARNING, we probably
>> should not lower it and (2) we might want to have a way to lower it
>> for troubleshooting purposes.
>
> I think the standard way of troubleshooting would be to run the
> extension's script by hand, no? So while I agree with (1),
> I'm not sure we need to sweat about (2).

Will go and fix (1), but like Tom I think (2) is handled by the
extension's author running pg_execute_from_file() rather than CREATE
EXTENSION for debugging purposes: then the settings are not changed.

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

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

On Wed, Oct 20, 2010 at 6:22 AM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> 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.

I would vote for overriding client_min_messages but not log_min_messages.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I would vote for overriding client_min_messages but not log_min_messages.

Well it defaults to WARNING so I see your point. Then again, we're
talking about hundreds of lines (3197 lines of isn, 531 lines for
hstore) of output per message, containing a script that you're not
maintaining. Do we really want that amount of extra logging?

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

On Wed, Oct 20, 2010 at 9:33 AM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I would vote for overriding client_min_messages but not log_min_messages.
>
> Well it defaults to WARNING so I see your point. Then again, we're
> talking about hundreds of lines (3197 lines of isn, 531 lines for
> hstore) of output per message, containing a script that you're not
> maintaining. Do we really want that amount of extra logging?

Well, my thought was that it makes sense to override the user's
logging preferences because, after all, if they wanted the extra
logging, they could run the script by hand. But what gets logged to
the system log is server policy, not user preference, and I'm
reluctant to think we should second-guess whatever the admin has
configured.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> writes:
> On Wed, Oct 20, 2010 at 12:58 PM, Alvaro Herrera
> <alvherre(at)commandprompt(dot)com> wrote:
>> Lets rename the directory.

> Hmmm, but we call it 'xml2' in the doc. There is no 'pgxml' at all in it.
> http://developer.postgresql.org/pgdocs/postgres/xml2.html

> However, I don't think we can change the module name because pg_upgrade
> will fail if the module (.so) name was changed. So, it might be the
> point of compromise to keep two names until we deprecate it completely.

If the extensions manager is dependent on the assumption that a module's
name matches the name of the directory it's built in, that assumption
needs to be removed anyway. There are too many use-cases where that
wouldn't hold, even if we try to force the standard contrib modules to
follow such a rule.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> If the extensions manager is dependent on the assumption that a module's
> name matches the name of the directory it's built in

It is not. There's some magic for simple cases so that contrib mostly
"works" with no editing, but of course, that's only mostly.

The version Itakagi-San worked with had not a single change to the
contrib sources, I've only begun to change things there (in v6) with the
spi case, that now produces 5 extensions control files out of a single
Makefile, thanks to this single new line:

CONTROL = $(addsuffix .control, $(MODULES))

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Oct 20, 2010 at 6:22 AM, Dimitri Fontaine
> <dimitri(at)2ndquadrant(dot)fr> wrote:
>> 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.

> I would vote for overriding client_min_messages but not log_min_messages.

Why? The problem with unreasonably bulky messages is just as
objectionable for the log.

regards, tom lane

Excerpts from Dimitri Fontaine's message of mié oct 20 07:22:53 -0300 2010:
> Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> writes:

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

Would it work to force a new transaction internally in CREATE EXTENSION,
and use the equivalent of SET LOCAL in the CREATE EXTENSION code?

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> If the extensions manager is dependent on the assumption that a module's
>> name matches the name of the directory it's built in

> It is not. There's some magic for simple cases so that contrib mostly
> "works" with no editing, but of course, that's only mostly.

> The version Itakagi-San worked with had not a single change to the
> contrib sources,

I don't think that "no changes to the makefiles" is a requirement,
or even a wish-list item, for this. I think it's perfectly reasonable
for the makefile to have to specify the module name; far better that
than that we get the name by some "magic" or other.

regards, tom lane

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Excerpts from Dimitri Fontaine's message of mi oct 20 07:22:53 -0300 2010:
>> 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?

> Would it work to force a new transaction internally in CREATE EXTENSION,

No, but maybe a savepoint?

> and use the equivalent of SET LOCAL in the CREATE EXTENSION code?

I had assumed that that was how he was doing it ...

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> I don't think that "no changes to the makefiles" is a requirement,
> or even a wish-list item, for this. I think it's perfectly reasonable
> for the makefile to have to specify the module name; far better that
> than that we get the name by some "magic" or other.

It seemed easy to get a reasonable approach requiring very few edits in
contribs so I favoured that. Now, it's still entirely possible to hand
adjust. Determining the extension name automatically from DATA_built or
DATA is only done where EXTENSION has not been provided, and guessing
the CONTROL file name from the EXTENSION name only occurs when CONTROL
has not been provided.

Of course if those changes (inlined there after) are seen as a bad idea,
then I will change all contrib Makefiles to add EXTENSION, EXTVERSION
(which always is MAJORVERSION here) and CONTROL (which almost always is
EXTENSION.control).

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

# create extension support
ifndef CONTROL
ifndef EXTENSION
ifdef DATA_built
EXTENSION = $(basename $(notdir $(firstword $(DATA_built))))
else ifdef DATA
EXTENSION = $(basename $(notdir $(firstword $(DATA))))
endif # DATA_built
endif # EXTENSION
ifndef EXTVERSION
EXTVERSION = $(MAJORVERSION)
endif
ifdef EXTENSION
CONTROL = $(EXTENSION).control
endif # EXTENSION
endif # CONTROL

control:
# create .control to keep track that we created the control file(s)
@for file in $(CONTROL); do \
test -f `basename $$file .control`.sql -a ! -f $$file && touch .control || true ; \
if [ -f .control ]; then \
if [ -n "$(EXTENSION)" ]; then \
(echo "name = '$(EXTENSION)'"; echo "version = '$(EXTVERSION)'") > $$file ; \
else \
(echo "name = '`basename $$file .control`'"; echo "version = '$(EXTVERSION)'") > $$file ; \
fi ; \
if [ -n "$(EXTCOMMENT)" ]; then echo "comment = '$(EXTCOMMENT)'" >> $$file ; fi ; \
fi ; \
done

install: all installdirs control
ifneq (,$(DATA)$(DATA_built)$(CONTROL))
@for file in $(addprefix $(srcdir)/, $(DATA)) $(DATA_built) $(CONTROL); do \
echo "$(INSTALL_DATA) $$file '$(DESTDIR)$(datadir)/$(datamoduledir)'"; \
$(INSTALL_DATA) $$file '$(DESTDIR)$(datadir)/$(datamoduledir)'; \
done
endif # DATA

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> and use the equivalent of SET LOCAL in the CREATE EXTENSION code?
>
> I had assumed that that was how he was doing it ...

I'm currently doing:
SetConfigOption("client_min_messages", "warning", PGC_SUSET, PGC_S_SESSION);

And then manually reverting to what was there before the command:
SetConfigOption("client_min_messages", old_cmsgs, PGC_SUSET, PGC_S_SESSION);

The thing is that CREATE EXTENSION can be part of a transaction, so even
SET LOCAL ain't going to work here, we need to reset before continuing
the transaction. I don't know that SET LOCAL is RESET after a savepoint,
so we would still need to care about that "by hand", right?

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

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> I don't think that "no changes to the makefiles" is a requirement,
>> or even a wish-list item, for this. I think it's perfectly reasonable
>> for the makefile to have to specify the module name; far better that
>> than that we get the name by some "magic" or other.

> It seemed easy to get a reasonable approach requiring very few edits in
> contribs so I favoured that. Now, it's still entirely possible to hand
> adjust. Determining the extension name automatically from DATA_built or
> DATA is only done where EXTENSION has not been provided,

That is simply a horrid idea. Just make it specify EXTENSION.

> and guessing
> the CONTROL file name from the EXTENSION name only occurs when CONTROL
> has not been provided.

Here, on the other hand, I'm wondering why have two variables at all.
Is there any sane use-case for the control file to not be named the same
as the extension? It seems like that would accomplish little except to
sow confusion.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> That is simply a horrid idea. Just make it specify EXTENSION.

Black magic it is, will remove in v7.

> Is there any sane use-case for the control file to not be named the same
> as the extension? It seems like that would accomplish little except to
> sow confusion.

The goal of the 3 variables EXTENSION, EXTVERSION, EXTCOMMENT is to
prepare the control file with 3 lines formatted variable = 'value'. If
you specify CONTROL instead, that should be the file name you're
providing directly.

It then grew up into being a directive to produce the right file set for
spi, but the simplest thing would be to hand prepare the files there. If
you agree with that, that's what I'll do in v7.

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

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> That is simply a horrid idea. Just make it specify EXTENSION.

And VERSION too, finally.

So any extension

>
>> and guessing
>> the CONTROL file name from the EXTENSION name only occurs when CONTROL
>> has not been provided.
>
> Here, on the other hand, I'm wondering why have two variables at all.
> Is there any sane use-case for the control file to not be named the same
> as the extension? It seems like that would accomplish little except to
> sow confusion.
>
> regards, tom lane

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

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> That is simply a horrid idea. Just make it specify EXTENSION.

And VERSION too. Sorry for previous email, fingers trying to work on
their own at the end of the day...

So, the idea is that $(EXTENSION) is a list of extensions you're
providing from the Makefile (most often, a list of one extension, but
contrib/spi is an exception here). Each extension in the list must have
a corresponding $EXTENSION.control file.

This control file contains at minimum a single line for the name of the
extension, but it's better already with a comment for users. I've been
filling them for our extensions, pasting from the documentation:

http://www.postgresql.org/docs/9/static/contrib.html

dim=# select name, version, installed as i, comment from pg_extensions;
name | version | i | comment
--------------------+----------+---+-------------------------------------------------------------------------
intarray | 9.1devel | f | one-dimensional arrays of integers: functions, operators, index support
adminpack | 9.1devel | f | Administrative functions for PostgreSQL
autoinc | 9.1devel | t | functions for autoincrementing fields
btree_gin | 9.1devel | f | GIN support for common types BTree operators
btree_gist | 9.1devel | t | GiST support for common types BTree operators
chkpass | 9.1devel | f | Store crypt()ed passwords
citext | 9.1devel | t | case-insensitive character string type
cube | 9.1devel | t | data type for representing multidimensional cubes
dblink | 9.1devel | f | connect to other PostgreSQL databases from within a database
dict_int | 9.1devel | f | example of an add-on dictionary template for full-text search
dict_xsyn | 9.1devel | f | example of an add-on dictionary template for full-text search
earthdistance | 9.1devel | t | calculating great circle distances on the surface of the Earth
fuzzystrmatch | 9.1devel | f | determine similarities and distance between strings
hstore | 9.1 | t | storing sets of key/value pairs
auto_username | 9.1devel | f | functions for tracking who changed a table
int_aggregate | 9.1devel | t | integer aggregator and an enumerator (obsolete)
isn | 9.1devel | t | data types for the international product numbering standards
lo | 9.1devel | f | managing Large Objects
ltree | 9.1devel | t | data type for hierarchical tree-like structure
moddatetime | 9.1devel | t | functions for tracking last modification time
pageinspect | 9.1devel | f | inspect the contents of database pages at a low level
pg_buffercache | 9.1devel | f | examine the shared buffer cache in real time
pg_freespacemap | 9.1devel | f | examine the free space map (FSM)
pg_stat_statements | 9.1devel | f | tracking execution statistics of all SQL statements executed
pg_trgm | 9.1devel | t | determine the similarity of text, with indexing support
pgcrypto | 9.1devel | f | cryptographic functions
pgrowlocks | 9.1devel | f | show row locking information for a specified table
pgstattuple | 9.1devel | f | obtain tuple-level statistics
refint | 9.1devel | f | functions for implementing referential integrity
seg | 9.1devel | f | data type for representing line segments, or floating point intervals
tablefunc | 9.1devel | t | various functions that return tables, including crosstab(text sql)
test_parser | 9.1devel | f | example of a custom parser for full-text search
timetravel | 9.1devel | f | functions for implementing time travel
tsearch2 | 9.1devel | f | backwards-compatible text search functionality (pre-8.3)
unaccent | 9.1devel | f | text search dictionary that removes accents
(35 rows)

(\dx+ also has the "Custom Variable Classes" column and that's too large
a paste then, so I've used the pg_extensions system view directly

Some extensions are missing here because they fail to compile on my
workstation where all the libs aren't installed --- ossp, xml2, etc
)

If you provide a $(VERSION) variable, then a line in the control file is
automatically added at make install (version = '$(VERSION)'), in order
to make life easier for extension authors.

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

Dunno if erroneous previous mail did contain the attachement, here it
is.

On Oct 20, 2010, at 3:12 PM, Dimitri Fontaine wrote:

> So, the idea is that $(EXTENSION) is a list of extensions you're
> providing from the Makefile (most often, a list of one extension, but
> contrib/spi is an exception here). Each extension in the list must have
> a corresponding $EXTENSION.control file.
>
> This control file contains at minimum a single line for the name of the
> extension, but it's better already with a comment for users. I've been
> filling them for our extensions, pasting from the documentation:

Might I suggest instead a META.json file like PGXN requires? Here's a simple example:

{
"name": "pair",
"abstract": "A key/value pair data type",
"version": "0.1.0",
"maintainer": "David E. Wheeler <david(at)justatheory(dot)com>",
"license": "postgresql",
}

They can have a lot more information, too. Her's the one I actually shipped with pair:

http://github.com/theory/kv-pair/blob/master/META.json

The meta spec is here:

http://github.com/theory/pgxn/wiki/PGXN-Meta-Spec

Anyway, the point is that it might be useful for us to sync on this format. I went with JSON for a few reasons:

* CPAN is switching to it (from YAML)
* It's extremely widespread
* It's useful for ac-hoc REST-style requests
* The format will likely be in 9.1.

Thoughts?

BTW, really excited that you're finally getting EXTENSION done, Dim. This is going to be *great* for PostgreSQL developers. I'll have to work it into my talk at West.

https://www.postgresqlconference.org/content/building-and-distributing-postgresql-extensions-without-learning-c

Best,

David

On Thu, Oct 21, 2010 at 7:12 AM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> This control file contains at minimum a single line for the name of the
> extension, but it's better already with a comment for users. I've been
> filling them for our extensions, pasting from the documentation:
>
>        name        | version  |
> --------------------+----------+
>  fuzzystrmatch      | 9.1devel |
>  hstore             | 9.1      |

Why does only hstore have version '9.1'? Any other modules have '9.1devel'.

> If you provide a $(VERSION) variable, then a line in the control file is
> automatically added at make install (version = '$(VERSION)'), in order
> to make life easier for extension authors.

In v7, a line of "version = '...'" is added at "make install", and removed
at "make clean". Also, if we runs "make install" multiple times, version
lines are added repeatedly. I don't think they are good ideas; we should
not modify source codes stored in git repo when we build them.

How about having *.control.in and replace magic keywords in them at "make"?
"make install" won't modify files at all, and "make clean" just removes
*.control. It is the way we're using for *.sql.in and MODULE_PATHNAME.

> Some extensions are missing here because they fail to compile on my
> workstation where all the libs aren't installed --- ossp, xml2, etc

I found xml2/pgxml.control should have 'pgxml" for the name.

--
Itagaki Takahiro

On Thu, Oct 21, 2010 at 8:14 AM, David E. Wheeler <david(at)kineticode(dot)com> wrote:
> Might I suggest instead a META.json file like PGXN requires?

I think JSON is also reasonable, but one of the problem to use JSON format is
we cannot apply the extension patch until JSON patch has been applied ;-)

BTW, does anyone needs JSON formatted configuration files for other purposes?
There might be some discussions in "Standby registration" or "Configuring
synchronous replication" threads. Module control files are so simple that
they don't always require JSON format, such as nested variable. But
configuration files for replication might be more complex. If needed,
it would be reasonable to introduce a JSON reader.

--
Itagaki Takahiro

Excerpts from Itagaki Takahiro's message of jue oct 21 00:01:59 -0300 2010:
> On Thu, Oct 21, 2010 at 8:14 AM, David E. Wheeler <david(at)kineticode(dot)com> wrote:
> > Might I suggest instead a META.json file like PGXN requires?
>
> I think JSON is also reasonable, but one of the problem to use JSON format is
> we cannot apply the extension patch until JSON patch has been applied ;-)

What's wrong with sticking to Makefile syntax? Are we intending to
build a JSON parser in GNU make perchance?

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Oct 20, 2010, at 9:58 PM, Alvaro Herrera wrote:

> What's wrong with sticking to Makefile syntax? Are we intending to
> build a JSON parser in GNU make perchance?

That metadata isn't *for* make, is it?

D

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
> Might I suggest instead a META.json file like PGXN requires? Here's a
> simple example:

I don't see what it buys us in this very context. The main thing here to
realize is that I wrote about no code to parse the control file. I don't
think the extension patch should depend on the JSON-in-core patch.

Now, once we have JSON and before the release, I guess given a good
reason to have much more complex configuration files that don't look at
all like postgresql.conf, we could revisit.

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

Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> writes:
> Why does only hstore have version '9.1'? Any other modules have
> '9.1devel'.

It's the only contrib that's not using PGXS but instead directly
includes $(top_builddir)/src/Makefile.global, and that file contains the
following:

# PostgreSQL version number
VERSION = 9.1devel
MAJORVERSION = 9.1

Then in contrib/hstore/Makefile we have VERSION = $(MAJORVERSION) and
that's what get used to build the control file.

We could decide to "fix" hstore Makefile to look more like all the other
ones in contrib, but I don't think that directly falls in the scope of
the extension's patch. I could provide a separate patch to this end,
that said.

> How about having *.control.in and replace magic keywords in them at "make"?
> "make install" won't modify files at all, and "make clean" just removes
> *.control. It is the way we're using for *.sql.in and MODULE_PATHNAME.

Thanks a lot for that idea, it's obvious now that you say it. Maybe I
should stop working (or posting patches) that late in the evening :)

It's done in the v8 patch, as always based on the git repository:
http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=shortlog;h=refs/heads/extension

As I think we're now on the right track about PGXS integration of the
feature, I went ahead and added documentation about EXTENSION and
VERSION.

> I found xml2/pgxml.control should have 'pgxml" for the name.

Well really I'm not sure about this one. As Tom said there's no need for
the extension's name to be the same as its directory or even file
names. That's why there's a 'name' property in the control file, after
all.

Now it could be that we want to clean this up in contrib, but that's out
of the extension's patch scope in my mind.

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

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> writes:
>> Why does only hstore have version '9.1'? Any other modules have
>> '9.1devel'.

> It's the only contrib that's not using PGXS but instead directly
> includes $(top_builddir)/src/Makefile.global,

... well, that's just a bug in hstore. *All* the contrib modules
should be using PGXS, unless they have a damn good reason not to;
which is not apparent for hstore.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> ... well, that's just a bug in hstore. *All* the contrib modules
> should be using PGXS, unless they have a damn good reason not to;
> which is not apparent for hstore.

Here's a patch.
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On Oct 21, 2010, at 12:33 AM, Dimitri Fontaine wrote:

> I don't see what it buys us in this very context. The main thing here to
> realize is that I wrote about no code to parse the control file. I don't
> think the extension patch should depend on the JSON-in-core patch.
>
> Now, once we have JSON and before the release, I guess given a good
> reason to have much more complex configuration files that don't look at
> all like postgresql.conf, we could revisit.

Sure. The reason to do it, though, is so that extension authors can create just one metadata file, instead of two (or three, if one must also put such data into the Makefile).

Best,

David

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
> Sure. The reason to do it, though, is so that extension authors can create
> just one metadata file, instead of two (or three, if one must also put such
> data into the Makefile).

That's a good idea, but my guess is that the implementation cost of
supporting the control format in your perl infrastructure is at least an
order of magnitude lower than the cost for me to support your current
JSON file format, so I lean towards you having an automated way to fill
in the json file from the control one...

The Makefile supports $(VERSION) because chances are it's already there
(think packaging or tarball release targets). Having yet another place
where to manually maintain a version number ain't appealing.

In the latest patch, though, the only other thing you find in the
Makefile about the extension is its basename, which must be the one of
both the .control and the .sql files. And it's possible for $(EXTENSION)
to be a list of them, too, because of contrib/spi.

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

On Oct 21, 2010, at 8:12 AM, Dimitri Fontaine wrote:

> That's a good idea, but my guess is that the implementation cost of
> supporting the control format in your perl infrastructure is at least an
> order of magnitude lower than the cost for me to support your current
> JSON file format, so I lean towards you having an automated way to fill
> in the json file from the control one...

Well, it *will* be easier. Eventually. Right now, the file has to be edited by hand. Which I can tell you from experience is rather…error-prone.

Anyway, I wouldn't push for a JSON file format until a parser was just there for you to use without too much trouble.

> The Makefile supports $(VERSION) because chances are it's already there
> (think packaging or tarball release targets). Having yet another place
> where to manually maintain a version number ain't appealing.

Be aware that PGXS sets a $(VERSION) variable already, so you'll need to use another name, I think, to guard from conflicts. This is in Makefile.global:

VERSION = 9.0.1
MAJORVERSION = 9.0

Maybe use EXTVERSION? You don't want to overwrite the core version because a makefile author could use it to change the build (pgTAP does this, for example).

> In the latest patch, though, the only other thing you find in the
> Makefile about the extension is its basename, which must be the one of
> both the .control and the .sql files. And it's possible for $(EXTENSION)
> to be a list of them, too, because of contrib/spi.

Right, that makes sense.

Best,

David

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
> Be aware that PGXS sets a $(VERSION) variable already, so you'll need
> to use another name, I think, to guard from conflicts. This is in
> Makefile.global:

Of course that's not a problem for contribs, and I used EXTVERSION in a
previous version of the patch. I guess I will get back to use
$(EXTVERSION) in the Makefile next time I have to produce a patch.

This part of the problem didn't receive much thoughts yet, and it shows
up. About the rest of the patch have been in my head for months, I
expect less problems there...

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

Excerpts from Dimitri Fontaine's message of jue oct 21 12:53:18 -0300 2010:

> This part of the problem didn't receive much thoughts yet, and it shows
> up. About the rest of the patch have been in my head for months, I
> expect less problems there...

Keep on it. You're doing a terrific job.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Excerpts from Dimitri Fontaine's message of jue oct 21 12:53:18 -0300 2010:
>> This part of the problem didn't receive much thoughts yet, and it shows
>> up. About the rest of the patch have been in my head for months, I
>> expect less problems there...
>
> Keep on it. You're doing a terrific job.

Thank you very much, such encouragements are highly appreciated :)

Of course, you what that means? Yes, another version of the patch, that
will build the control file out of the control.in at build time rather
than install time, and that's back to using EXTVERSION both in the
Makefile and in the .control.in file.

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

On Thu, Oct 21, 2010 at 11:12 AM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> "David E. Wheeler" <david(at)kineticode(dot)com> writes:
>> Sure. The reason to do it, though, is so that extension authors can create
>> just one metadata file, instead of two (or three, if one must also put such
>> data into the Makefile).
>
> That's a good idea, but my guess is that the implementation cost of
> supporting the control format in your perl infrastructure is at least an
> order of magnitude lower than the cost for me to support your current
> JSON file format, so I lean towards you having an automated way to fill
> in the json file from the control one...

Ah, truth to power. :-)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Oct 22, 2010 at 1:31 AM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Of course, you what that means? Yes, another version of the patch, that
> will build the control file out of the control.in at build time rather
> than install time, and that's back to using EXTVERSION both in the
> Makefile and in the .control.in file.

Here are detailed report for v9 patch.

* extension.v9.patch.gz seems to contain other changes in the repo.
Did you use old master to get the diff?

* 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

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

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

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

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.

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

--
Itagaki Takahiro

Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)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

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> 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)

That's done now and for those paying attention, of course those examples
won't need to add a script property in their control files, as soon as
we both scan the SHAREDIR to find the proper control file and that the
default script is ${name}.sql, which is what's used everywhere in our
contribs.

New patch to follow later, including the other modifications on the
table (error message, script file encoding, etc).

Note that the control files being parsed to check their name property
against the extension name that's been asked by the user, I think that
means they have to be in a fixed known encoding.

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

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> That's done now and for those paying attention, of course those examples
> won't need to add a script property in their control files, as soon as
> we both scan the SHAREDIR to find the proper control file and that the
> default script is ${name}.sql, which is what's used everywhere in our
> contribs.

Ok, that's confusing. The ${name} above is the name sans extension of
the control file (`basename foo.control .control`), not the name of the
extension. That's why it works without having to override the script
property in the contribs control files: _int.control and _int.sql are
the files for the extension named 'intarray'.

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

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

Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> writes:
> 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

https://commitfest.postgresql.org/action/patch_view?id=404

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

Excerpts from Dimitri Fontaine's message of vie oct 22 10:43:58 -0300 2010:
> Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> writes:

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

Hang on. Did I miss something? Why doesn't the control file name equal
the extension name? I think the idea that you have to scan the whole
share directory and parse all control files to find the one you need is
a bit strange.

It seems more sensible to allow the variation to occur in the Makefile,
i.e. _int/Makefile should contain
EXTENSION=intarray

Was this discussed previously? If so, why was this idea rejected?

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

This seems sensible ... at least as sensible as it is to allow COPY to
specify the encoding.

> > CREATE EXTENSION could have optional ENCODING option.
> > => CREATE EXTENSION name [ ENCODING 'which' ]

This seems like putting the burden on the user on getting the command
right. That seems prone to failure.

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

That makes more sense.

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

I doubt this is necessary. If the DBA needs to override it (why?), it's
possible to edit the file.

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

I think it is OK to have the control files be pure ASCII (this doesn't
mean they are in SQL_ASCII though). The problems will start when we
decide to allow translations for extension descriptions. But we can
leave that for another day.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Hang on. Did I miss something? Why doesn't the control file name equal
> the extension name? I think the idea that you have to scan the whole
> share directory and parse all control files to find the one you need is
> a bit strange.

Yes. It's horrid for performance, and it's worse for understandability,
and there's no obvious benefit to set against those. Please let's make
the rule that the control file name equals the extension name.

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

> This seems sensible ... at least as sensible as it is to allow COPY to
> specify the encoding.

But then you have to figure out what encoding it is, and you have to
know that before you start reading the file. I think a better idea
would be to do the same thing we did for text search config files:
mandate that these files have to be in utf8.

> I think it is OK to have the control files be pure ASCII (this doesn't
> mean they are in SQL_ASCII though). The problems will start when we
> decide to allow translations for extension descriptions. But we can
> leave that for another day.

Well, we can see the issue now, and anyway who's to say an extension
might not want to load up some non-ASCII data?

regards, tom lane

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>> 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.
>
> Hang on. Did I miss something? Why doesn't the control file name equal
> the extension name? I think the idea that you have to scan the whole
> share directory and parse all control files to find the one you need is
> a bit strange.

The default behavior is to have the control file name the same as the
extension's file name, so there's no directory scanning in this case. It
so happen that the existing "extensions" are not currently following the
rule, even when we limit ourselves to contrib, so I adjusted the code to
be able to be more friendly.

Performance wise, it only concerns the command CREATE EXTENSION and the
queries to the pg_extensions system view, that's used in \dx. Does not
seem that critical.

Now, if we want to do it the other way round and force extension name to
be the filename, we will have to live with all the restrictions that
filename imposes and that are not the same depending on the OS and the
filesystem, I think, and with systems where we have no way to know what
is the filesystem encoding. Am I wring in thinking that this might be a
problem?

If that's not a problem and we want to have the extension name into the
control file name, then I'll go remove the name property there.

> It seems more sensible to allow the variation to occur in the Makefile,
> i.e. _int/Makefile should contain
> EXTENSION=intarray
>
> Was this discussed previously? If so, why was this idea rejected?

It's already possible to do this. But the contrib uses _int.sql.in and
MODULE_big = _int and I didn't change that. I had the control file named
the same as the script file, but with the current implementation we can
change that (there's a script property in the control file).

>> > * 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 )
>
> This seems sensible ... at least as sensible as it is to allow COPY to
> specify the encoding.

Well why not, for convenience, but you can surround the call to this
function with SET client_encoding and get the same behaviour. Will add
the argument if required, though.

>> > CREATE EXTENSION could have optional ENCODING option.
>> > => CREATE EXTENSION name [ ENCODING 'which' ]
>
> This seems like putting the burden on the user on getting the command
> right. That seems prone to failure.

Given that the control file now supports an encoding parameter, I don't
see what this is good for, but I might be missing something obvious for
people working with different encodings etc. Japan seems to be quite
special as far as encodings are concerned.

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

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> Yes. It's horrid for performance, and it's worse for understandability,
> and there's no obvious benefit to set against those. Please let's make
> the rule that the control file name equals the extension name.

Performance… well if you say that CREATE EXTENSION performance prevails
on its flexibility, let's say that I didn't expect it. See also my
previous mail for details (directory scanning only happens for those
extensions having chosen not to name their control file sensibly) and
the filesystem/OS problems (are they real?).

> But then you have to figure out what encoding it is, and you have to
> know that before you start reading the file. I think a better idea
> would be to do the same thing we did for text search config files:
> mandate that these files have to be in utf8.

Agreed, if as I understand it you're talking about the control file
itself, which supports an 'encoding' parameter that applies to the
SQL script.

>> I think it is OK to have the control files be pure ASCII (this doesn't
>> mean they are in SQL_ASCII though). The problems will start when we
>> decide to allow translations for extension descriptions. But we can
>> leave that for another day.
>
> Well, we can see the issue now, and anyway who's to say an extension
> might not want to load up some non-ASCII data?

The case is covered with the 'script' and 'encoding' parameters present
in v10 I think. If we force the control file itself to be UTF8 then we
can allow translations embedded in the file, it seems.

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

Excerpts from Dimitri Fontaine's message of vie oct 22 12:25:07 -0300 2010:

> Now, if we want to do it the other way round and force extension name to
> be the filename, we will have to live with all the restrictions that
> filename imposes and that are not the same depending on the OS and the
> filesystem, I think, and with systems where we have no way to know what
> is the filesystem encoding. Am I wring in thinking that this might be a
> problem?

I don't see a problem limiting extension names to use only ASCII chars,
and a subset of those, at that. They're just names. If you want to get
fancy you can use the description.

> If that's not a problem and we want to have the extension name into the
> control file name, then I'll go remove the name property there.
>
> > It seems more sensible to allow the variation to occur in the Makefile,
> > i.e. _int/Makefile should contain
> > EXTENSION=intarray
> >
> > Was this discussed previously? If so, why was this idea rejected?
>
> It's already possible to do this. But the contrib uses _int.sql.in and
> MODULE_big = _int and I didn't change that. I had the control file named
> the same as the script file, but with the current implementation we can
> change that (there's a script property in the control file).

I think it makes more sense to change the script name in the control
file. That way you never need to scan anything. As for this being
seldom used, consider the case where the user mistypes the extension name.
You will be scanning the directory without good reason.

> >> > * 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 )
> >
> > This seems sensible ... at least as sensible as it is to allow COPY to
> > specify the encoding.
>
> Well why not, for convenience, but you can surround the call to this
> function with SET client_encoding and get the same behaviour. Will add
> the argument if required, though.

I don't think it is. In fact, it seems better to leave it out. CREATE
EXTENSION will have its own mechanism for specifying the encoding (which
it'll obtain from the control file), so I don't see the need.

> >> > CREATE EXTENSION could have optional ENCODING option.
> >> > => CREATE EXTENSION name [ ENCODING 'which' ]
> >
> > This seems like putting the burden on the user on getting the command
> > right. That seems prone to failure.
>
> Given that the control file now supports an encoding parameter, I don't
> see what this is good for, but I might be missing something obvious for
> people working with different encodings etc. Japan seems to be quite
> special as far as encodings are concerned.

Seems we're both arging the say way, but neither of us is Japanese ;-)

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Alvaro Herrera's message of vie oct 22 13:17:41 -0300 2010:

> > Given that the control file now supports an encoding parameter, I don't
> > see what this is good for, but I might be missing something obvious for
> > people working with different encodings etc. Japan seems to be quite
> > special as far as encodings are concerned.
>
> Seems we're both arging the say way, but neither of us is Japanese ;-)

Argh. "arguing the same way". Blame it on typing on a high-latency
link :-(

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Excerpts from Dimitri Fontaine's message of vie oct 22 12:25:07 -0300 2010:
>
>> Now, if we want to do it the other way round and force extension name to
>> be the filename, we will have to live with all the restrictions that
>> filename imposes and that are not the same depending on the OS and the
>> filesystem, I think, and with systems where we have no way to know what
>> is the filesystem encoding. Am I wring in thinking that this might be a
>> problem?
>
> I don't see a problem limiting extension names to use only ASCII chars,
> and a subset of those, at that. They're just names. If you want to get
> fancy you can use the description.

So extension names are forced into English? I would live with that, I
just don't find the answer friendly to the users.

I'd think we can live with some directory scanning that only happens
when installing extensions, or checking available extensions. All of
which being superuser only.

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

On 10/22/2010 12:30 PM, Dimitri Fontaine wrote:
> Alvaro Herrera<alvherre(at)commandprompt(dot)com> writes:
>> Excerpts from Dimitri Fontaine's message of vie oct 22 12:25:07 -0300 2010:
>>
>>> Now, if we want to do it the other way round and force extension name to
>>> be the filename, we will have to live with all the restrictions that
>>> filename imposes and that are not the same depending on the OS and the
>>> filesystem, I think, and with systems where we have no way to know what
>>> is the filesystem encoding. Am I wring in thinking that this might be a
>>> problem?
>> I don't see a problem limiting extension names to use only ASCII chars,
>> and a subset of those, at that. They're just names. If you want to get
>> fancy you can use the description.
> So extension names are forced into English? I would live with that, I
> just don't find the answer friendly to the users.

ASCII is not the same thing as English. You can create the names in Pig
Latin or Redneck if you want. It's the charset that's being restricted
here, and we restrict many more things than this to ASCII.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> ASCII is not the same thing as English. You can create the names in Pig
> Latin or Redneck if you want. It's the charset that's being restricted here,
> and we restrict many more things than this to ASCII.

I'd like to read Itagaki's opinion here, will wait :)

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

Excerpts from Dimitri Fontaine's message of vie oct 22 13:30:22 -0300 2010:

> So extension names are forced into English? I would live with that, I
> just don't find the answer friendly to the users.

Well, things like pgfoundry project names are also restricted AFAICT and
I don't think anyone has a problem with that.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Well, things like pgfoundry project names are also restricted AFAICT and
> I don't think anyone has a problem with that.

For things you publish, sure. But extensions will also handle in-house
developments of functions and other in-database API-like stuff, in fact
any SQL script you have that you don't want to maintain in the database
dumps is an extension.

So it's broader than just public Open Source projects.

Anyway, my point is that by default there's no directory scanning: the
lookup is first directed towards ${extension-name}.control, of
course. Only when this file does not exists or its name property is
different from the extension name do we get to scan the directory, and
we stop as soon as we find the .control file with the right name in it.

So I think it's a good compromise, and as it's superuser-only I would
think it's acceptable as-is. Apparently it's not, which ain't the end of
the world but an unexpected surprise for me. And when I don't
understand, I tend to insist until I do or until I resign, whichever
comes first, but you would know that by now :)

I'll go rework the patch sometime later to drop the name from the
control file, but that also means fixing several contrib modules by
changing their file names, operation for which the project has no policy
yet as far as I understand (we used CVS before).

For information, when we talk about performance problem, please note
that on my workstation with a default setup (not that it's important
here) we're talking about 86,420 ms for a loop of 100
perform * from pg_extensions;

That displays 36 extensions and needs to parse their files twice and for
some of them need to scan the directory and parse other extension
control files before to get to the right one. Average less than 1ms to
do all that on my workstation, and typically less than 3ms if you
include psql side of things.

Superuser only. To manage extensions, nothing to do with live load or
user queries. But if you say performance is important here and 3ms to
display all available extensions sucks, so be it.

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

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> For information, when we talk about performance problem, please note
> that on my workstation with a default setup (not that it's important
> here) we're talking about 86,420 ms for a loop of 100
> perform * from pg_extensions;

That's right, but

> That displays 36 extensions and needs to parse their files twice and for
> some of them need to scan the directory and parse other extension
> control files before to get to the right one. Average less than 1ms to
> do all that on my workstation, and typically less than 3ms if you
> include psql side of things.

That's not what happens, the pg_extensions() SRF will scan the directory
once and parse each control file once, of course. I'm tired enough to
mix the behaviour of finding the control file given *one* extension name
at CREATE EXTENSION time with listing all available extensions. Sorry
for the noise.

In my mind though, the baseline remains the same. Now I will have a
sleep and prepare for holidays, in some meaningful order…

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

Excerpts from Dimitri Fontaine's message of vie oct 22 16:21:14 -0300 2010:

> So I think it's a good compromise, and as it's superuser-only I would
> think it's acceptable as-is. Apparently it's not, which ain't the end of
> the world but an unexpected surprise for me. And when I don't
> understand, I tend to insist until I do or until I resign, whichever
> comes first, but you would know that by now :)

Yeah, well, this is just my personal opinion. Others are welcome to
chime in.

> I'll go rework the patch sometime later to drop the name from the
> control file, but that also means fixing several contrib modules by
> changing their file names, operation for which the project has no policy
> yet as far as I understand (we used CVS before).

Change what file names? You lost me there. I thought the extension
name was going to be equal to the control file name, and said control
file doesn't exist yet, so you don't need to rename any existant file.
Am I confusing something?

That said, I don't think there's any reason now to stop a committer from
renaming files (as long as this has been previously discussed and agreed
to, just like any other commit).

> Superuser only. To manage extensions, nothing to do with live load or
> user queries. But if you say performance is important here and 3ms to
> display all available extensions sucks, so be it.

To be honest I'm not all that concerned with raw performance here. I
just think that scanning a directory is a strange way for the search to
behave.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Fri, Oct 22, 2010 at 4:02 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
> That said, I don't think there's any reason now to stop a committer from
> renaming files (as long as this has been previously discussed and agreed
> to, just like any other commit).

Yeah - what is the feasibility of cleaning up the things where there
are naming inconsistencies right now?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Excerpts from Robert Haas's message of vie oct 22 17:07:10 -0300 2010:
> On Fri, Oct 22, 2010 at 4:02 PM, Alvaro Herrera
> <alvherre(at)commandprompt(dot)com> wrote:
> > That said, I don't think there's any reason now to stop a committer from
> > renaming files (as long as this has been previously discussed and agreed
> > to, just like any other commit).
>
> Yeah - what is the feasibility of cleaning up the things where there
> are naming inconsistencies right now?

We're in Git-land now. Why should we worry unnecessarily about old
restrictions? It wasn't a policy, just a tool limitation.

Do you have concrete proposals? If so please start a new thread :-)

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

> Yeah - what is the feasibility of cleaning up the things where there
> are naming inconsistencies right now?

Easy. Heck, the only reason we didn't do it 2 years ago was that we
were waiting for extensions before bothering.

Go Dimitri! Yaaay.

--
-- Josh Berkus
PostgreSQL Experts Inc.
http://www.pgexperts.com

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> Anyway, my point is that by default there's no directory scanning: the
> lookup is first directed towards ${extension-name}.control, of
> course. Only when this file does not exists or its name property is
> different from the extension name do we get to scan the directory, and
> we stop as soon as we find the .control file with the right name in it.

This seems (a) way overcomplicated, and (b) sorely in need of a
unique-index mechanism.

Please just use the filenames and have done. This is lily-gilding
at its finest. People who want to use non-ascii filenames are welcome
to do so, and deal with any ensuing fallout themselves.

regards, tom lane

On Sat, Oct 23, 2010 at 5:26 AM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>> Yeah - what is the feasibility of cleaning up the things where there
>> are naming inconsistencies right now?
>
> Easy.  Heck, the only reason we didn't do it 2 years ago was that we
> were waiting for extensions before bothering.

We could rename the module name, directory, and documentation path,
but could not rename .so files because pg_restore would fail to restore
functions written in C because they contains previous name of .so files.

The issue will be solved by the EXTENSION patch, but the feature cannot
be used to upgrade to 9.1 from older versions, no?

--
Itagaki Takahiro

Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> writes:
> On Sat, Oct 23, 2010 at 5:26 AM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>>> Yeah - what is the feasibility of cleaning up the things where there
>>> are naming inconsistencies right now?
>>
>> Easy. Heck, the only reason we didn't do it 2 years ago was that we
>> were waiting for extensions before bothering.

> We could rename the module name, directory, and documentation path,
> but could not rename .so files because pg_restore would fail to restore
> functions written in C because they contains previous name of .so files.

> The issue will be solved by the EXTENSION patch, but the feature cannot
> be used to upgrade to 9.1 from older versions, no?

Hmm, there seems to be some confusion here as to whether we are talking
about "move the source code around" or "change user-visible module names".
I'm distinctly not for the latter, but I'm not sure if that's what Josh
meant.

regards, tom lane

Excerpts from Alvaro Herrera's message of vie oct 22 17:02:22 -0300 2010:
> Excerpts from Dimitri Fontaine's message of vie oct 22 16:21:14 -0300 2010:

> > I'll go rework the patch sometime later to drop the name from the
> > control file, but that also means fixing several contrib modules by
> > changing their file names, operation for which the project has no policy
> > yet as far as I understand (we used CVS before).
>
> Change what file names? You lost me there. I thought the extension
> name was going to be equal to the control file name, and said control
> file doesn't exist yet, so you don't need to rename any existant file.
> Am I confusing something?

Hmm, after reading the latest blog post, it seems that the patch
requires that the control file is equal to the .sql install script. Is
this the case? I don't see a reason for this requirement; why not
simply have a line for the install script in the control file? That
way, you don't need to rename the .sql files.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Alvaro Herrera's message of lun oct 25 10:37:22 -0300 2010:
> Excerpts from Alvaro Herrera's message of vie oct 22 17:02:22 -0300 2010:
>
> > > I'll go rework the patch sometime later to drop the name from the
> > > control file, but that also means fixing several contrib modules by
> > > changing their file names, operation for which the project has no policy
> > > yet as far as I understand (we used CVS before).
> >
> > Change what file names? You lost me there. I thought the extension
> > name was going to be equal to the control file name, and said control
> > file doesn't exist yet, so you don't need to rename any existant file.
> > Am I confusing something?
>
> Hmm, after reading the latest blog post, it seems that the patch
> requires that the control file is equal to the .sql install script. Is
> this the case? I don't see a reason for this requirement; why not
> simply have a line for the install script in the control file? That
> way, you don't need to rename the .sql files.

Ah, some reading of the patch reveals that the "script" defaults to the
control file name, but it can be overridden.

I noticed that you're using ExclusiveLock when creating an extension,
citing the handling of the global variable create_extension for this.
There are two problems here: one is that you're releasing the lock way
too early: if you wanted this to be effective, you'd need to hold on to
the lock until after you've registered the extension.

The other is that there is no need for this at all, because this backend
cannot be concurrently running another CREATE EXTENSION comand, and
this is a backend-local variable. So there's no point.

Why palloc create_extension every time? Isn't it better to initialize
it properly and have a boolean value telling whether it's to be used?
Also, if an extension fails partway through creation, the var will be
left set. I think you need a PG_TRY block to reset it.

(I find the repeated coding pattern that tests create_extension for
NULL-ness before calling recordDependencyOn a bit awkward; maybe hide it
in a function or macro? But then maybe that's just me. Also, why
palloc it? Seems better to have it static. Notice your new calls to
recordDependencyOn are the only ones with operands not using the &
operator.)

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Dimitri Fontaine's message of vie oct 22 16:43:56 -0300 2010:
> Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> > For information, when we talk about performance problem, please note
> > that on my workstation with a default setup (not that it's important
> > here) we're talking about 86,420 ms for a loop of 100
> > perform * from pg_extensions;

BTW it strikes me that it would be easier on the code that there were
just a couple of simple functions, one returning the list of installed
extensions and another one returning the list of installable
extensions. The rest of SRF functions needn't be implemented in C, you
could implement them in SQL instead by joining to pg_depend and whatnot.

Also, PFA a couple of minor fixes.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Le 25 oct. 2010 à 17:26, Alvaro Herrera a écrit :
> Ah, some reading of the patch reveals that the "script" defaults to the
> control file name, but it can be overridden.

Yes, that's new in v10. In v11 I've kept that and removed the name property in the control file, so that we have:

cat contrib/intarray/intarray.control.in
# intarray
comment = 'one-dimensional arrays of integers: functions, operators, index support'
version = 'EXTVERSION'
script = '_int.sql'

> I noticed that you're using ExclusiveLock when creating an extension,
> citing the handling of the global variable create_extension for this.
> There are two problems here: one is that you're releasing the lock way
> too early: if you wanted this to be effective, you'd need to hold on to
> the lock until after you've registered the extension.
>
> The other is that there is no need for this at all, because this backend
> cannot be concurrently running another CREATE EXTENSION comand, and
> this is a backend-local variable. So there's no point.

I wanted to protect from another backend trying to create the same extension at the same time. So the critical path is the inserting into the catalog. I now see I failed to include the duplicate object check into the critical path, when I added that later.

Do you confirm protecting the insertion in the catalog is not worthy of special locking? To get proper locking requires some more thinking than I did put in, but if you say I'd better remove it...

> Why palloc create_extension every time? Isn't it better to initialize
> it properly and have a boolean value telling whether it's to be used?
> Also, if an extension fails partway through creation, the var will be
> left set. I think you need a PG_TRY block to reset it.

Good catches. I'm still uneasy with which memory context what allocation belongs too, hence the palloc()ing here.

> (I find the repeated coding pattern that tests create_extension for
> NULL-ness before calling recordDependencyOn a bit awkward; maybe hide it
> in a function or macro? But then maybe that's just me. Also, why
> palloc it? Seems better to have it static. Notice your new calls to
> recordDependencyOn are the only ones with operands not using the &
> operator.)

In fact the goal of the test is to check if we're in the code path for CREATE EXTENSION, rather than pointer validity per-say. I'll go have it static, too, with a bool to determine the code path.

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

PS: I hope to be able to send this email, but uploading the git repo will be uneasy from the wifi here at best. Will send patches if email is ok.

Le 25 oct. 2010 à 17:26, Alvaro Herrera a écrit :
> Ah, some reading of the patch reveals that the "script" defaults to the
> control file name, but it can be overridden.

Yes, that's new in v10. In v11 I've kept that and removed the name property in the control file, so that we have:

cat contrib/intarray/intarray.control.in
# intarray
comment = 'one-dimensional arrays of integers: functions, operators, index support'
version = 'EXTVERSION'
script = '_int.sql'

> I noticed that you're using ExclusiveLock when creating an extension,
> citing the handling of the global variable create_extension for this.
> There are two problems here: one is that you're releasing the lock way
> too early: if you wanted this to be effective, you'd need to hold on to
> the lock until after you've registered the extension.
>
> The other is that there is no need for this at all, because this backend
> cannot be concurrently running another CREATE EXTENSION comand, and
> this is a backend-local variable. So there's no point.

I wanted to protect from another backend trying to create the same extension at the same time. So the critical path is the inserting into the catalog. I now see I failed to include the duplicate object check into the critical path, when I added that later.

Do you confirm protecting the insertion in the catalog is not worthy of special locking? To get proper locking requires some more thinking than I did put in, but if you say I'd better remove it...

> Why palloc create_extension every time? Isn't it better to initialize
> it properly and have a boolean value telling whether it's to be used?
> Also, if an extension fails partway through creation, the var will be
> left set. I think you need a PG_TRY block to reset it.

Good catches. I'm still uneasy with which memory context what allocation belongs too, hence the palloc()ing here.

> (I find the repeated coding pattern that tests create_extension for
> NULL-ness before calling recordDependencyOn a bit awkward; maybe hide it
> in a function or macro? But then maybe that's just me. Also, why
> palloc it? Seems better to have it static. Notice your new calls to
> recordDependencyOn are the only ones with operands not using the &
> operator.)

In fact the goal of the test is to check if we're in the code path for CREATE EXTENSION, rather than pointer validity per-say. I'll go have it static, too, with a bool to determine the code path.

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

PS: I hope to be able to send this email, but uploading the git repo will be uneasy from the wifi here at best. Will send patches if email is ok.

Hi,

Thank you for your detailed reviewing, including a patch! Attached is current version of the patch, v11, with your work and comments included. Unfortunately it looks like the git repository won't receive any update until I'm back home, next tuesday.

Le 25 oct. 2010 à 17:26, Alvaro Herrera a écrit :
> Ah, some reading of the patch reveals that the "script" defaults to the
> control file name, but it can be overridden.

Yes. The other mail seems to have made it finally :) twice :(

> I noticed that you're using ExclusiveLock when creating an extension,
> citing the handling of the global variable create_extension for this.
> There are two problems here: one is that you're releasing the lock way
> too early: if you wanted this to be effective, you'd need to hold on to
> the lock until after you've registered the extension.
>
> The other is that there is no need for this at all, because this backend
> cannot be concurrently running another CREATE EXTENSION comand, and
> this is a backend-local variable. So there's no point.

I'm now holding the lock just while inserting in the catalogs, and rechecking for duplicates while I have the lock.

> Why palloc create_extension every time? Isn't it better to initialize
> it properly and have a boolean value telling whether it's to be used?
> Also, if an extension fails partway through creation, the var will be
> left set. I think you need a PG_TRY block to reset it.

Done in v11. I wasn't exactly sure about what to have in the CATCH block, so I've put the minimum, reseting the bool.

> (I find the repeated coding pattern that tests create_extension for
> NULL-ness before calling recordDependencyOn a bit awkward; maybe hide it
> in a function or macro? But then maybe that's just me. Also, why
> palloc it? Seems better to have it static. Notice your new calls to
> recordDependencyOn are the only ones with operands not using the &
> operator.)

Updated to fit a better style, with bool create_extension set to true in the command only, and AddressObject extension that's used everywhere (&extension). No more palloc()ing here.

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

On Wed, Oct 20, 2010 at 01:36, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> Ah yes, thinking it's an easy patch is not helping. Please find attached
> a revised version of it.

I checked cfparser.v2.patch.

It exports the static parseRecoveryCommandFileLine() in xlog.c
as the global cfParseOneLine() in cfparser.c without modification.

It generates one warning, but it can be easily fixed.
cfparser.c:34: warning: no previous prototype for 'cfParseOneLine'

Some discussions about the patch:

* Is "cf" the best name for the prefix? Less abbreviated forms might
be less confusable. Personally, I prefer "conf".

* Can we export ParseConfigFile() in guc-file.l rather than
parseRecoveryCommandFileLine()? It can solve the issue that unquoted
parameter values in recovery.conf are not recognized. Even if we
won't merge them, just allowing unquoted values would be useful.

--
Itagaki Takahiro

On Sun, Nov 21, 2010 at 8:10 PM, Itagaki Takahiro
<itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> On Wed, Oct 20, 2010 at 01:36, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
>> Ah yes, thinking it's an easy patch is not helping. Please find attached
>> a revised version of it.
>
> I checked cfparser.v2.patch.
>
> It exports the static parseRecoveryCommandFileLine() in xlog.c
> as the global cfParseOneLine() in cfparser.c without modification.
>
> It generates one warning, but it can be easily fixed.
>  cfparser.c:34: warning: no previous prototype for 'cfParseOneLine'
>
> Some discussions about the patch:
>
> * Is "cf" the best name for the prefix? Less abbreviated forms might
>  be less confusable. Personally, I prefer "conf".
>
> * Can we export ParseConfigFile() in guc-file.l rather than
>  parseRecoveryCommandFileLine()? It can solve the issue that unquoted
>  parameter values in recovery.conf are not recognized. Even if we
>  won't merge them, just allowing unquoted values would be useful.

I'd really like to see postgresql.conf and recovery.conf parsing
merged, and I suspect, as Itagaki-san says, that postgresql.conf
parsing is the better model for any new code.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> writes:
> I checked cfparser.v2.patch.

Thanks for reviewing it!

> It exports the static parseRecoveryCommandFileLine() in xlog.c
> as the global cfParseOneLine() in cfparser.c without modification.
>
> It generates one warning, but it can be easily fixed.
> cfparser.c:34: warning: no previous prototype for 'cfParseOneLine'

Mmmm, that must have been a cherry-picking error on my side, it seems I
forgot to include this patch in the cfparser branch. Sorry about that.

http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=commitdiff;h=e6b4c670a0189ee8a799521af06c1ee63f9e530e

> Some discussions about the patch:
>
> * Is "cf" the best name for the prefix? Less abbreviated forms might
> be less confusable. Personally, I prefer "conf".

Will change accordingly if that's the choice.

> * Can we export ParseConfigFile() in guc-file.l rather than
> parseRecoveryCommandFileLine()? It can solve the issue that unquoted
> parameter values in recovery.conf are not recognized. Even if we
> won't merge them, just allowing unquoted values would be useful.

Should we then consider recovery.conf entries as ordinary GUCs? That
would allow to connect to a standby and issue 'show primary_conninfo'
there. This has been asked before, already.

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

On Mon, Nov 22, 2010 at 18:36, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:

>> * Can we export ParseConfigFile() in guc-file.l rather than
>>   parseRecoveryCommandFileLine()?
>
> Should we then consider recovery.conf entries as ordinary GUCs? That
> would allow to connect to a standby and issue 'show primary_conninfo'
> there. This has been asked before, already.

No. My suggestion was just to use the internal parser.
ParseConfigFile() returns parsed parameters as head_p and tail_p.
So, we can use it independently from GUC variables.

static bool
ParseConfigFile(const char *config_file, const char *calling_file,
int depth, GucContext context, int elevel,
struct name_value_pair **head_p,
struct name_value_pair **tail_p)

Special codes for "include" and "custom_variable_classes" in it
might not be needed to parse recovery.conf and extensions.
We would disable them when we parse non-guc configuration files.
Or, we could allow "include" in recovery.conf if we think it is
useful in some cases.

--
Itagaki Takahiro

Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> writes:
> No. My suggestion was just to use the internal parser.

What about something like the attached, cfparser.v3.patch?

If that looks ok, do we want to add some documentation about the new
lexer capabilities? Also, for what good reason would we want to prevent
people from using the include facility?

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

Excerpts from Dimitri Fontaine's message of lun nov 22 18:12:39 -0300 2010:
> Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> writes:
> > No. My suggestion was just to use the internal parser.
>
> What about something like the attached, cfparser.v3.patch?
>
> If that looks ok, do we want to add some documentation about the new
> lexer capabilities? Also, for what good reason would we want to prevent
> people from using the include facility?

Hmm, the first thought that comes to mind is that the GucContext param
to ParseConfigFile is unused and can be removed. This is probably an
oversight from when include files were introduced in 2006.

I don't like the fact that this code handles custom_variable_classes
internally. I think this would be exposed to the parsing of extension
control files, which is obviously wrong.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Alvaro Herrera's message of lun nov 22 18:59:52 -0300 2010:

> Hmm, the first thought that comes to mind is that the GucContext param
> to ParseConfigFile is unused and can be removed. This is probably an
> oversight from when include files were introduced in 2006.

Committed and pushed.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Dimitri Fontaine's message of lun nov 22 18:12:39 -0300 2010:
> Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> writes:
> > No. My suggestion was just to use the internal parser.
>
> What about something like the attached, cfparser.v3.patch?

the handling of relative vs absolute paths is bogus here. I think it'd
make more sense to have a bool "are we including"; and if that's false and
the path is not absolute, then the file is relative to CWD; or maybe we
make it absolute by prepending PGDATA; maybe something else? (need to
think of something that makes sense for both recovery.conf and extension
control files)

> If that looks ok, do we want to add some documentation about the new
> lexer capabilities?

beyond extra code comments? probably not.

> Also, for what good reason would we want to prevent
> people from using the include facility?

Not sure about this

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Hmm, the first thought that comes to mind is that the GucContext param
> to ParseConfigFile is unused and can be removed. This is probably an
> oversight from when include files were introduced in 2006.

Thanks for caring about that part.

> I don't like the fact that this code handles custom_variable_classes
> internally. I think this would be exposed to the parsing of extension
> control files, which is obviously wrong.

Well, in fact, not that much. The extension code has a special error
case when dealing with custom variables if the class hasn't been already
parsed, and what ParseConfigFile() is doing is pushing the
custom_variable_classes setting in front of the list.

guc-file.l says:
/*
* This variable must be processed first as it controls
* the validity of other variables; so it goes at the head
* of the result list. If we already found a value for it,
* replace with this one.
*/

extension.c says:
ereport(ERROR,
(errmsg("Unsupported parameter '%s' in file: %s",
tok1, filename),
errhint("Be sure to have 'custom_variable_classes' set "
"in a line before any custom variable.")));

So if we don't change the code in ParseConfigFile() that will push
custom_variable_classes in front of the list, all I have to change in
the extension.c file is the error message.

I fail to see a future usage of custom_variable_classes where it
wouldn't help to have that in the list before any user setting that
depends on it.

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