Обсуждение: BUG #7748: "drop owned by" fails with error message: "unrecognized object class: 1262"

The following bug has been logged on the website:

Bug reference:      7748
Logged by:          Thomas Kellerer
Email address:      spam_eater@gmx.net
PostgreSQL version: 9.2.2
Operating system:   Windows XP, 32bit
Description:        =


Using "drop owned by someuser" in 9.2.2 fails with the error =

"unrecognized object class: 1262"

This is what happens on the commandline. The PostgreSQL cluster was
initialized right before running these steps using the command:

initdb -D pgdata --lc-messages=3DEnglish -U postgres -E UTF8 -A md5

This is what happens when using "drop owned by":

c:\etc\postgres-9.2>psql
psql (9.2.2)
Type "help" for help.

postgres=3D# create user testuser with password 'secret';
CREATE ROLE
postgres=3D# create database testdb owner testuser;
CREATE DATABASE
postgres=3D# \q

c:\etc\postgres-9.2>psql -U testuser testdb
psql (9.2.2)
Type "help" for help.

testdb=3D> create table foobar (id integer);
CREATE TABLE
testdb=3D> drop owned by testuser;
ERROR:  unrecognized object class: 1262
postgres=3D# select version();
                           version
-------------------------------------------------------------
 PostgreSQL 9.2.2, compiled by Visual C++ build 1600, 32-bit
(1 row)

Dropping the objects manually works without problems.

spam_eater@gmx.net writes:
> postgres=# create user testuser with password 'secret';
> CREATE ROLE
> postgres=# create database testdb owner testuser;
> CREATE DATABASE
> testdb=> drop owned by testuser;
> ERROR:  unrecognized object class: 1262

I can reproduce this in all versions back to 8.3.  In 8.2, the role's
ownership of the database is silently ignored, which I think was the
design intention.  I doubt that we want DROP OWNED dropping whole
databases.  At most maybe we should raise a NOTICE?

            regards, tom lane

Tom Lane wrote on 09.12.2012 17:43:
> spam_eater@gmx.net writes:
>> postgres=# create user testuser with password 'secret';
>> CREATE ROLE
>> postgres=# create database testdb owner testuser;
>> CREATE DATABASE
>> testdb=> drop owned by testuser;
>> ERROR:  unrecognized object class: 1262
>
> I can reproduce this in all versions back to 8.3.  In 8.2, the role's
> ownership of the database is silently ignored, which I think was the
> design intention.  I doubt that we want DROP OWNED dropping whole
> databases.  At most maybe we should raise a NOTICE?
>

I don't want to drop the database, just all objects that are created by that user (tables, schemas, ...)
(In my example I first create a table)

And this has definitely worked before, otherwise my unit tests would have failed all the time.

Regards
Thomas

Tom Lane wrote on 09.12.2012 17:43:
> spam_eater@gmx.net writes:
>> postgres=# create user testuser with password 'secret';
>> CREATE ROLE
>> postgres=# create database testdb owner testuser;
>> CREATE DATABASE
>> testdb=> drop owned by testuser;
>> ERROR:  unrecognized object class: 1262
>
> I can reproduce this in all versions back to 8.3.  In 8.2, the role's
> ownership of the database is silently ignored, which I think was the
> design intention.  I doubt that we want DROP OWNED dropping whole
> databases.  At most maybe we should raise a NOTICE?
>

Just for clarification, this is how it worked in 9.1 (and I'm pretty sure in 9.2.1 as well):

First I create a testuser and the database:

C:\>psql -X -U postgres postgres
psql (9.1.3)

postgres=# create database testdb owner testuser;
ERROR:  role "testuser" does not exist
postgres=# create user testuser with password 'secret';
CREATE ROLE
postgres=# create database testdb owner testuser;
CREATE DATABASE
postgres=# \q

Then I log in as the testuser, create a table and drop all objects again:

C:\>psql -X -U testuser testdb
Password for user testuser:
psql (9.1.3)

testdb=> create table foobar (id integer);
CREATE TABLE
testdb=> drop owned by testuser;
DROP OWNED
testdb=>

Tom Lane wrote:
>=20
> spam_eater@gmx.net writes:
> > postgres=3D# create user testuser with password 'secret';
> > CREATE ROLE
> > postgres=3D# create database testdb owner testuser;
> > CREATE DATABASE
> > testdb=3D> drop owned by testuser;
> > ERROR:  unrecognized object class: 1262
>=20
> I can reproduce this in all versions back to 8.3.  In 8.2, the role's
> ownership of the database is silently ignored, which I think was the
> design intention.  I doubt that we want DROP OWNED dropping whole
> databases.  At most maybe we should raise a NOTICE?

I broke it recently: fe3b5eb08

Got a one day old guy at home, can't look at this for at least a
week, sorry.

--=20
=C1lvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sun, Dec 9, 2012 at 11:13 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Tom Lane wrote:
>>
>> spam_eater@gmx.net writes:
>> > postgres=3D# create user testuser with password 'secret';
>> > CREATE ROLE
>> > postgres=3D# create database testdb owner testuser;
>> > CREATE DATABASE
>> > testdb=3D> drop owned by testuser;
>> > ERROR:  unrecognized object class: 1262
>>
>> I can reproduce this in all versions back to 8.3.  In 8.2, the role's
>> ownership of the database is silently ignored, which I think was the
>> design intention.  I doubt that we want DROP OWNED dropping whole
>> databases.  At most maybe we should raise a NOTICE?
>
> I broke it recently: fe3b5eb08
>

whatever is the right way to solve this... shouldn't we do something
similar in shdepReassignOwned() in which we are still ignoring shared
objects?

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitaci=F3n
Phone: +593 4 5107566         Cell: +593 987171157

On Sun, Dec 9, 2012 at 8:28 PM, Jaime Casanova <jaime@2ndquadrant.com> wrote:
> On Sun, Dec 9, 2012 at 11:13 PM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
>> Tom Lane wrote:
>>>
>>> spam_eater@gmx.net writes:
>>> > postgres=# create user testuser with password 'secret';
>>> > CREATE ROLE
>>> > postgres=# create database testdb owner testuser;
>>> > CREATE DATABASE
>>> > testdb=> drop owned by testuser;
>>> > ERROR:  unrecognized object class: 1262
>>>
>>> I can reproduce this in all versions back to 8.3.  In 8.2, the role's
>>> ownership of the database is silently ignored, which I think was the
>>> design intention.  I doubt that we want DROP OWNED dropping whole
>>> databases.  At most maybe we should raise a NOTICE?
>>
>> I broke it recently: fe3b5eb08
>>
>
> whatever is the right way to solve this... shouldn't we do something
> similar in shdepReassignOwned() in which we are still ignoring shared
> objects?

Based on the commit message, it seems like it should *only* be in
shdepReassignOwned.

However, when I put it there it fails, as the code that does the
ownership change cannot deal with tablespaces (or databases)

ERROR:  unexpected classid 1213

I am unsure of the goal here.  The docs clearly say that only objects
in the current database are affected, so why are we even trying to do
something with tablespaces (or databases), which do not live in any
database?  And if we want to change the contract to allow it to climb
out of the current database, why limit it to shared objects rather
than crawling all databases?

Cheers,

Jeff

On Tue, Dec 11, 2012 at 2:04 PM, Jeff Janes <jeff.janes@gmail.com> wrote:
>
> I am unsure of the goal here.  The docs clearly say that only objects
> in the current database are affected, so why are we even trying to do
> something with tablespaces (or databases), which do not live in any
> database?  And if we want to change the contract to allow it to climb
> out of the current database, why limit it to shared objects rather
> than crawling all databases?
>

ok. you're right, what i suggested before of making something similar
on DROP ASSIGNED is actually a violation of the POLA.
about your question, i guess the compromise =C1lvaro was taken here is
to affect all objects that could be *seen* from this database you
can't climb to other objects in other databases because they can't be
seen.

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitaci=F3n
Phone: +593 4 5107566         Cell: +593 987171157

On Sunday, December 9, 2012, Alvaro Herrera wrote:

> Tom Lane wrote:
> >
> > spam_eater@gmx.net <javascript:;> writes:
> > > postgres=# create user testuser with password 'secret';
> > > CREATE ROLE
> > > postgres=# create database testdb owner testuser;
> > > CREATE DATABASE
> > > testdb=> drop owned by testuser;
> > > ERROR:  unrecognized object class: 1262
> >
> > I can reproduce this in all versions back to 8.3.  In 8.2, the role's
> > ownership of the database is silently ignored, which I think was the
> > design intention.  I doubt that we want DROP OWNED dropping whole
> > databases.  At most maybe we should raise a NOTICE?
>
> I broke it recently: fe3b5eb08
>
> Got a one day old guy at home, can't look at this for at least a
> week, sorry.
>

Since back-branch releases are coming up, I think fe3b5eb08 and it's
analogues in all branches should be reverted.

The issue it was intended to solve was not really a bug in the first place,
and this patch didn't solve it anyway. But it introduced new behavior (into
all supported branches) which certainly is a bug.


I don't think the original issue it was intended to solve can be solved
easily, either. It would be easy to fix the code to add table-spaces to the
list of things to be reassigned, but the code that does the actual
reassignment can't deal with table-spaces anyway.



Cheers,


Jeff

Jeff Janes escribi=F3:

> Since back-branch releases are coming up, I think fe3b5eb08 and it's
> analogues in all branches should be reverted.

Yes, I have this on my list of things to do before the next minor
release.

--=20
=C1lvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Jeff Janes escribió:

> Since back-branch releases are coming up, I think fe3b5eb08 and it's
> analogues in all branches should be reverted.
>
> The issue it was intended to solve was not really a bug in the first place,
> and this patch didn't solve it anyway. But it introduced new behavior (into
> all supported branches) which certainly is a bug.

I disagree with this assessment, and propose the attached patch instead.

The documentation for DROP OWNED says in its opening paragraph "Any
privileges granted to the given roles on objects in the current database
will also be revoked."  So commit fe3b5eb08 was correct, though
incomplete; DROP OWNED is really supposed to operate on tablespaces and
databases.  Not drop them, mind you --- but if the user has been granted
privileges on them, those should be revoked.

On the other hand, running REASSIGN OWNED means to reassign ownership of
objects to some other user.  There is no reason this cannot be done to
shared objects as well as local objects.  I note, though, that REASSIGN
OWNED's documentation mentions "objects in the current database"; but in
spirit, the point of it is to drop users.  So applying to shared objects
seems within charter to me.

So what this patch does, is ensure that both REASSIGN OWNED and DROP
OWNED operate on shared objects: the former changes ownership of those
shared objects to the target user; and the latter removes granted
privileges on those shared objects.


This is the patch for the master branch; I have not tried to backpatch
it yet.  Conflicts are expected due to the refactoring of ALTER commands
by KaiGai.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera escribi=F3:

> On the other hand, running REASSIGN OWNED means to reassign ownership o=
f
> objects to some other user.  There is no reason this cannot be done to
> shared objects as well as local objects.  I note, though, that REASSIGN
> OWNED's documentation mentions "objects in the current database"; but i=
n
> spirit, the point of it is to drop users.  So applying to shared object=
s
> seems within charter to me.

FWIW, the docs for REASSIGN OWNED have a note at the bottom:
 "The REASSIGN OWNED command does not affect the ownership of any
 databases owned by the role."

This was added because Josh Berkus commanded us to:
http://www.postgresql.org/message-id/4BE2367C.30605@agliodbs.com

As far as I'm concerned, that note should be removed.  Also, there's
this bit:

    "Because REASSIGN OWNED only affects the objects in the current
    database, it is usually necessary to execute this command in each
    database that contains objects owned by a role that is to be removed."

This would be reworded thusly:

    "Because REASSIGN OWNED does not affect objects in other
    databases, it is usually necessary to execute this command in each
    database that contains objects owned by a role that is to be removed."

--=20
=C1lvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> I disagree with this assessment, and propose the attached patch instead.

+1 for this approach in HEAD ...

> This is the patch for the master branch; I have not tried to backpatch
> it yet.  Conflicts are expected due to the refactoring of ALTER commands
> by KaiGai.

... but I'm worried that you'd need a substantially larger patch in back
branches, and accordingly I'm not sure this is what to do in the back
branches.  Without that refactoring, you might need to duplicate more
code, so it might be safer to just revert as Jeff suggested.

The documentation also needs more work than what you suggest in your
followup.  The command reference pages should explicitly say that they
operate on both objects in the current database and shared objects.
Explicitly defining shared objects as databases and tablespaces
wouldn't hurt any.

It also strikes me that where you suggest "Because REASSIGN OWNED does
not affect objects in other databases ...", it might be clearer to say
"Because REASSIGN OWNED does not affect objects within other databases
...", which makes the idea of containment a little stronger.

            regards, tom lane

Tom Lane escribi=F3:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > I disagree with this assessment, and propose the attached patch inste=
ad.
>=20
> +1 for this approach in HEAD ...
>=20
> > This is the patch for the master branch; I have not tried to backpatc=
h
> > it yet.  Conflicts are expected due to the refactoring of ALTER comma=
nds
> > by KaiGai.
>=20
> ... but I'm worried that you'd need a substantially larger patch in bac=
k
> branches, and accordingly I'm not sure this is what to do in the back
> branches.  Without that refactoring, you might need to duplicate more
> code, so it might be safer to just revert as Jeff suggested.

I had a look at what it'd take to backpatch.  For REASSIGN OWNED, you're
right that it'd require some refactoring, and it's probably not
worthwhile (the code is not really all that complicated).  However, for
DROP OWNED the proposed hunks apply fine.  Only 8.3 needs a different
patch, but it's only because whitespace is different.

So what we would end up with, is that DROP OWNED works for shared
objects (i.e. grants on tablespaces and databases are revoked), but
REASSIGN OWNED does not; so you're forced to do ALTER
DATABASE/TABLESPACE SET OWNER.

Since it's the grants that are more likely to cause headaches than
ownership when trying to drop users, I suggest that applying those
patches is the most convenient.  (We know that this is a real problem
because of the bug reports we've gotten.)

> The documentation also needs more work than what you suggest in your
> followup.  The command reference pages should explicitly say that they
> operate on both objects in the current database and shared objects.
> Explicitly defining shared objects as databases and tablespaces
> wouldn't hurt any.
>=20
> It also strikes me that where you suggest "Because REASSIGN OWNED does
> not affect objects in other databases ...", it might be clearer to say
> "Because REASSIGN OWNED does not affect objects within other databases
> ...", which makes the idea of containment a little stronger.

Okay, I will revise those proposed docs changes.

--=20
=C1lvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> I had a look at what it'd take to backpatch.  For REASSIGN OWNED, you're
> right that it'd require some refactoring, and it's probably not
> worthwhile (the code is not really all that complicated).  However, for
> DROP OWNED the proposed hunks apply fine.  Only 8.3 needs a different
> patch, but it's only because whitespace is different.

> So what we would end up with, is that DROP OWNED works for shared
> objects (i.e. grants on tablespaces and databases are revoked), but
> REASSIGN OWNED does not; so you're forced to do ALTER
> DATABASE/TABLESPACE SET OWNER.

> Since it's the grants that are more likely to cause headaches than
> ownership when trying to drop users, I suggest that applying those
> patches is the most convenient.  (We know that this is a real problem
> because of the bug reports we've gotten.)

Seems reasonable to me.

            regards, tom lane

Pushed, thanks.

Jeff, Thomas, Jaime: please have a look and let me know what you think.

--=20
=C1lvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Jan 28, 2013 at 2:37 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Pushed, thanks.
>
> Jeff, Thomas, Jaime: please have a look and let me know what you think.

I've tested it in the 9_2_STABLE branch and found no problems.

Thanks,

Jeff