Thread: DROP OWNED again
People, Here is the patch for DROP OWNED (finally!). This patch implements two new commands, DROP OWNED and REASSIGN OWNED BY. DROP OWNED drops the objects owned by any of a list of roles (in the current database, of course). It also revokes all privileges that have been granted to any of them. One must have privileges of all the mentioned roles in order to be able to do this. (So a superuser can do it for any role, and in the simple case a role can do it only to itself.) REASSIGN OWNED gives the objects away to some other role. It doesn't touch grants. So if a role has been granted something and you want to drop it but keep the objects, you must do REASSIGN OWNED and then DROP OWNED. One must have all privileges of all the mentioned roles in order to do this, including the receiving role. The idea of all this is that if you want to drop a role, you first issue a DROP ROLE, note all the databases on which it says it has dependences, connect to each and issue REASSIGN OWNED and/or DROP OWNED as appropiate, and finally issue DROP ROLE again. This eases dropping a role (or that is the theory anyway). The patch is missing regression tests. I will include them when I apply it. I intend to apply it tomorrow or so, unless somebody has (strong? Is the time when people "strongly objected" to things gone?) objections to it. -- Alvaro Herrera http://www.PlanetPostgreSQL.org "La rebeldía es la virtud original del hombre" (Arthur Schopenhauer)
Attachment
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> /*
> ! * Called to execute the utility commands GRANT and REVOKE.
> ! *
> ! * stmt may be a complete GrantStmt created by the parser, or it may be
> ! * missing the "objects" list and the "grantees" list. In this case,
> ! * they are taken from the second and third parameters, respectively.
> */
> void
> ! ExecuteGrantStmt(GrantStmt *stmt, Oid object, Oid grantee)
This seems like a really ugly API. What's so hard about expecting the
caller to construct a valid GrantStmt?
(I get the impression from a quick scan of the code that the comment
is a long way from telling the truth about what's really happening,
either.)
> + static void AlterConversionOwner_int(Relation rel, Oid conversionOid,
> + Oid newOwnerId);
If these are supposed to mean "AlterConversionOwner_internal", please
spell them that way. Sitting beside "AlterConversionOwner_oid", it
sure looks like the "int" is meant to be read as "integer".
regards, tom lane
Tom Lane wrote: > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > /* > > ! * Called to execute the utility commands GRANT and REVOKE. > > ! * > > ! * stmt may be a complete GrantStmt created by the parser, or it may be > > ! * missing the "objects" list and the "grantees" list. In this case, > > ! * they are taken from the second and third parameters, respectively. > > */ > > void > > ! ExecuteGrantStmt(GrantStmt *stmt, Oid object, Oid grantee) > > This seems like a really ugly API. What's so hard about expecting the > caller to construct a valid GrantStmt? Well, a valid GrantStmt wants to have textual names of objects, so I figured it would be best to offer both alternatives to the caller, while at the same time not having to look up some things twice: there are several syscaches for both names and Oids, so if you can get the object's tuple using the name syscache, why not use it directly? The attached does not attempt to save the second lookup, but it seems better code -- it offers two entry points, one with GrantStmt just as it's not, and another one with only Oids. The former calls the latter upon looking up the object's Oids. So it's somewhat more inefficient, though probably negligibly so. > (I get the impression from a quick scan of the code that the comment > is a long way from telling the truth about what's really happening, > either.) Hmm, not sure why you would think so, but it doesn't matter now. > > + static void AlterConversionOwner_int(Relation rel, Oid conversionOid, > > + Oid newOwnerId); > > If these are supposed to mean "AlterConversionOwner_internal", please > spell them that way. Sitting beside "AlterConversionOwner_oid", it > sure looks like the "int" is meant to be read as "integer". Sure -- done. I have added the regression tests in this new version, which I'll apply on monday barring any objections. (I do not attach the SGML files because these didn't change since the last post.) -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Attachment
Alvaro Herrera wrote: > I have added the regression tests in this new version, which I'll apply > on monday barring any objections. (I do not attach the SGML files > because these didn't change since the last post.) Applied. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
I intend to apply later today the attached patch in order to reduce some code duplication in aclchk.c and clean a bit the API I just introduced in the previous patch. This reduces aclchk.c from 2377 lines to 2206. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Attachment
Alvaro Herrera wrote:
> I intend to apply later today the attached patch in order to reduce some
> code duplication in aclchk.c and clean a bit the API I just introduced
> in the previous patch. This reduces aclchk.c from 2377 lines to 2206.
Of course, it would be much better if the proposed patch actually
worked, which it doesn't because there's a function call that I didn't
generalize: the code is calling pg_class_aclmask(), which of course only
works for relations.
Now I noticed that there are multiple functions pg_class_aclmask,
pg_database_aclmask, pg_language_aclmask, etc. Is there any objection
to making the exported routine expose the object type as an AclKind
parameter instead of having one function for each object type?
So instead of having
extern AclMode pg_class_aclmask(Oid table_oid, Oid roleid,
AclMode mask, AclMaskHow how);
extern AclMode pg_database_aclmask(Oid db_oid, Oid roleid,
AclMode mask, AclMaskHow how);
extern AclMode pg_proc_aclmask(Oid proc_oid, Oid roleid,
AclMode mask, AclMaskHow how);
extern AclMode pg_language_aclmask(Oid lang_oid, Oid roleid,
AclMode mask, AclMaskHow how);
extern AclMode pg_namespace_aclmask(Oid nsp_oid, Oid roleid,
AclMode mask, AclMaskHow how);
extern AclMode pg_tablespace_aclmask(Oid spc_oid, Oid roleid,
AclMode mask, AclMaskHow how);
We would have
extern AclMode pg_aclmask(AclKind objkind, Oid obj_oid, Oid roleid,
AclMode mask, AclMaskHow how);
And this would call the appropiate static function.
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Now I noticed that there are multiple functions pg_class_aclmask,
> pg_database_aclmask, pg_language_aclmask, etc. Is there any objection
> to making the exported routine expose the object type as an AclKind
> parameter instead of having one function for each object type?
How about "in addition to" instead of "instead"? I see no reason to
impose extra notation and a level of indirection on the places that know
perfectly well which object type they are dealing with.
regards, tom lane
Alvaro Herrera wrote: > I intend to apply later today the attached patch in order to reduce some > code duplication in aclchk.c and clean a bit the API I just introduced > in the previous patch. This reduces aclchk.c from 2377 lines to 2206. I applied this patch yesterday, but I did not receive the commit message. However I do see it in the online archives. Is there something fishy going on here, or just the mail was lost on its way here? -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support