Обсуждение: BUG #13666: REASSIGN OWNED BY doesn't affect the relation underlying composite type

The following bug has been logged on the website:

Bug reference:      13666
Logged by:          Chris Pacejo
Email address:      cpacejo@clearskydata.com
PostgreSQL version: 9.4.4
Operating system:   CentOS 7 (kernel 3.10.0-123.el7.x86_64)
Description:

=# CREATE TYPE foo AS (a integer, b integer);

=# ALTER TYPE foo OWNER TO user1;

=# SELECT typowner, relowner FROM pg_type JOIN pg_class ON typrelid =
relfilenode WHERE typname = 'foo';
-[ RECORD 1 ]---
typowner | 16384
relowner | 16384

=# REASSIGN OWNED BY user1 TO user2;

=# SELECT typowner, relowner FROM pg_type JOIN pg_class ON typrelid =
relfilenode WHERE typname = 'foo';
-[ RECORD 1 ]---
typowner | 8713825
relowner | 16384


This prevents user2 from being able to modify 'foo':

=> ALTER TYPE foo RENAME ATTRIBUTE b TO c;
ERROR:  must be owner of relation foo


Furthermore, while trying to replicate in another database, I encountered:

=# REASSIGN OWNED BY user1 TO user2;
ERROR:  unexpected classid 1418

Not sure if this is related or not.

cpacejo@clearskydata.com wrote:

> =# CREATE TYPE foo AS (a integer, b integer);
>
> =# ALTER TYPE foo OWNER TO user1;
>
> =# SELECT typowner, relowner FROM pg_type JOIN pg_class ON typrelid =
> relfilenode WHERE typname = 'foo';
> -[ RECORD 1 ]---
> typowner | 16384
> relowner | 16384
>
> =# REASSIGN OWNED BY user1 TO user2;
>
> =# SELECT typowner, relowner FROM pg_type JOIN pg_class ON typrelid =
> relfilenode WHERE typname = 'foo';
> -[ RECORD 1 ]---
> typowner | 8713825
> relowner | 16384

Hmm.  I guess we're missing a recursion step somewhere.  I would have
assumed that the pg_class entry would also have a dependency on the
owner and should would have been visited in the initial loop.  Strange.

> Furthermore, while trying to replicate in another database, I encountered:
>
> =# REASSIGN OWNED BY user1 TO user2;
> ERROR:  unexpected classid 1418
>
> Not sure if this is related or not.

Not related.  1418 is pg_user_mapping (FDW stuff).  Not sure what should
happen here; my inclination without thinking too hard is that REASSIGN
OWNED should ignore that object and DROP OWNED should remove it.

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

Alvaro Herrera wrote:
> cpacejo@clearskydata.com wrote:

> > Furthermore, while trying to replicate in another database, I encountered:
> >
> > =# REASSIGN OWNED BY user1 TO user2;
> > ERROR:  unexpected classid 1418
> >
> > Not sure if this is related or not.
>
> Not related.  1418 is pg_user_mapping (FDW stuff).  Not sure what should
> happen here; my inclination without thinking too hard is that REASSIGN
> OWNED should ignore that object and DROP OWNED should remove it.

Ouch, I just noticed that you had already reported this months ago, and
I was even aware of it!  I just pushed a fix for it without mentioning
you as first reporter :-(

I'm now looking at the other problem you reported here.

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

Alvaro Herrera wrote:
> cpacejo@clearskydata.com wrote:

> > =# REASSIGN OWNED BY user1 TO user2;
> >
> > =# SELECT typowner, relowner FROM pg_type JOIN pg_class ON typrelid =
> > relfilenode WHERE typname = 'foo';
> > -[ RECORD 1 ]---
> > typowner | 8713825
> > relowner | 16384
>
> Hmm.  I guess we're missing a recursion step somewhere.  I would have
> assumed that the pg_class entry would also have a dependency on the
> owner and should would have been visited in the initial loop.  Strange.

I think I see what's going on here: in AlterTypeOwner the code calls
ATExecChangeOwner when it detects a composite type to start the
modification starting from the type relation's, from where it cascades
onto the type itself.  However, AlterTypeOwnerInternal hasn't heard of
this trick and just changes itself and the array entry, so the pg_class
entry is not modified.

I think the bug is that shdepReassignOwned shouldn't be calling
AlterTypeOwnerInternal directly, because that routine is not of a high
enough level.  We need another routine sitting between AlterTypeOwner
and AlterTypeOwnerInternal which does the real work for the former
(including calling ATExecChangeOwner for composites), after looking up
the type OID from the name list; then shdepReassignOwned should call
that new intermediate function.

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

Alvaro Herrera wrote:

> I think the bug is that shdepReassignOwned shouldn't be calling
> AlterTypeOwnerInternal directly, because that routine is not of a high
> enough level.  We need another routine sitting between AlterTypeOwner
> and AlterTypeOwnerInternal which does the real work for the former
> (including calling ATExecChangeOwner for composites), after looking up
> the type OID from the name list; then shdepReassignOwned should call
> that new intermediate function.

I think that code is a bit confused.  In the attached patch I renamed
AlterTypeOwnerInternal to AlterTypeOwnerBare, and created a new
AlterTypeOwnerInternal which either calls ATExecChangeOwner (if
composite) of AlterTypeOwnerBare (if not); then, on ATExecChangeOwner we
can call *Bare if necessary, and on REASSIGN OWNED (shdepReassignOwned)
we can call AlterTypeOwnerInternal which now behaves correctly in the
case of a composite type.  There's some duplicate code that was
previously part of AlterTypeOwner directly; that's now calling
AlterTypeOwnerInternal instead.

There's an imcomplete comment above AlterTypeOwnerInternal related to a
function parameter that doesn't exist, introduced by 05f3f9c7b292; I
think this was supposed to control whether object access hooks are
called or not.  I wonder if we need to fix that -- this patch simply
removes the comment.  Cc'ing KaiGai and Robert about that.

(There's some inefficiency now that wasn't previously in that we open
pg_type repeatedly in ALTER TYPE OWNER, but I don't think we care about
that.)

This has been wrong all along; I wonder how come we hadn't gotten any
reports about this.  That said, the test coverage for REASSIGN OWNED
leaves something to be desired.  This patch adds a composite type to
that, but there's a lot more object types that would be worth covering.
(Note the supplied test case uses a "regrole" cast, so it's not
appropriate to apply that to 9.4 and earlier.)

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

Here's the final patch.

The commit msg says backpatched to 9.1, but given lack of backpatch of
another bugfix (commit 59367fdf9 for bug #9923) it doesn't actually
apply.  I'd rather backpatch that fix and then apply this one.

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

Alvaro Herrera wrote:

> The commit msg says backpatched to 9.1, but given lack of backpatch of
> another bugfix (commit 59367fdf9 for bug #9923) it doesn't actually
> apply.  I'd rather backpatch that fix and then apply this one.

So I applied it to 9.5 and master only.  If there are more votes for
back-patching both changes to earlier branches, I can do so.  I think we
should do that -- in essence, this bug lets you corrupt your catalogs by
dropping a user that still owns objects.

I guess REASSIGN OWNED is not the most popular of commands.

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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Alvaro Herrera wrote:
>> The commit msg says backpatched to 9.1, but given lack of backpatch of
>> another bugfix (commit 59367fdf9 for bug #9923) it doesn't actually
>> apply.  I'd rather backpatch that fix and then apply this one.

> So I applied it to 9.5 and master only.  If there are more votes for
> back-patching both changes to earlier branches, I can do so.  I think we
> should do that -- in essence, this bug lets you corrupt your catalogs by
> dropping a user that still owns objects.

I think Bruce did not backpatch 59367fdf9 for fear of
backwards-compatibility complaints, but I tend to agree that that decision
was mistaken.  The argument that you can be left with essentially corrupt
catalogs seems pretty strong.

            regards, tom lane

Alvaro Herrera wrote:
> Here's the final patch.
>
> The commit msg says backpatched to 9.1, but given lack of backpatch of
> another bugfix (commit 59367fdf9 for bug #9923) it doesn't actually
> apply.  I'd rather backpatch that fix and then apply this one.

Done that way.

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