Thread: BUG #13666: REASSIGN OWNED BY doesn't affect the relation underlying composite type
BUG #13666: REASSIGN OWNED BY doesn't affect the relation underlying composite type
From
cpacejo@clearskydata.com
Date:
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.
Re: BUG #13666: REASSIGN OWNED BY doesn't affect the relation underlying composite type
From
Alvaro Herrera
Date:
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
Re: BUG #13666: REASSIGN OWNED BY doesn't affect the relation underlying composite type
From
Alvaro Herrera
Date:
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
Re: BUG #13666: REASSIGN OWNED BY doesn't affect the relation underlying composite type
From
Alvaro Herrera
Date:
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
Re: BUG #13666: REASSIGN OWNED BY doesn't affect the relation underlying composite type
From
Alvaro Herrera
Date:
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
Attachment
Re: BUG #13666: REASSIGN OWNED BY doesn't affect the relation underlying composite type
From
Alvaro Herrera
Date:
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
Attachment
Re: BUG #13666: REASSIGN OWNED BY doesn't affect the relation underlying composite type
From
Alvaro Herrera
Date:
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
Re: BUG #13666: REASSIGN OWNED BY doesn't affect the relation underlying composite type
From
Tom Lane
Date:
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
Re: BUG #13666: REASSIGN OWNED BY doesn't affect the relation underlying composite type
From
Alvaro Herrera
Date:
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