Thread: 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
Attachment
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@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.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@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.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@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.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@commandprompt.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.gitgit fetch extensionsextension: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@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.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@2ndQuadrant.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
Attachment
Alvaro Herrera <alvherre@commandprompt.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
Attachment
Dimitri Fontaine <dimitri@2ndQuadrant.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
Attachment
Excerpts from Dimitri Fontaine's message of vie oct 15 16:15:23 -0300 2010: > Dimitri Fontaine <dimitri@2ndQuadrant.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@commandprompt.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@commandprompt.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@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.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@commandprompt.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@2ndQuadrant.fr> writes:
> Alvaro Herrera <alvherre@commandprompt.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@sss.pgh.pa.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@2ndQuadrant.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
Attachment
Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> Tom Lane <tgl@sss.pgh.pa.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@2ndQuadrant.fr> writes: > > Tom Lane <tgl@sss.pgh.pa.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@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.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@commandprompt.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@commandprompt.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@commandprompt.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@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.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@sss.pgh.pa.us> writes:
> Alvaro Herrera <alvherre@commandprompt.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
Attachment
Dimitri Fontaine <dimitri@2ndQuadrant.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
Attachment
On Mon, Oct 18, 2010 at 8:37 PM, Dimitri Fontaine <dimitri@2ndquadrant.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.takahiro@gmail.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.takahiro@gmail.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
Attachment
On Oct 19, 2010, at 8:33 AM, Dimitri Fontaine <dimitri@2ndQuadrant.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@gmail.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@2ndquadrant.fr> wrote: > Robert Haas <robertmhaas@gmail.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@2ndQuadrant.fr> writes:
> Robert Haas <robertmhaas@gmail.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@sss.pgh.pa.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@2ndQuadrant.fr> writes:
> Tom Lane <tgl@sss.pgh.pa.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@sss.pgh.pa.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
Attachment
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; maybeerrmsg("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@commandprompt.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@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.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
Attachment
Alvaro Herrera <alvherre@commandprompt.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
Attachment
On Tue, Oct 19, 2010 at 12:09 PM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote: > Tom Lane <tgl@sss.pgh.pa.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@gmail.com> writes:
> On Tue, Oct 19, 2010 at 12:09 PM, Dimitri Fontaine
> <dimitri@2ndquadrant.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@sss.pgh.pa.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@commandprompt.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@2ndquadrant.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 objectsget 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@commandprompt.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@commandprompt.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.takahiro@gmail.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@sss.pgh.pa.us> writes: > Robert Haas <robertmhaas@gmail.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.takahiro@gmail.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
Attachment
On Wed, Oct 20, 2010 at 6:22 AM, Dimitri Fontaine <dimitri@2ndquadrant.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@gmail.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@2ndquadrant.fr> wrote: > Robert Haas <robertmhaas@gmail.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.takahiro@gmail.com> writes: > On Wed, Oct 20, 2010 at 12:58 PM, Alvaro Herrera > <alvherre@commandprompt.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@sss.pgh.pa.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@gmail.com> writes:
> On Wed, Oct 20, 2010 at 6:22 AM, Dimitri Fontaine
> <dimitri@2ndquadrant.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.takahiro@gmail.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@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> Tom Lane <tgl@sss.pgh.pa.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@commandprompt.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@sss.pgh.pa.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@sss.pgh.pa.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@2ndQuadrant.fr> writes:
> Tom Lane <tgl@sss.pgh.pa.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@sss.pgh.pa.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@sss.pgh.pa.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@sss.pgh.pa.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.
Attachment
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@justatheory.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@2ndquadrant.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@kineticode.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@kineticode.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@commandprompt.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@kineticode.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.takahiro@gmail.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
Attachment
Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> Itagaki Takahiro <itagaki.takahiro@gmail.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@sss.pgh.pa.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
Attachment
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@kineticode.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 experienceis 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@kineticode.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@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.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
Attachment
On Thu, Oct 21, 2010 at 11:12 AM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote: > "David E. Wheeler" <david@kineticode.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@2ndquadrant.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.takahiro@gmail.com> writes: > * extension.v9.patch.gz seems to contain other changes in the repo. > Did you use old master to get the diff? Sorry about that, I've been using git diff master.. but this time, I did merge pgmaster (upstream) into extension directly rather than into my local master then merge into extension. So that the patch contained all the accumulated diffs between previous git pull and this one. Please find attached a clean v9 patch, that is easy to get from the git repository too. http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=shortlog;h=refs/heads/extension > * Typo in doc/xfunc.sgml. They are to be "replaceable" probably. > openjade:xfunc.sgml:2510:32:E: element "REPLACABLE" undefined > openjade:xfunc.sgml:2523:46:E: element "REPLACABLE" undefined Fixing now, will be ok in v10 later today. > * There are some inconsistency between extension names in \dx+ view > and actual name used by CREATE EXTENSION. > - auto_username => insert_username > - intarray => _int > - xml2 => pgxml > We might need to rename them, or add 'installer'/'uninstaller' entries > into control files to support different extension names from .so name. The problem here is that the design and the implementation differ. The design is to have no flexibility on the SQL script name, it has to share the .control base name. Now, the implementation of \dx+ will report the name field read into the control file, which can be different from the file name. Please note that given DROP EXTENSION name [ RESTRICT | CASCADE ]; I think that uninstall script are deprecated. So the fix is to either change the design so that the script file to use is to be found in the control file, or change the implementation to remove the name field into the control file: the name of the extension would then be the control file name sans extension. I lean toward adding support for a script variable into the control file which defaults to script = '${name}.sql' and will have to be edited only in those 3 cases you're reporting about. I'm going to work on that this morning, it looks simple enough to get reworked if necessary. (yes it means we have to scan all control files to find the one where the name property is the one given in the CREATE EXTENSION command, but the code already exists --- it still has to be refactored) > * pg_execute_from_file() and encoding > It expects the file is in server encoding, but it is not always true > because we support multiple encodings in the same installation. > How about adding encoding parameter to the function? > => pg_execute_from_file( file, encoding ) > CREATE EXTENSION could have optional ENCODING option. > => CREATE EXTENSION name [ ENCODING 'which' ] > > I strongly hope the multi-encoding support for my Japanese textsearch > extension. Comments in the extension is written in UTF-8, but both > UTF-8 and EUC_JP are equally used for database encodings. That's just a part of the problem that is yet to be addressed, be assured that I also want to fix it. Will go discover some more code and find the APIs to make your proposal happen, should get implemented in v10. You're not saying it, but I think the encoding argument of the function should be optional and default to database encoding, as the check is already implemented: pg_verifymbstr(query_string, nbytes, false); > * Error messages in pg_execute_from_file() > - "must be superuser to get file information" would be > "must be superuser to execute file" . > - "File '%s' could not be executed" would be > "could not execute file: '%s'". Our message style guide is here: > http://www.postgresql.org/docs/9.0/static/error-style-guide.html > Many messages in extension.c are also to be adjusted. Will do. > commands/extension.c needs to be cleaned up a bit more: > * fsize in read_extension_control_file() is not used. > * ferror() test just after AllocateFile() is not needed; > NULL checking is enough. > * malloc() in add_extension_custom_variable_classes(). > I think the README says nothing about malloc() except assign_hook > cases; palloc would be better here. I didn't read it as assign_hook only, because at least in firsts incarnations of the patch it looked like I was creating guc entries by myself. So I compared to guc_malloc, guc_realloc, guc_strdup. Will happily switch to memory context dependent allocations if that's what to do here. > BTW, did you register your patch to the next commitfest? > It would be better to do so for tracking the activities. > https://commitfest.postgresql.org/action/commitfest_view?id=8 Will do starting with v10, but until about v8 the patch was missing features: I wanted to make sure the direction taken was ok, but it didn't feel ready to submit. It's taking shape now ;) Regards, and thanks again for your reviewing time! -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Attachment
Dimitri Fontaine <dimitri@2ndQuadrant.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@2ndQuadrant.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.takahiro@gmail.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
Attachment
Itagaki Takahiro <itagaki.takahiro@gmail.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.takahiro@gmail.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@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.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@commandprompt.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@sss.pgh.pa.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@commandprompt.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@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.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@commandprompt.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@dunslane.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@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.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@2ndQuadrant.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@commandprompt.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@commandprompt.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@commandprompt.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@commandprompt.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@2ndQuadrant.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@agliodbs.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.takahiro@gmail.com> writes:
> On Sat, Oct 23, 2010 at 5:26 AM, Josh Berkus <josh@agliodbs.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@commandprompt.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@commandprompt.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@2ndQuadrant.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@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Attachment
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 theinserting into the catalog. I now see I failed to include the duplicate object check into the critical path, when I addedthat later. Do you confirm protecting the insertion in the catalog is not worthy of special locking? To get proper locking requires somemore 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 sendpatches 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 theinserting into the catalog. I now see I failed to include the duplicate object check into the critical path, when I addedthat later. Do you confirm protecting the insertion in the catalog is not worthy of special locking? To get proper locking requires somemore 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 sendpatches 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 andcomments 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'sused everywhere (&extension). No more palloc()ing here. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Attachment
On Wed, Oct 20, 2010 at 01:36, Dimitri Fontaine <dimitri@2ndquadrant.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 unquotedparameter values in recovery.conf are not recognized. Even if we won't merge them, just allowing unquoted valueswould be useful. -- Itagaki Takahiro
On Sun, Nov 21, 2010 at 8:10 PM, Itagaki Takahiro <itagaki.takahiro@gmail.com> wrote: > On Wed, Oct 20, 2010 at 01:36, Dimitri Fontaine <dimitri@2ndquadrant.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.takahiro@gmail.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@2ndquadrant.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.takahiro@gmail.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
Attachment
Excerpts from Dimitri Fontaine's message of lun nov 22 18:12:39 -0300 2010: > Itagaki Takahiro <itagaki.takahiro@gmail.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@commandprompt.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@commandprompt.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.takahiro@gmail.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@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.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
othervariables; so it goes at the head * of the result list. If we already found a value for it, *
replacewith 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
Alvaro Herrera <alvherre@commandprompt.com> writes: > 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) Current coding in extensions prepend any control or script file with sharepath, so that we're only dealing with absolute filename here. The idea is that it's no business for any other part of the code to have to know where we decide to install control and script files. My feeling is that when !is_absolute_path(config_file) and calling_file is NULL we should make the config_file absolute by prepending PGDATA. Please find that done in attached v4 of the cfparser patch. >> If that looks ok, do we want to add some documentation about the new >> lexer capabilities? > > beyond extra code comments? probably not. Great. >> Also, for what good reason would we want to prevent >> people from using the include facility? > > Not sure about this Ok, nothing special here. -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Attachment
On 22.11.2010 03:35, Robert Haas wrote: > On Sun, Nov 21, 2010 at 8:10 PM, Itagaki Takahiro > <itagaki.takahiro@gmail.com> wrote: >> On Wed, Oct 20, 2010 at 01:36, Dimitri Fontaine<dimitri@2ndquadrant.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. +1. There was unanimous agreement in the synchronous replication threads that recovery.conf should be parsed with the GUC parser. The current recovery.conf parser doesn't support escaping, for example. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On Tue, Nov 23, 2010 at 18:19, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote: > Please find that done in attached v4 of the cfparser patch. RECOVERY_COMMAND_FILE is opened twice in the patch. The first time is for checking the existence, and the second time is for parsing. Instead of the repeat, how about adding FILE* version of parser? It will be also called from ParseConfigFile() as a sub routine. bool ParseConfigFd(FILE *fd, const char *config_file, int depth, ...) BTW, the parser supports "include" and "custom_variable_classes" not only for postgresql.conf but also for all files. Is it an intended behavior? I think they are harmless, so we don't have to change the codes; "include" might be useful even in recovery.conf, and "custom_variable_classes" will be "unrecognized recovery parameter" error after all. -- Itagaki Takahiro
Itagaki Takahiro <itagaki.takahiro@gmail.com> writes: > RECOVERY_COMMAND_FILE is opened twice in the patch. The first time > is for checking the existence, and the second time is for parsing. > Instead of the repeat, how about adding FILE* version of parser? > It will be also called from ParseConfigFile() as a sub routine. > > bool ParseConfigFd(FILE *fd, const char *config_file, int depth, ...) Something like the attached, version 5 of the patch? I've been using the function name ParseConfigFp because the internal parameter was called fp in the previous function body. I suppose that could easily be changed at commit time if necessary. > BTW, the parser supports "include" and "custom_variable_classes" > not only for postgresql.conf but also for all files. Is it an > intended behavior? I think they are harmless, so we don't have > to change the codes; "include" might be useful even in recovery.conf, > and "custom_variable_classes" will be "unrecognized recovery > parameter" error after all. Extensions will need the support for custom_variable_classes as it is done now, and as you say, the recovery will just error out. You have to clean your recovery.conf file then try again (I just tried and confirm). I personally don't see any harm to have the features in all currently known uses-cases, and I don't see any point in walking an extra mile here to remove a feature in some cases. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Attachment
On Thu, Nov 25, 2010 at 05:02, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote: > Something like the attached, version 5 of the patch? I've been using the > function name ParseConfigFp because the internal parameter was called fp > in the previous function body. I suppose that could easily be changed at > commit time if necessary. Thanks. I'll move the patch to Ready for Committer. Here is a list of items for committers, including only cosmetic changes. - Comments in recovery.conf.sample needs to be adjusted. # (The quotes around the value are NOT optional, but the "=" is.)It seems to be the only description about quotes are not omissible before. - We might not need to check the result of ParseConfigFp() because it always raises FATAL on errors. - We could remove the unused argument "calling_file" in ParseConfigFp(). - I feel "struct name_value_pair" and "ConfigNameValuePair" a bit redundant names. I'd prefer something like ConfigVariable. - "fp" might be a better name than FILE *fd. There are 4 usages in xlog.c. > Extensions will need the support for custom_variable_classes as it is > done now, and as you say, the recovery will just error out. You have to > clean your recovery.conf file then try again (I just tried and confirm). > > I personally don't see any harm to have the features in all currently > known uses-cases, and I don't see any point in walking an extra mile > here to remove a feature in some cases. Sure. We will leave them. -- Itagaki Takahiro
Itagaki Takahiro <itagaki.takahiro@gmail.com> writes: > Thanks. I'll move the patch to Ready for Committer. Thanks! -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On Thu, Nov 25, 2010 at 4:06 PM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote: > Itagaki Takahiro <itagaki.takahiro@gmail.com> writes: >> Thanks. I'll move the patch to Ready for Committer. > > Thanks! I have committed the cfparser patch to which the above comments refer.One patch per thread makes things easier! I adopted most of Itagaki Takahiro's suggestions, which were very helpful. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > I have committed the cfparser patch to which the above comments refer. Excellent, thank you! On to merging that into the extension's main branch, will still wait until after pg_execute_sql_file() is in to produce extension.v15.patch, unless advised otherwise. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support