SEO

This implements a proposal made last november:

http://archives.postgresql.org/pgsql...1/msg00578.php

Basically, it tries to catch people loading modules which belong to the
wrong version or have had certain constants changed, or architechture
mismatches. It's a bit more fine grained though, it currently catches
changes in any of the following:

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

It may be overkill, but better safe than sorry. The only one I'm
ambivalent about is the first one. We don't require a recompile between
minor version changes, or do we?

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

PG_MODULE_MAGIC

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

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFEXmPRIB7bNG8LQkwRArnhAJ9yDTvotqSNfyS0ulqs2G BEMG2UbQCgifmj
hejx7O1Y4khhZMyCafrv2aY=
=tVzB
-----END PGP SIGNATURE-----

Martijn van Oosterhout <kleptog@svana.org> writes:
> This implements a proposal made last november:
> http://archives.postgresql.org/pgsql...1/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

---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

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/
> From each according to his ability. To each according to his ability to litigate.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFEX1IZIB7bNG8LQkwRAhw6AJ4pBLFahZFNzgMA4RoctQ dw7jsT7QCcDZP/
Cm/u30NQLM+G2/cUd+yCEr4=
=o5qA
-----END PGP SIGNATURE-----

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

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

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/
> From each according to his ability. To each according to his ability to litigate.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFEX1pMIB7bNG8LQkwRAmjvAJ9VjBUUhIjYtLlIiMnXxE 5bquUUfQCcD0Z7
FjzX/pYc4aGB2K8NVI+Aolc=
=KGx9
-----END PGP SIGNATURE-----

On 5/8/06, Martijn van Oosterhout <kleptog@svana.org> wrote:
> This implements a proposal made last november:
>
> http://archives.postgresql.org/pgsql...1/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

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

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/
> From each according to his ability. To each according to his ability to litigate.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFEfXgrIB7bNG8LQkwRAnm8AJsETWS9bnzZQZ8lL+r0r6 8Z7ilMPgCePtDF
b0SZzLo79GR1cpSHykDcZgk=
=Tc9X
-----END PGP SIGNATURE-----

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

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

"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

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

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/
> From each according to his ability. To each according to his ability to litigate.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFEfdFrIB7bNG8LQkwRAhCdAJkB6Oqrpnac7Ns6R44PCL vWfPiTvgCgkhCc
OqGpWsLQZImIkLvw3y9DoGw=
=GsCd
-----END PGP SIGNATURE-----