Extensions, this time with a patch

Started by Dimitri Fontaineover 15 years ago128 messageshackers
Jump to latest
#1Dimitri Fontaine
dimitri@2ndQuadrant.fr

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

Attachments:

extension.v0.patchtext/x-patchDownload+1842-525
cfparser.v0.patchtext/x-patchDownload+133-96
#2David Fetter
david@fetter.org
In reply to: Dimitri Fontaine (#1)
Re: Extensions, this time with a patch

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

#3Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: David Fetter (#2)
Re: Extensions, this time with a patch

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

#4Tom Lane
tgl@sss.pgh.pa.us
In reply to: Alvaro Herrera (#3)
Re: Extensions, this time with a patch

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

#5David Fetter
david@fetter.org
In reply to: Alvaro Herrera (#3)
Re: Extensions, this time with a patch

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

#6Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Alvaro Herrera (#3)
Re: Extensions, this time with a patch

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

#7Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Dimitri Fontaine (#6)
Re: Extensions, this time with a patch

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

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

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

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

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

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

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

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

#8Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Alvaro Herrera (#7)
Re: Extensions, this time with a patch

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

#9Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Dimitri Fontaine (#1)
Re: Extensions, this time with a patch

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

Attachments:

extension.v1.patch.gzapplication/octet-streamDownload
#10Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Alvaro Herrera (#7)
Re: Extensions, this time with a patch

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

Attachments:

cfparser.v1.patchtext/x-patchDownload+135-98
pg_execute_from_file.v0.patchtext/x-patchDownload+204-8
#11Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Dimitri Fontaine (#10)
Re: Extensions, this time with a patch

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

Attachments:

extension.v2.patch.gzapplication/octet-streamDownload
pg_execute_from_file.v1.patchtext/x-patchDownload+221-8
#12Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Dimitri Fontaine (#11)
Re: Extensions, this time with a patch

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

#13Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Alvaro Herrera (#12)
Re: Extensions, this time with a patch

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

#14Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Dimitri Fontaine (#11)
Re: Extensions, this time with a patch

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

#15Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Alvaro Herrera (#13)
Re: Extensions, this time with a patch

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

#16Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Alvaro Herrera (#14)
Re: Extensions, this time with a patch

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

#17Tom Lane
tgl@sss.pgh.pa.us
In reply to: Dimitri Fontaine (#16)
Re: Extensions, this time with a patch

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

#18Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Tom Lane (#17)
Re: Extensions, this time with a patch

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

#19Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Dimitri Fontaine (#18)
Re: Extensions, this time with a patch

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

Attachments:

pg_execute_from_file.v2.patchtext/x-patchDownload+220-9
#20Tom Lane
tgl@sss.pgh.pa.us
In reply to: Dimitri Fontaine (#18)
Re: Extensions, this time with a patch

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

#21Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Tom Lane (#20)
#22Tom Lane
tgl@sss.pgh.pa.us
In reply to: Alvaro Herrera (#21)
#23Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Tom Lane (#22)
#24David Fetter
david@fetter.org
In reply to: Alvaro Herrera (#23)
#25Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Tom Lane (#22)
#26Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Dimitri Fontaine (#9)
#27Itagaki Takahiro
itagaki.takahiro@gmail.com
In reply to: Dimitri Fontaine (#26)
#28Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Itagaki Takahiro (#27)
#29Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Itagaki Takahiro (#27)
#30Robert Haas
robertmhaas@gmail.com
In reply to: Dimitri Fontaine (#29)
#31Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Robert Haas (#30)
#32Robert Haas
robertmhaas@gmail.com
In reply to: Dimitri Fontaine (#31)
#33Tom Lane
tgl@sss.pgh.pa.us
In reply to: Dimitri Fontaine (#31)
#34Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Tom Lane (#33)
#35Tom Lane
tgl@sss.pgh.pa.us
In reply to: Dimitri Fontaine (#34)
#36Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Tom Lane (#35)
#37Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Dimitri Fontaine (#25)
#38Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Dimitri Fontaine (#11)
#39Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Alvaro Herrera (#37)
#40Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Alvaro Herrera (#38)
#41Robert Haas
robertmhaas@gmail.com
In reply to: Dimitri Fontaine (#36)
#42Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#41)
#43Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Dimitri Fontaine (#36)
#44Itagaki Takahiro
itagaki.takahiro@gmail.com
In reply to: Dimitri Fontaine (#29)
#45Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Itagaki Takahiro (#44)
#46Itagaki Takahiro
itagaki.takahiro@gmail.com
In reply to: Alvaro Herrera (#45)
#47Itagaki Takahiro
itagaki.takahiro@gmail.com
In reply to: Itagaki Takahiro (#44)
#48Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Tom Lane (#42)
#49Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Itagaki Takahiro (#44)
#50Robert Haas
robertmhaas@gmail.com
In reply to: Dimitri Fontaine (#49)
#51Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Robert Haas (#50)
#52Robert Haas
robertmhaas@gmail.com
In reply to: Dimitri Fontaine (#51)
#53Tom Lane
tgl@sss.pgh.pa.us
In reply to: Itagaki Takahiro (#46)
#54Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Tom Lane (#53)
#55Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#50)
#56Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Dimitri Fontaine (#49)
#57Tom Lane
tgl@sss.pgh.pa.us
In reply to: Dimitri Fontaine (#54)
#58Tom Lane
tgl@sss.pgh.pa.us
In reply to: Alvaro Herrera (#56)
#59Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Tom Lane (#57)
#60Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Tom Lane (#58)
#61Tom Lane
tgl@sss.pgh.pa.us
In reply to: Dimitri Fontaine (#59)
#62Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Tom Lane (#61)
#63Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Tom Lane (#61)
#64Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Tom Lane (#61)
#65David E. Wheeler
david@kineticode.com
In reply to: Dimitri Fontaine (#64)
#66Itagaki Takahiro
itagaki.takahiro@gmail.com
In reply to: Dimitri Fontaine (#64)
#67Itagaki Takahiro
itagaki.takahiro@gmail.com
In reply to: David E. Wheeler (#65)
#68Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Itagaki Takahiro (#67)
#69David E. Wheeler
david@kineticode.com
In reply to: Alvaro Herrera (#68)
#70Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: David E. Wheeler (#65)
#71Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Itagaki Takahiro (#66)
#72Tom Lane
tgl@sss.pgh.pa.us
In reply to: Dimitri Fontaine (#71)
#73Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Tom Lane (#72)
#74David E. Wheeler
david@kineticode.com
In reply to: Dimitri Fontaine (#70)
#75Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: David E. Wheeler (#74)
#76David E. Wheeler
david@kineticode.com
In reply to: Dimitri Fontaine (#75)
#77Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: David E. Wheeler (#76)
#78Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Dimitri Fontaine (#77)
#79Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Alvaro Herrera (#78)
#80Robert Haas
robertmhaas@gmail.com
In reply to: Dimitri Fontaine (#75)
#81Itagaki Takahiro
itagaki.takahiro@gmail.com
In reply to: Dimitri Fontaine (#79)
#82Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Itagaki Takahiro (#81)
#83Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Dimitri Fontaine (#82)
#84Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Dimitri Fontaine (#83)
#85Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Itagaki Takahiro (#81)
#86Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Itagaki Takahiro (#81)
#87Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Dimitri Fontaine (#85)
#88Tom Lane
tgl@sss.pgh.pa.us
In reply to: Alvaro Herrera (#87)
#89Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Alvaro Herrera (#87)
#90Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Tom Lane (#88)
#91Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Dimitri Fontaine (#89)
#92Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Alvaro Herrera (#91)
#93Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Alvaro Herrera (#91)
#94Andrew Dunstan
andrew@dunslane.net
In reply to: Dimitri Fontaine (#93)
#95Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Andrew Dunstan (#94)
#96Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Dimitri Fontaine (#93)
#97Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Alvaro Herrera (#96)
#98Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Dimitri Fontaine (#97)
#99Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Dimitri Fontaine (#97)
#100Robert Haas
robertmhaas@gmail.com
In reply to: Alvaro Herrera (#99)
#101Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Robert Haas (#100)
#102Josh Berkus
josh@agliodbs.com
In reply to: Robert Haas (#100)
#103Tom Lane
tgl@sss.pgh.pa.us
In reply to: Dimitri Fontaine (#97)
#104Itagaki Takahiro
itagaki.takahiro@gmail.com
In reply to: Josh Berkus (#102)
#105Tom Lane
tgl@sss.pgh.pa.us
In reply to: Itagaki Takahiro (#104)
#106Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Alvaro Herrera (#99)
#107Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Alvaro Herrera (#106)
#108Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Dimitri Fontaine (#98)
#109Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Alvaro Herrera (#107)
#110Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Alvaro Herrera (#107)
#111Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Alvaro Herrera (#107)
#112Itagaki Takahiro
itagaki.takahiro@gmail.com
In reply to: Dimitri Fontaine (#111)
#113Robert Haas
robertmhaas@gmail.com
In reply to: Itagaki Takahiro (#112)
#114Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Itagaki Takahiro (#112)
#115Itagaki Takahiro
itagaki.takahiro@gmail.com
In reply to: Dimitri Fontaine (#114)
#116Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Itagaki Takahiro (#115)
#117Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Dimitri Fontaine (#116)
#118Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Alvaro Herrera (#117)
#119Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Dimitri Fontaine (#116)
#120Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Alvaro Herrera (#117)
#121Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Alvaro Herrera (#119)
#122Heikki Linnakangas
heikki.linnakangas@enterprisedb.com
In reply to: Robert Haas (#113)
#123Itagaki Takahiro
itagaki.takahiro@gmail.com
In reply to: Dimitri Fontaine (#121)
#124Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Itagaki Takahiro (#123)
#125Itagaki Takahiro
itagaki.takahiro@gmail.com
In reply to: Dimitri Fontaine (#124)
#126Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Itagaki Takahiro (#125)
#127Robert Haas
robertmhaas@gmail.com
In reply to: Dimitri Fontaine (#126)
#128Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Robert Haas (#127)