Re: adminpack and pg_catalog

Why does adminpack install functions into pg_catalog? This is
inconsistent with the rest of the contrib/ packages, not to mention the
definition of pg_catalog itself (which ought to hold builtin object
definitions). And as AndrewSN pointed out on IRC, it also breaks
pg_dump.

-Neil

On Thu, Oct 19, 2006 at 02:37:34PM -0400, Neil Conway wrote:
> Why does adminpack install functions into pg_catalog? This is
> inconsistent with the rest of the contrib/ packages, not to mention the
> definition of pg_catalog itself (which ought to hold builtin object
> definitions). And as AndrewSN pointed out on IRC, it also breaks
> pg_dump.

When we wrote newsysviews we faced the same dilema of where to put
things. We ultimately decided on a pg_ schema in the hope that no one
else would use that name (we also prefaced everything in the schema with
pg_, but a big reason for that was potential inclusion into core).
--
Jim Nasby jim(at)nasby(dot)net
EnterpriseDB http://enterprisedb.com 512.569.9461 (cell)

On 19/10/06 19:37, "Neil Conway" <neilc(at)samurai(dot)com> wrote:

> Why does adminpack install functions into pg_catalog? This is
> inconsistent with the rest of the contrib/ packages, not to mention the
> definition of pg_catalog itself (which ought to hold builtin object
> definitions).

The adminpack was originally written and intended to become builtin
functions, and in fact most of it has. pg_catalog was used to ensure
compatibility in the future, and because it's the only schema we can be sure
we'll find in any given database.

> And as AndrewSN pointed out on IRC, it also breaks
> pg_dump.

It does? In what way? I don't recall ever having any trouble restoring dumps
in the past.

Regards, Dave.

Neil Conway wrote:
> Why does adminpack install functions into pg_catalog? This is
> inconsistent with the rest of the contrib/ packages, not to mention the
> definition of pg_catalog itself (which ought to hold builtin object
> definitions). And as AndrewSN pointed out on IRC, it also breaks
> pg_dump.
>
Having pg_dump not saving the function definitions is an intended
behaviour. Actually, this was different with admin80, and restoring a
8.0 backup to a 8.1 server will throw several errors now.
I'd consider installing contrib modules as an act of installation, not
something that backup/restore should perform (finally, pg_restore isn't
able to do so, since it can't provide the dll/lib module).

Regards,
Andreas

On Fri, 2006-10-20 at 11:50 +0200, Andreas Pflug wrote:
> Having pg_dump not saving the function definitions is an intended
> behaviour.

The manual defines the pg_catalog schema as containing "the system
tables and all the built-in data types, functions, and
operators" (section 5.7.5). adminpack is none of the above, so I don't
think it should be located in pg_catalog.

> I'd consider installing contrib modules as an act of installation, not
> something that backup/restore should perform

AFAICS this is inconsistent with how every other contrib module behaves:
installing the contrib module into a database results in DDL for that
contrib module being included in pg_dump's output.

> (finally, pg_restore isn't able to do so, since it can't provide
> the dll/lib module).

This is not related to adminpack per se: pg_dump is never be able to
provide the shared object for any C language UDF. By your logic, pg_dump
shouldn't emit DDL for any such function.

-Neil

On Fri, 2006-10-20 at 05:52 +0100, Dave Page wrote:
> The adminpack was originally written and intended to become builtin
> functions

This is not unique to adminpack: several contrib modules might
eventually become (or have already become) builtins, but adminpack is
the only module that defines objects in the pg_catalog schema.

> pg_catalog was used to ensure compatibility in the future

This is again not unique to adminpack. If users install a contrib module
into a schema that is in their search path, then if the module is
subsequently moved to pg_catalog, no queries will need to be changed. If
users install a module into some schema that isn't in their search path
and use explicit schema references, they are essentially asking for
their application to break if the object moves to a different schema.

> > And as AndrewSN pointed out on IRC, it also breaks pg_dump.
>
> It does? In what way?

It breaks in the sense of "completely not working" :)

% pg_dump | grep file_write
% cd contrib/adminpack
% grep -A1 file_write adminpack.sql
CREATE FUNCTION pg_catalog.pg_file_write(text, text, bool) RETURNS
bigint
AS '$libdir/adminpack', 'pg_file_write'
LANGUAGE C VOLATILE STRICT;
% psql -f adminpack.sql
CREATE FUNCTION
CREATE FUNCTION
CREATE FUNCTION
CREATE FUNCTION
CREATE FUNCTION
CREATE FUNCTION
CREATE FUNCTION
CREATE FUNCTION
% pg_dump | grep file_write
%

-Neil

Neil Conway wrote:
> Why does adminpack install functions into pg_catalog? This is
> inconsistent with the rest of the contrib/ packages, not to mention
> the definition of pg_catalog itself (which ought to hold builtin
> object definitions).

Nothing except initdb should add objects in pg_catalog. AFAICS,
adminpack doesn't have any special requirements, so it should behave
like all other contrib modules.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Neil Conway wrote:
> On Fri, 2006-10-20 at 05:52 +0100, Dave Page wrote:
>
>> The adminpack was originally written and intended to become builtin
>> functions
>>
>
> This is not unique to adminpack: several contrib modules might
> eventually become (or have already become) builtins, but adminpack is
> the only module that defines objects in the pg_catalog schema.
>
.. which appears simply pragmatic, taken that it features server
maintenance functions, not functions usually called from user applications.

>
>> pg_catalog was used to ensure compatibility in the future
>>
>
> This is again not unique to adminpack. If users install a contrib module
> into a schema that is in their search path, then if the module is
> subsequently moved to pg_catalog, no queries will need to be changed. If
> users install a module into some schema that isn't in their search path
> and use explicit schema references, they are essentially asking for
> their application to break if the object moves to a different schema.
>
Please note that adminpack is intended for administrator's use, and
should be robust to (i.e. not dependent on) search path. We previously
had this dependency in pgadmin, and found it sucks. Putting the stuff in
pg_catalog works as desired and has no negative effects (apart from the
contrib not working after pg_dump/pg_restore if not installed, which is
expected behaviour anyway).

However, adminpack was crippled to the edge of usability for me already,
I'm prepared to see it fade away further (Since there's still no
pg_terminate_backend available which is definitely needed, I regularly
need to install my personal adminpack).

Regards,
Andreas

On Fri, 2006-10-20 at 22:59 +0200, Peter Eisentraut wrote:
> Nothing except initdb should add objects in pg_catalog. AFAICS,
> adminpack doesn't have any special requirements, so it should behave
> like all other contrib modules.

Okay. Are there any opinions on whether we should make this change to
contrib/adminpack now (i.e. during the 8.2 beta), later (for 8.3), or
not all at?

-Neil

On Friday 20 October 2006 21:03, Neil Conway wrote:
> On Fri, 2006-10-20 at 22:59 +0200, Peter Eisentraut wrote:
> > Nothing except initdb should add objects in pg_catalog. AFAICS,
> > adminpack doesn't have any special requirements, so it should behave
> > like all other contrib modules.
>
> Okay. Are there any opinions on whether we should make this change to
> contrib/adminpack now (i.e. during the 8.2 beta), later (for 8.3), or
> not all at?
>

Not saying we should or shouldn't, but if we are going to do it, istm we ought
to do it now while we are still in beta. Is there a backwards compatability
issue for older systems? (Not that this doesn't get worse if we release yet
another release with it)

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

Neil Conway <neilc(at)samurai(dot)com> writes:
> On Fri, 2006-10-20 at 22:59 +0200, Peter Eisentraut wrote:
>> Nothing except initdb should add objects in pg_catalog. AFAICS,
>> adminpack doesn't have any special requirements, so it should behave
>> like all other contrib modules.

> Okay. Are there any opinions on whether we should make this change to
> contrib/adminpack now (i.e. during the 8.2 beta), later (for 8.3), or
> not all at?

AFAIR the point of adminpack was to support pgAdmin, which expects those
functions to be in pg_catalog. At some point we might as well just take
it out instead of whack it until it meets some arbitrary restrictions
and isn't at all backwards-compatible anymore.

(No, I don't find these arguments that it mustn't put anything in
pg_catalog to be very compelling ... if we seriously believed that,
we'd have arranged for the system to enforce it.)

regards, tom lane

On Fri, 2006-10-20 at 22:59 +0200, Peter Eisentraut wrote:
> Nothing except initdb should add objects in pg_catalog. AFAICS,
> adminpack doesn't have any special requirements, so it should behave
> like all other contrib modules.

Where are we on this? When this topic was last discussed, the three
alternatives were:

(1) Modify contrib/adminpack to not use the pg_catalog schema,
per the consensus that contrib/ packages installing objects
into that schema is broken behavior

(2) Don't modify contrib/adminpack, for the sake of backward
compatibility

(3) Remove contrib/adminpack from the Postgres distribution

I think the discussion was edging toward #3, but #2 is the only option
that I'm not happy with. Any other opinions out there?

Cheers,

Neil

Neil Conway wrote:
> On Fri, 2006-10-20 at 22:59 +0200, Peter Eisentraut wrote:
>> Nothing except initdb should add objects in pg_catalog. AFAICS,
>> adminpack doesn't have any special requirements, so it should behave
>> like all other contrib modules.
>
> Where are we on this? When this topic was last discussed, the three
> alternatives were:
>
> (1) Modify contrib/adminpack to not use the pg_catalog schema,
> per the consensus that contrib/ packages installing objects
> into that schema is broken behavior
>
> (2) Don't modify contrib/adminpack, for the sake of backward
> compatibility
>
> (3) Remove contrib/adminpack from the Postgres distribution
>
> I think the discussion was edging toward #3, but #2 is the only option
> that I'm not happy with. Any other opinions out there?

Looking back over the thread, it appears that only you and Peter
objected to it as it is now. Tom, Andreas and myself were of the opinion
it was fine as it is, and whilst he didn't comment on how it should be
implemented, Simon made the point that supporting admin tools from the
core distribution was important which I take to mean he is against #3.

Regards, Dave.

On Mon, 2006-11-06 at 09:02 +0000, Dave Page wrote:
> Neil Conway wrote:
> > On Fri, 2006-10-20 at 22:59 +0200, Peter Eisentraut wrote:
> >> Nothing except initdb should add objects in pg_catalog. AFAICS,
> >> adminpack doesn't have any special requirements, so it should behave
> >> like all other contrib modules.
> >
> > Where are we on this? When this topic was last discussed, the three
> > alternatives were:
> >
> > (1) Modify contrib/adminpack to not use the pg_catalog schema,
> > per the consensus that contrib/ packages installing objects
> > into that schema is broken behavior
> >
> > (2) Don't modify contrib/adminpack, for the sake of backward
> > compatibility
> >
> > (3) Remove contrib/adminpack from the Postgres distribution
> >
> > I think the discussion was edging toward #3, but #2 is the only option
> > that I'm not happy with. Any other opinions out there?
>
> Looking back over the thread, it appears that only you and Peter
> objected to it as it is now. Tom, Andreas and myself were of the opinion
> it was fine as it is, and whilst he didn't comment on how it should be
> implemented, Simon made the point that supporting admin tools from the
> core distribution was important which I take to mean he is against #3.

Definitely against #3. [Argument: not just pgAdmin, essential feature]

At the moment we only allow 2 types of table. Approved core catalog
tables and user tables.

ISTM we need 3 types of tables, with the additional type being add-on
system functionality, such as adminpack, but also possibly including
performance logging tables, extended trace tools, security packages
(e.g. Veil) etc etc. We have made provision for user add-ons to allocate
LWlocks and shared memory, but no provision for additional tables.
Surely we must be able to accommodate things as simple as additional
tables?

So we would then have (change names as appropriate)
- core catalog
- catalog extensions
- user tables.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

"Simon Riggs" <simon(at)2ndquadrant(dot)com> writes:
> At the moment we only allow 2 types of table. Approved core catalog
> tables and user tables.

> ISTM we need 3 types of tables, with the additional type being add-on
> system functionality, such as adminpack,

What? The adminpack module only creates functions.

regards, tom lane

On Mon, 2006-11-06 at 13:37 -0500, Tom Lane wrote:
> "Simon Riggs" <simon(at)2ndquadrant(dot)com> writes:
> > At the moment we only allow 2 types of table. Approved core catalog
> > tables and user tables.
>
> > ISTM we need 3 types of tables, with the additional type being add-on
> > system functionality, such as adminpack,
>
> What? The adminpack module only creates functions.

AFAIK the namespace is the issue, not the type of object.

As I mentioned, we can add LWlock extensions but we don't have an
official home for other database objects.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

On Monday 06 November 2006 13:12, Simon Riggs wrote:
> On Mon, 2006-11-06 at 09:02 +0000, Dave Page wrote:
> > Neil Conway wrote:
> > > On Fri, 2006-10-20 at 22:59 +0200, Peter Eisentraut wrote:
> > >> Nothing except initdb should add objects in pg_catalog. AFAICS,
> > >> adminpack doesn't have any special requirements, so it should behave
> > >> like all other contrib modules.
> > >
> > > Where are we on this? When this topic was last discussed, the three
> > > alternatives were:
> > >
> > > (1) Modify contrib/adminpack to not use the pg_catalog schema,
> > > per the consensus that contrib/ packages installing objects
> > > into that schema is broken behavior
> > >
> > > (2) Don't modify contrib/adminpack, for the sake of backward
> > > compatibility
> > >
> > > (3) Remove contrib/adminpack from the Postgres distribution
> > >
> > > I think the discussion was edging toward #3, but #2 is the only option
> > > that I'm not happy with. Any other opinions out there?
> >
> > Looking back over the thread, it appears that only you and Peter
> > objected to it as it is now. Tom, Andreas and myself were of the opinion
> > it was fine as it is, and whilst he didn't comment on how it should be
> > implemented, Simon made the point that supporting admin tools from the
> > core distribution was important which I take to mean he is against #3.
>
> Definitely against #3. [Argument: not just pgAdmin, essential feature]
>

While I don't disagree that this is an important feature, the fact that it is
being designed with pgadmin specific backwards compatability (for example the
functions that rename core functions) leaves me dubious as to it being a more
general solution. Because of that I would be comfortable with acting on #3.

Now, if I ignore the above, and focus on that I would like to see this
functionality because it helps me with phppgadmin, then I would lean toward
#1 (for a number of reasons really)

Personally I think I'd rather see the whole thing pulled, renamed to its own
schema, and toss in a version function and a kill backend function and let it
go on its merry way... in any case #2 just seems to be the worst of all
possibilities.

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

Robert Treat wrote:
> While I don't disagree that this is an important feature, the fact that it is
> being designed with pgadmin specific backwards compatability (for example the
> functions that rename core functions) leaves me dubious as to it being a more
> general solution. Because of that I would be comfortable with acting on #3.

The rename happened because when one of the functions was moved into
core, someone decided to rename it along the way which broke
compatibility with previous versions of pgAdmin.

Whilst I won't in any way deny that the adminpack was specifically
written for pgAdmin though, I have no problem with it being enhanced to
provide non-core functionality for other admin tools in the future... in
fact I would encourage it. I think it's better to support as many admin
tools as possible from one contrib module, rather than for each tool to
have it's own.

Regards Dave.