Thread: adminpack and pg_catalog

adminpack and pg_catalog

From
Neil Conway
Date:
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




Re: adminpack and pg_catalog

From
"Jim C. Nasby"
Date:
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@nasby.net
EnterpriseDB      http://enterprisedb.com      512.569.9461 (cell)


Re: adminpack and pg_catalog

From
Dave Page
Date:


On 19/10/06 19:37, "Neil Conway" <neilc@samurai.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.



Re: adminpack and pg_catalog

From
Andreas Pflug
Date:
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



Re: adminpack and pg_catalog

From
Neil Conway
Date:
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




Re: adminpack and pg_catalog

From
Neil Conway
Date:
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




Re: adminpack and pg_catalog

From
Peter Eisentraut
Date:
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/


Re: adminpack and pg_catalog

From
Andreas Pflug
Date:
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



Re: adminpack and pg_catalog

From
Neil Conway
Date:
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




Re: adminpack and pg_catalog

From
Tom Lane
Date:
Neil Conway <neilc@samurai.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


Re: adminpack and pg_catalog

From
"Dave Page"
Date:

-----Original Message-----
From: "Neil Conway" <neilc@samurai.com>
To: "Peter Eisentraut" <peter_e@gmx.net>
Cc: "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>; "Dave Page" <dpage@vale-housing.co.uk>
Sent: 21/10/06 02:03
Subject: Re: [HACKERS] adminpack and pg_catalog

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?

If you change it you will make it useless as pgAdmin won't necessarily find the functions it expects. You might as well
justremove it (which will almost certainly cause delays to pgAdmin - and pgInstallers - release as I'll need to find
timeto put it all back how it was). 

Regards, Dave

Re: adminpack and pg_catalog

From
"Dave Page"
Date:

-----Original Message-----
From: "Neil Conway" <neilc@samurai.com>
To: "Dave Page" <dpage@vale-housing.co.uk>
Cc: "PostgreSQL-development" <pgsql-hackers@postgresql.org>
Sent: 20/10/06 21:19
Subject: Re: adminpack and pg_catalog

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

No,  it does not 'break pg_dump'. What you have shown is that pg_dump doesn't dump adminpack which is exactly what we
want,and perfectly correct behaviour from pg_dump.  

Regards, Dave

Re: adminpack and pg_catalog

From
Peter Eisentraut
Date:
Dave Page wrote:
> If you change it you will make it useless as pgAdmin won't
> necessarily find the functions it expects. You might as well just
> remove it (which will almost certainly cause delays to pgAdmin - and
> pgInstallers - release as I'll need to find time to put it all back
> how it was).

If pgAdmin is going to dictate what we do with adminpack, adminpack 
should rather be shipped with pgAdmin.

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


Re: adminpack and pg_catalog

From
Robert Treat
Date:
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


Re: adminpack and pg_catalog

From
"Simon Riggs"
Date:
On Sat, 2006-10-21 at 12:37 +0200, Peter Eisentraut wrote:
> Dave Page wrote:
> > If you change it you will make it useless as pgAdmin won't
> > necessarily find the functions it expects. You might as well just
> > remove it (which will almost certainly cause delays to pgAdmin - and
> > pgInstallers - release as I'll need to find time to put it all back
> > how it was).
> 
> If pgAdmin is going to dictate what we do with adminpack, adminpack 
> should rather be shipped with pgAdmin.

Admin tools are critical.

adminpack is being made available for any and all admin packages, so all
tools, including for example psql *could* be enhanced to use those.
Whether they do is up to them, but at least their life is potentially
easier.

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




Re: adminpack and pg_catalog

From
Neil Conway
Date:
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
installingobjects           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




Re: adminpack and pg_catalog

From
Dave Page
Date:
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.


Re: adminpack and pg_catalog

From
"Simon Riggs"
Date:
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




Re: adminpack and pg_catalog

From
Tom Lane
Date:
"Simon Riggs" <simon@2ndquadrant.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


Re: adminpack and pg_catalog

From
"Simon Riggs"
Date:
On Mon, 2006-11-06 at 13:37 -0500, Tom Lane wrote:
> "Simon Riggs" <simon@2ndquadrant.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




Re: adminpack and pg_catalog

From
Robert Treat
Date:
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


Re: adminpack and pg_catalog

From
Dave Page
Date:
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.