Horton

Thread

  1. Bug #6593, extensions, and proposed new patch policy

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2012-04-18T15:41:40Z

    Hackers,

Per bug #6593, REASSIGN OWNED fails when the affected role owns an
extension.  This would be trivial to fix if extensions had support code
for ALTER EXTENSION / OWNER, but they don't.  So the only back-patchable
fix right now seems to be to throw an error on REASSIGN OWNED when the 
user owns an extension.  (If anyone wants to claim that we ought to work
on a real fix that allows changing the owner internally from REASSIGN
OWNED, without introducing ALTER EXTENSION support for doing so, let me
know and I'll see about it.)

In HEAD we can do the more invasive fix of actually adding support code
for changing an extension's owner.  And it seems to me that, going 
further, we should have a policy that any ownable object type we add 
must come with appropriate support for changing owner.

Thoughts?

-- 
Álvaro Herrera <alvherre@alvh.no-ip.org>

  2. Re: Bug #6593, extensions, and proposed new patch policy

    Robert Haas <robertmhaas@gmail.com> — 2012-04-18T16:05:03Z

    On Wed, Apr 18, 2012 at 11:41 AM, Alvaro Herrera
<alvherre@alvh.no-ip.org> wrote:
> Per bug #6593, REASSIGN OWNED fails when the affected role owns an
> extension.  This would be trivial to fix if extensions had support code
> for ALTER EXTENSION / OWNER, but they don't.  So the only back-patchable
> fix right now seems to be to throw an error on REASSIGN OWNED when the
> user owns an extension.  (If anyone wants to claim that we ought to work
> on a real fix that allows changing the owner internally from REASSIGN
> OWNED, without introducing ALTER EXTENSION support for doing so, let me
> know and I'll see about it.)

I would be OK with the latter.

> In HEAD we can do the more invasive fix of actually adding support code
> for changing an extension's owner.  And it seems to me that, going
> further, we should have a policy that any ownable object type we add
> must come with appropriate support for changing owner.

+1.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  3. Re: Bug #6593, extensions, and proposed new patch policy

    Alvaro Herrera <alvherre@commandprompt.com> — 2012-04-18T21:27:27Z

    Excerpts from Robert Haas's message of mié abr 18 13:05:03 -0300 2012:
> On Wed, Apr 18, 2012 at 11:41 AM, Alvaro Herrera
> <alvherre@alvh.no-ip.org> wrote:
> > Per bug #6593, REASSIGN OWNED fails when the affected role owns an
> > extension.  This would be trivial to fix if extensions had support code
> > for ALTER EXTENSION / OWNER, but they don't.  So the only back-patchable
> > fix right now seems to be to throw an error on REASSIGN OWNED when the
> > user owns an extension.  (If anyone wants to claim that we ought to work
> > on a real fix that allows changing the owner internally from REASSIGN
> > OWNED, without introducing ALTER EXTENSION support for doing so, let me
> > know and I'll see about it.)
> 
> I would be OK with the latter.

Here's a patch for that.

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

  4. Re: Bug #6593, extensions, and proposed new patch policy

    Robert Haas <robertmhaas@gmail.com> — 2012-04-18T22:00:57Z

    On Wed, Apr 18, 2012 at 5:27 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
> Excerpts from Robert Haas's message of mié abr 18 13:05:03 -0300 2012:
>> On Wed, Apr 18, 2012 at 11:41 AM, Alvaro Herrera
>> <alvherre@alvh.no-ip.org> wrote:
>> > Per bug #6593, REASSIGN OWNED fails when the affected role owns an
>> > extension.  This would be trivial to fix if extensions had support code
>> > for ALTER EXTENSION / OWNER, but they don't.  So the only back-patchable
>> > fix right now seems to be to throw an error on REASSIGN OWNED when the
>> > user owns an extension.  (If anyone wants to claim that we ought to work
>> > on a real fix that allows changing the owner internally from REASSIGN
>> > OWNED, without introducing ALTER EXTENSION support for doing so, let me
>> > know and I'll see about it.)
>>
>> I would be OK with the latter.
>
> Here's a patch for that.

Looks sane on a quick once-over.  I do wonder about the comment,
though.  If we add ALTER EXTENSION .. OWNER, should that try to change
the ownership of the objects contained inside the extension?  Your
comment implies that the answer should be yes, but I'm not totally
convinced...  what if the user has altered the ownership of the
objects manually, for example?  I guess that's a question for another
day, just wondering out loud.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  5. Re: Bug #6593, extensions, and proposed new patch policy

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-04-18T23:54:53Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Apr 18, 2012 at 5:27 PM, Alvaro Herrera
> <alvherre@commandprompt.com> wrote:
>> Here's a patch for that.

> Looks sane on a quick once-over.  I do wonder about the comment,
> though.  If we add ALTER EXTENSION .. OWNER, should that try to change
> the ownership of the objects contained inside the extension?

I would certainly think that not doing so would violate the principle
of least astonishment.

> Your
> comment implies that the answer should be yes, but I'm not totally
> convinced...  what if the user has altered the ownership of the
> objects manually, for example?

So?  ALTER OWNER doesn't care about the previous ownership of objects,
it just reassigns them as told.  So even if that had been done, I'd
expect the post-ALTER state to be that everything has the new owner.

However, ignoring that issue for the moment, this patch is making me
uncomfortable.  I have a vague recollection that we deliberately omitted
ALTER EXTENSION OWNER because of security or definitional worries.
(Dimitri, does that ring any bells?)  I wonder whether we should insist
that the new owner be a superuser, as the original owner must have been.

			regards, tom lane

  6. Re: Bug #6593, extensions, and proposed new patch policy

    Robert Haas <robertmhaas@gmail.com> — 2012-04-19T01:10:58Z

    On Wed, Apr 18, 2012 at 7:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> However, ignoring that issue for the moment, this patch is making me
> uncomfortable.  I have a vague recollection that we deliberately omitted
> ALTER EXTENSION OWNER because of security or definitional worries.
> (Dimitri, does that ring any bells?)  I wonder whether we should insist
> that the new owner be a superuser, as the original owner must have been.

Don't we have non-superuser extensions, that can be installed with
just DBA privileges?

Anyhow, it seems a bit nannyish, unless I'm missing something.  If the
current owner is a superuser and s/he wants to give the object to a
non-superuser, you can't really stop them.  They can just make the
target user a superuser, give 'em the object, and make them not a
superuser, all in one transaction no less.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  7. Re: Bug #6593, extensions, and proposed new patch policy

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2012-04-19T15:42:00Z

    Hi,

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Apr 18, 2012 at 7:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> However, ignoring that issue for the moment, this patch is making me
>> uncomfortable.  I have a vague recollection that we deliberately omitted
>> ALTER EXTENSION OWNER because of security or definitional worries.
>> (Dimitri, does that ring any bells?)  I wonder whether we should insist
>> that the new owner be a superuser, as the original owner must have been.

I remember not having included any OWNER in my submitted patch, because
I though we didn't really need one as far as the pg_extension entry is
concerned. You added one to better integrate with our standard practice
and for pg_dump purposes IIRC, but that's about it. I don't remember
that we talked about specific extension's objects owner: they get owned
by the user installing them.

> Don't we have non-superuser extensions, that can be installed with
> just DBA privileges?

We do, so the extension owner could well be the database owner and not a
superuser. That can only happens if the extension contains no C coded
parts, obviously, and that's why it's so easy to forget about --- plans
have been discussed to work on per-database module installation, not yet
about non-superuser installing a C module. I think the open possibility
of crashing the whole cluster is driving us not to relax the trusted bit
for the C language ever.

> Anyhow, it seems a bit nannyish, unless I'm missing something.  If the
> current owner is a superuser and s/he wants to give the object to a
> non-superuser, you can't really stop them.  They can just make the
> target user a superuser, give 'em the object, and make them not a
> superuser, all in one transaction no less.

The interesting use case would be reassigning the extension from a
database owner to a superuser so that the database owner can't remove it
nor upgrade it no more. I think we want to allow that.

The reassign from a superuser to a non-superuser can be argued both
ways, unless the extension is marked as superuser = true in the control
file. Which means we should better register that into the catalogs
because the control file can be updated on the file system and that's
out of reach for the cluster.

What about only issuing a WARNING that the extensions are not supported
by reassign owned in 9.1 (and probably 9.2)?

In next versions, we will be able to register the control file superuser
parameter and allow reassigning in a more controlled fashion, both the
catalog entry in pg_extension and the extension's objects.

Maybe rather than adding the control file into the catalogs piece after
piece we should install a copy of it at create/alter time in a database
specific location from the per major version shared location in the OS.
That way the user could be updating the OS shipped file then decide
which cluster and which database to upgrade with that, and you could
have PostGIS 1.5 in database foo and 2.0 in database bar. The OS would
still need to provide for a way to upgrade them separately, but that
would be much better than what we have now.

When doing things this way, we could trust the extension's control file
in cases such as ALTER EXTENSION OWNER, because it's now a private copy
of it.

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

  8. Re: Bug #6593, extensions, and proposed new patch policy

    Alvaro Herrera <alvherre@commandprompt.com> — 2012-04-19T20:30:04Z

    Excerpts from Dimitri Fontaine's message of jue abr 19 12:42:00 -0300 2012:

> What about only issuing a WARNING that the extensions are not supported
> by reassign owned in 9.1 (and probably 9.2)?

Raise a warning and then do what?  While you can continue reassigning
the rest of the objects to someone else, this doesn't help the poor
fella who's just trying to drop the owner of the extension -- it still
can't be dropped.  Moreover, since there's no ALTER OWNER command for
extensions, the user can't just change it to someone else manually.
The only option is to do DROP OWNED, which will drop the extension along
with all the objects that belong to it.  In fact, the documentation
states that the way to drop a user that owns objects is to run REASSIGN
OWNED, then DROP OWNED, (repeat for all databases), then DROP ROLE.  So
if the DBA does that, he might end up dropping the extension by
accident.

Maybe we could just add a protection that the user to which the
extension is reassigned must be a superuser or the database owner.

Remember that we're talking about REASSIGN OWNED here, which will
automatically reassign not only the extension itself, but also the
contained objects.  There is no danger that we will end up with an
inconsistent installation.  Also, if the objects in the extension have
been manually given to someone else, they will stay owned by that other
user, precisely because the code as written does not recurse.

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

  9. Re: Bug #6593, extensions, and proposed new patch policy

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2012-04-19T21:08:38Z

    Alvaro Herrera <alvherre@commandprompt.com> writes:
> Remember that we're talking about REASSIGN OWNED here, which will
> automatically reassign not only the extension itself, but also the
> contained objects.  There is no danger that we will end up with an
> inconsistent installation.  Also, if the objects in the extension have
> been manually given to someone else, they will stay owned by that other
> user, precisely because the code as written does not recurse.

Oh, right, I forgot the scope of the command. Given those bits of missed
context, +1 from me here. Sorry about missing that in my previous email.

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

  10. Re: Bug #6593, extensions, and proposed new patch policy

    Alvaro Herrera <alvherre@commandprompt.com> — 2012-07-03T19:22:59Z

    Excerpts from Alvaro Herrera's message of mié abr 18 18:27:27 -0300 2012:
> Excerpts from Robert Haas's message of mié abr 18 13:05:03 -0300 2012:
> > On Wed, Apr 18, 2012 at 11:41 AM, Alvaro Herrera
> > <alvherre@alvh.no-ip.org> wrote:
> > > Per bug #6593, REASSIGN OWNED fails when the affected role owns an
> > > extension.  This would be trivial to fix if extensions had support code
> > > for ALTER EXTENSION / OWNER, but they don't.  So the only back-patchable
> > > fix right now seems to be to throw an error on REASSIGN OWNED when the
> > > user owns an extension.  (If anyone wants to claim that we ought to work
> > > on a real fix that allows changing the owner internally from REASSIGN
> > > OWNED, without introducing ALTER EXTENSION support for doing so, let me
> > > know and I'll see about it.)
> > 
> > I would be OK with the latter.
> 
> Here's a patch for that.

Applied to master, 9.2 and 9.1.  Sorry for dropping the ball on it.

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

  11. Re: Bug #6593, extensions, and proposed new patch policy

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2012-07-04T08:51:29Z

    Alvaro Herrera <alvherre@commandprompt.com> writes:
> Applied to master, 9.2 and 9.1.

Thank you!

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support