[PATCH] Magic block for modules

Martijn van Oosterhout <kleptog@svana.org> writes:

This implements a proposal made last november:
http://archives.postgresql.org/pgsql-hackers/2005-11/msg00578.php

Ah, good, I'd been meaning to do this.

changes in any of the following:

PG_VERSION_NUM
CATALOG_VERSION_NO
the size of 8 basic C types
BLCKSZ=20
NAMEDATALEN=20
HAVE_INT64_TIMESTAMP
INDEX_MAX_KEYS
FUNC_MAX_ARGS
VARHDRSZ
MAXDIM
The compiler used (only brand, not version)

That seems way overkill to me. FUNC_MAX_ARGS is good to check, but
most of those other things are noncritical for typical add-on modules.
In particular I strongly object to the check on compiler. Some of us do
use systems where gcc and vendor compilers are supposed to interoperate
... and aren't all those Windows compilers supposed to, too? AFAIK
it's considered the linker's job to prevent loading 32-bit code into
a 64-bit executable or vice versa, so I don't think we need to be
checking for common assumptions about sizeof(long).

Currently, modules without a magic block are merely logged at LOG
level. This needs some discussion though.

I'm pretty sure we had agreed that magic blocks should be required;
otherwise this check will accomplish little.

regards, tom lane

On Sun, May 07, 2006 at 08:21:43PM -0400, Tom Lane wrote:

changes in any of the following:

PG_VERSION_NUM
CATALOG_VERSION_NO
the size of 8 basic C types
BLCKSZ=20
NAMEDATALEN=20
HAVE_INT64_TIMESTAMP
INDEX_MAX_KEYS
FUNC_MAX_ARGS
VARHDRSZ
MAXDIM
The compiler used (only brand, not version)

That seems way overkill to me. FUNC_MAX_ARGS is good to check, but
most of those other things are noncritical for typical add-on modules.

I was trying to find variables that when changed would make some things
corrupt. For example, a changed NAMEDATALEN will make any use of the
syscache a source of errors. A change in INDEX_MAX_KEYS will break the
GiST interface, etc. I wondered about letting module writers to select
which parts are relevent to them but that just seems like handing
people a footgun.

In particular I strongly object to the check on compiler. Some of us do
use systems where gcc and vendor compilers are supposed to interoperate
... and aren't all those Windows compilers supposed to, too? AFAIK

Maybe that's the case now, it didn't used to be. I seem to remember
people having difficulties because they compiled the server with MinGW
and the modules with VC++. I'll take it out though, it's not like it
costs anything.

it's considered the linker's job to prevent loading 32-bit code into
a 64-bit executable or vice versa, so I don't think we need to be
checking for common assumptions about sizeof(long).

I know ELF headers contain some of this info, and unix in general
doesn't try to allow different bit sizes in one binary. Windows used to
(maybe still has) a mechanism to allow 32-bit code to call 16-bit
libraries. Do they allow the same for 64-bit libs?

I'm pretty sure we had agreed that magic blocks should be required;
otherwise this check will accomplish little.

Sure, I just didn't want to break every module in one weekend. I was
thinking of adding it with LOG level now, send a message on -announce
saying that at the beginning of the 8.2 freeze it will be an ERROR.
Give people time to react.

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

Martijn van Oosterhout <kleptog@svana.org> writes:

On Sun, May 07, 2006 at 08:21:43PM -0400, Tom Lane wrote:

That seems way overkill to me. FUNC_MAX_ARGS is good to check, but
most of those other things are noncritical for typical add-on modules.

I was trying to find variables that when changed would make some things
corrupt. For example, a changed NAMEDATALEN will make any use of the
syscache a source of errors. A change in INDEX_MAX_KEYS will break the
GiST interface, etc.

By that rationale you'd have to record just about every #define in the
system headers. And it still wouldn't be bulletproof --- what of
custom-modified code with, say, extra fields inserted into some widely
used struct?

But you're missing the larger point, which is that in many cases this
would be breaking stuff without any need at all. The majority of
catversion bumps, for instance, are for things that don't affect the
typical add-on module. So checking for identical catversion won't
accomplish much except to force additional recompile churn on people
doing development against CVS HEAD. The original proposal was just
to check for major PG version match. I can see checking FUNC_MAX_ARGS
too, because that has a very direct impact on the ABI that every
external function sees, but I think the cost/benefit ratio rises pretty
darn steeply after that.

Another problem with an expansive list of stuff-to-check is where does
the add-on module find it out from? AFAICS your proposal would make for
a large laundry list of random headers that every add-on would now have
to #include. If it's not defined by postgres.h or fmgr.h (which are two
things that every backend addon is surely including already) then I'm
dubious about using it in the magic block.

Sure, I just didn't want to break every module in one weekend. I was
thinking of adding it with LOG level now, send a message on -announce
saying that at the beginning of the 8.2 freeze it will be an ERROR.
Give people time to react.

I think that will just mean that we'll break every module at the start
of 8.2 freeze ;-). Unless we forget to change it to error, which IMHO
is way too likely.

regards, tom lane

On Mon, May 08, 2006 at 10:32:47AM -0400, Tom Lane wrote:

Martijn van Oosterhout <kleptog@svana.org> writes:

I was trying to find variables that when changed would make some things
corrupt. For example, a changed NAMEDATALEN will make any use of the
syscache a source of errors. A change in INDEX_MAX_KEYS will break the
GiST interface, etc.

By that rationale you'd have to record just about every #define in the
system headers. And it still wouldn't be bulletproof --- what of
custom-modified code with, say, extra fields inserted into some widely
used struct?

I can see that. That's why I specifically aimed at the ones defined in
pg_config_manual.h, ie, the ones marked "twiddle me".

... So checking for identical catversion won't
accomplish much except to force additional recompile churn on people
doing development against CVS HEAD. The original proposal was just
to check for major PG version match.

Ok, I've taken out CATVERSION and cut PG version to just the major
version. I've also dropped the compiler and several others.

Another problem with an expansive list of stuff-to-check is where does
the add-on module find it out from?

All these symbols are defined by including c.h only, which is included
by postgres.h, so this is not an issue. I obviously didn't include any
symbols that a module would need to add special includes for. The only
outlier was CATVERSION but we're dropping that test.

I think that will just mean that we'll break every module at the start
of 8.2 freeze ;-). Unless we forget to change it to error, which IMHO
is way too likely.

Ok, one week then. Not everyone follows -patches and will be mighty
confused when a CVS update suddenly breaks everything.

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

Martijn van Oosterhout <kleptog@svana.org> writes:

On Sun, May 07, 2006 at 08:21:43PM -0400, Tom Lane wrote:

I'm pretty sure we had agreed that magic blocks should be required;
otherwise this check will accomplish little.

Sure, I just didn't want to break every module in one weekend. I was
thinking of adding it with LOG level now, send a message on -announce
saying that at the beginning of the 8.2 freeze it will be an ERROR.
Give people time to react.

Now that the magic-block patch is in, we need to revisit this bit of the
discussion. I'm for making lack of a magic block an ERROR immediately.
I don't see the point of waiting; in fact, if we wait till freeze we'll
just make the breakage more concentrated. At the very least it ought
to be a WARNING immediately, because a LOG message is just not visible
enough.

Comments?

regards, tom lane

On Tue, May 30, 2006 at 06:20:33PM -0400, Tom Lane wrote:

Now that the magic-block patch is in, we need to revisit this bit of the
discussion. I'm for making lack of a magic block an ERROR immediately.
I don't see the point of waiting; in fact, if we wait till freeze we'll
just make the breakage more concentrated. At the very least it ought
to be a WARNING immediately, because a LOG message is just not visible
enough.

If you like I can send a patch that adds it to all of contrib and some
of the other places required so that make check passes...

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

On Sun, May 07, 2006 at 08:21:43PM -0400, Tom Lane wrote:

I'm pretty sure we had agreed that magic blocks should be

required;

otherwise this check will accomplish little.

Sure, I just didn't want to break every module in one

weekend. I was

thinking of adding it with LOG level now, send a message on

-announce

saying that at the beginning of the 8.2 freeze it will be an ERROR.
Give people time to react.

Now that the magic-block patch is in, we need to revisit this
bit of the discussion. I'm for making lack of a magic block
an ERROR immediately.
I don't see the point of waiting; in fact, if we wait till
freeze we'll just make the breakage more concentrated. At
the very least it ought to be a WARNING immediately, because
a LOG message is just not visible enough.

Comments?

If it's eventually going to be an ERROR, it's better to make it ERROR
from the start.

People working off cvs snapshot will (hopefully) expect temporary
breakage during the development period. In general, you'd expect less
breakage the closer to release you are.

//Magnus

Magnus Hagander wrote:

On Sun, May 07, 2006 at 08:21:43PM -0400, Tom Lane wrote:

I'm pretty sure we had agreed that magic blocks should be

required;

otherwise this check will accomplish little.

Sure, I just didn't want to break every module in one

weekend. I was

thinking of adding it with LOG level now, send a message on

-announce

saying that at the beginning of the 8.2 freeze it will be an ERROR.
Give people time to react.

Now that the magic-block patch is in, we need to revisit this
bit of the discussion. I'm for making lack of a magic block
an ERROR immediately.
I don't see the point of waiting; in fact, if we wait till
freeze we'll just make the breakage more concentrated. At
the very least it ought to be a WARNING immediately, because
a LOG message is just not visible enough.

Comments?

If it's eventually going to be an ERROR, it's better to make it ERROR
from the start.

People working off cvs snapshot will (hopefully) expect temporary
breakage during the development period. In general, you'd expect less
breakage the closer to release you are.

I say make it an ERROR and we can relax it later. If you make it a
warning, we might not hear about it.

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On 5/8/06, Martijn van Oosterhout <kleptog@svana.org> wrote:

This implements a proposal made last november:

http://archives.postgresql.org/pgsql-hackers/2005-11/msg00578.php

All it requires is to include the header "pgmagic.h" and to put
somewhere in their source:

PG_MODULE_MAGIC

Could you serve this as special docstring instead? Eg:

PG_MODULE(foomodule)

is mandatory, there you can to your magic, and optional:

PG_MODULE_DESC("Do foo")
PG_MODULE_AUTHOR("FooMan <baz@foo>")

This provides more motivation for module authors and also creates
(visually) smooth path to provide automatic install, uninstall and registration:

PG_MODULE_INSTALL(inst_sql)
PG_MODULE_UNINSTALL(uninst_sql)

create module foo from '$libdir/foo';
drop module foo;

This seems like worthwhile direction to move, especially
as it requires pretty small amount of changes.

--
marko

On Wed, May 31, 2006 at 01:08:41PM +0300, Marko Kreen wrote:

On 5/8/06, Martijn van Oosterhout <kleptog@svana.org> wrote:

All it requires is to include the header "pgmagic.h" and to put
somewhere in their source:

PG_MODULE_MAGIC

Could you serve this as special docstring instead? Eg:

PG_MODULE(foomodule)

is mandatory, there you can to your magic, and optional:

<snip>

I like it, but I'm not sure there's enough consensus for that. I've
suggested before including install info inside the modules themselves
but there doesn't seem to be much interest in that.

Apart from that there's issues with implementation. The Linux kernel
can do it easily because it knows it will be using ELF, thus can use
sections to store this info. Postgresql has to support many more types,
making things like this tricky (but not impossible).

Personally I'd like postgres to move to a system where external modules
can easily be installed, uninstalled and upgraded. However, I've not
seen the demand yet.

Have a nice day
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

Martijn van Oosterhout <kleptog@svana.org> writes:

If you like I can send a patch that adds it to all of contrib and some
of the other places required so that make check passes...

Think I got them all already:
http://archives.postgresql.org/pgsql-committers/2006-05/msg00384.php
but if you see any I missed...

regards, tom lane

On 5/31/06, Martijn van Oosterhout <kleptog@svana.org> wrote:

On Wed, May 31, 2006 at 01:08:41PM +0300, Marko Kreen wrote:

On 5/8/06, Martijn van Oosterhout <kleptog@svana.org> wrote:

All it requires is to include the header "pgmagic.h" and to put
somewhere in their source:

PG_MODULE_MAGIC

Could you serve this as special docstring instead? Eg:

PG_MODULE(foomodule)

is mandatory, there you can to your magic, and optional:

<snip>

I like it, but I'm not sure there's enough consensus for that. I've
suggested before including install info inside the modules themselves
but there doesn't seem to be much interest in that.

I am not suggesting to try to go all the way, just to make sure that
your current patch fits into that direction.

Apart from that there's issues with implementation. The Linux kernel
can do it easily because it knows it will be using ELF, thus can use
sections to store this info. Postgresql has to support many more types,
making things like this tricky (but not impossible).

PostgreSQL already requires symbol loading functionality
for V1 function signatures, so per-module symbols won't be
much burden.

Personally I'd like postgres to move to a system where external modules
can easily be installed, uninstalled and upgraded. However, I've not
seen the demand yet.

Demand happens only when users get used to such niceties on some
other databases. Considering that PostgreSQL is extensibility-wise
most advanced database and anything we offer is worlds best,
there won't be any demand in years to come.

I rather think we should create that demand. Tasks like

- see what modules are installed in database.
- install module
- remove module

are rather clunky in current setup. Making them easier would be good thing.

Ofcourse, its easy to tell others to do things. I'll try to hack on that area
myself also. If not earlier then maybe on Summit Code Sprint at least.

--
marko

"Marko Kreen" <markokr@gmail.com> writes:

Could you serve this as special docstring instead? Eg:
PG_MODULE(foomodule)

I have no objection to that, and see no real implementation problem with
it: we just add a "const char *" field to the magic block.  The other
stuff seems too blue-sky, and I'm not even sure that it's the right
direction to proceed in.  Marko seems to be envisioning a future where
an extension module is this binary blob with install/deinstall/etc code
all hardwired into it.  I don't like that a bit.  I think the current
scheme with separate SQL scripts is a *good* thing, because it makes it
a lot easier for users to tweak the SQL definitions, eg, install the
functions into a non-default schema.  Also, I don't have a problem
imagining extension modules that contain no C code, just PL functions
--- so the SQL script needs to be considered the primary piece of the
module, not the shared library.

Is it worth adding a module name to the magic block, or should we just
leave well enough alone? It's certainly not something foreseen as part
of the purpose of that block. In the absence of some fairly concrete
ideas what to do with it, I'm probably going to vote keep-it-simple.

regards, tom lane

On Wed, May 31, 2006 at 11:14:27AM -0400, Tom Lane wrote:

Is it worth adding a module name to the magic block, or should we just
leave well enough alone? It's certainly not something foreseen as part
of the purpose of that block. In the absence of some fairly concrete
ideas what to do with it, I'm probably going to vote keep-it-simple.

I actually considered it while writing the patch but decided against
given the general tendancy against putting extra info into the modules
in general...

Personally I think it's a good idea, except: where is this info going
to be displayed or used?

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

On 5/31/06, Tom Lane <tgl@sss.pgh.pa.us> wrote:

"Marko Kreen" <markokr@gmail.com> writes:

Could you serve this as special docstring instead? Eg:
PG_MODULE(foomodule)

I have no objection to that, and see no real implementation problem with
it: we just add a "const char *" field to the magic block. The other
stuff seems too blue-sky, and I'm not even sure that it's the right
direction to proceed in.

It was not blue-sky, it was handwaving :)

Marko seems to be envisioning a future where
an extension module is this binary blob with install/deinstall/etc code
all hardwired into it.  I don't like that a bit.  I think the current
scheme with separate SQL scripts is a *good* thing, because it makes it
a lot easier for users to tweak the SQL definitions, eg, install the
functions into a non-default schema.  Also, I don't have a problem
imagining extension modules that contain no C code, just PL functions
--- so the SQL script needs to be considered the primary piece of the
module, not the shared library.

I'll later post a list of ideas that we can hopefully agree on
and discuss them further.

Is it worth adding a module name to the magic block, or should we just
leave well enough alone? It's certainly not something foreseen as part
of the purpose of that block. In the absence of some fairly concrete
ideas what to do with it, I'm probably going to vote keep-it-simple.

Yes, if we want to keep separate SQL for modules then
putting stuff into .so is pointless.

--
marko

On Wed, May 31, 2006 at 11:14:27AM -0400, Tom Lane wrote:
<snip>

...  The other
stuff seems too blue-sky, and I'm not even sure that it's the right
direction to proceed in.  Marko seems to be envisioning a future where
an extension module is this binary blob with install/deinstall/etc code
all hardwired into it.  I don't like that a bit.  I think the current
scheme with separate SQL scripts is a *good* thing, because it makes it
a lot easier for users to tweak the SQL definitions, eg, install the
functions into a non-default schema.  Also, I don't have a problem
imagining extension modules that contain no C code, just PL functions
--- so the SQL script needs to be considered the primary piece of the
module, not the shared library.

While you do have a good point about non-binary modules, our module
handling need some help IMHO. For example, the current hack for CREATE
LANGUAGE to fix things caused by old pg_dumps. I think that's the
totally wrong approach long term, I think the pg_dump shouldn't be
including the CREATE LANGUAGE statement at all, but should be saying
something like "INSTALL plpgsql" and pg_restore works out what is
needed for that module.

The above requires getting a few bits straight:

1. When given the name of an external module, you need to be able to
find the SQL commands needed to make it work.

2. You need to be able to tell if something is installed already or
not.

3. You need to be able to uninstall it again. Why do we rely on
hand-written uninstall scripts when we have a perfectly functional
dependancy mechanism that can adequatly track what was added and remove
it again on demand.

With these in place, upgrades across versions of postgres could become
a lot easier. People using tsearch2 now would get only "INSTALL
tsearch2" in their dumps and when they upgrade to 8.2 they get the new
definitions for tsearch using GIN. No old definitions to confuse people
or the database. (Note: I'm not sure if tsearch would be compatable at
the query level, but that's not relevent to the point I'm making).

We could get straight into discussions of mechanism, but it would be
nice to know if people think the above is a worthwhile idea.

Have a ncie day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

[ moving this thread to -hackers ]

Martijn van Oosterhout <kleptog@svana.org> writes:

While you do have a good point about non-binary modules, our module
handling need some help IMHO. For example, the current hack for CREATE
LANGUAGE to fix things caused by old pg_dumps. I think that's the
totally wrong approach long term, I think the pg_dump shouldn't be
including the CREATE LANGUAGE statement at all, but should be saying
something like "INSTALL plpgsql" and pg_restore works out what is
needed for that module.

There's a lot to be said for this, but I keep having the nagging
feeling that people are equating "module" with "shared library", which
seems far from sufficiently general. I'd like to see "module" mean
"an arbitrary collection of SQL objects". So I think the raw definition
sought by your "INSTALL" would always be a SQL script, and any shared
libs that might come along with that are secondary. The idea of using
pg_depend to manage UNINSTALL is an excellent one.

1. When given the name of an external module, you need to be able to
find the SQL commands needed to make it work.

No problem, the name is the name of a SQL script file stored in a specific
installation directory.

2. You need to be able to tell if something is installed already or
not.

pg_module system catalog. You'd need this anyway since there has to be
some representation of the "module object" in the catalogs for its
component objects to have pg_depend dependencies on.

With these in place, upgrades across versions of postgres could become
a lot easier. People using tsearch2 now would get only "INSTALL
tsearch2" in their dumps and when they upgrade to 8.2 they get the new
definitions for tsearch using GIN. No old definitions to confuse people
or the database. (Note: I'm not sure if tsearch would be compatable at
the query level, but that's not relevent to the point I'm making).

Let's see, I guess pg_dump would have to be taught to ignore any objects
that it can see are directly dependent on a module object. What about
indirect dependencies though? The exact semantics don't seem clear to me.

Also, this seems to be getting into territory that Oracle has already
trod --- someone should study exactly what they do for PL/SQL modules
and whether we want to be compatible or not. Perhaps there's even
something in SQL2003 about it?

regards, tom lane

On Wednesday 31 May 2006 13:24, Martijn van Oosterhout wrote:

On Wed, May 31, 2006 at 11:14:27AM -0400, Tom Lane wrote:

Is it worth adding a module name to the magic block, or should we just
leave well enough alone? It's certainly not something foreseen as part
of the purpose of that block. In the absence of some fairly concrete
ideas what to do with it, I'm probably going to vote keep-it-simple.

I actually considered it while writing the patch but decided against
given the general tendancy against putting extra info into the modules
in general...

Personally I think it's a good idea, except: where is this info going
to be displayed or used?

Marko's suggestion on producing a list of installed modules comes to mind, and
I suspect tools like pgadmin or ppa will want to be able to show this
information.

--
Robert Treat
Build A Brighter Lamp :: Linux Apache {middleware} PostgreSQL

On Wed, May 31, 2006 at 05:33:44PM -0400, Tom Lane wrote:

Martijn van Oosterhout <kleptog@svana.org> writes:

While you do have a good point about non-binary modules, our module
handling need some help IMHO. For example, the current hack for CREATE
LANGUAGE to fix things caused by old pg_dumps. I think that's the
totally wrong approach long term, I think the pg_dump shouldn't be
including the CREATE LANGUAGE statement at all, but should be saying
something like "INSTALL plpgsql" and pg_restore works out what is
needed for that module.

There's a lot to be said for this, but I keep having the nagging
feeling that people are equating "module" with "shared library", which
seems far from sufficiently general. I'd like to see "module" mean
"an arbitrary collection of SQL objects".

I agree that module is often used interchangably with shared library.
We need to handle the other case too. It would be a lot easier if we
had an example of an SQL only module, since contrib doesn't appear to
have one (at first glance anyway).

So I think the raw definition
sought by your "INSTALL" would always be a SQL script, and any shared
libs that might come along with that are secondary. The idea of using
pg_depend to manage UNINSTALL is an excellent one.

Well, in that case I'd like to give some concrete suggestions:

1. The $libdir in future may be used to find SQL scripts as well as
shared libraries. They'll have different extensions so no possibility
of conflict.

2. Create something like "BEGIN MODULE xxx" which starts a transaction
and marks any objects created within it as owned by module "xxx". I
think it should be tied to a transaction level to avoid half installed
things, but maybe people would prefer it to work more like schemas.

pg_module system catalog. You'd need this anyway since there has to be
some representation of the "module object" in the catalogs for its
component objects to have pg_depend dependencies on.

Ack. "Owned by" in the above sense means that the object depends on the
module. You could do it the other way round (module depends on object)
but that makes it harder to change things manually. DROP MODULE would
work easier too.

Let's see, I guess pg_dump would have to be taught to ignore any objects
that it can see are directly dependent on a module object. What about
indirect dependencies though? The exact semantics don't seem clear to me.

At a base level, you could definitly drop the functions. Dropping types
is harder because columns might be using them. Normally we use CASCADE
to specify that.

Also, this seems to be getting into territory that Oracle has already
trod --- someone should study exactly what they do for PL/SQL modules
and whether we want to be compatible or not. Perhaps there's even
something in SQL2003 about it?

Probably a good idea...

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/