Re: Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement

On Sat, Nov 19, 2011 at 1:49 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> But I'm not sure why we do.  My thought here was that we should
>> extended the ObjectProperty array in objectaddress.c so that
>> AlterObjectNamespace can get by with fewer arguments - specifically,
>> it seems like the following ought to be things that can be looked up
>> via the ObjectProperty mechanism:
>>
>> int oidCacheId, int nameCacheId, int Anum_name, int Anum_namespace,
>> int Anum_owner, AclObjectKind acl_kind
>>
> Thanks for your reviewing, and sorry for not a timely response.
>
> I tried to add these items into ObjectProperty and replace existing caller of
> AlterObjectNamespace, however, it seemed to me these members (especially
> AclObjectKind) were too specific with current implementation of the
> AlterObjectNamespace.

Hmm, maybe so. But then we could still move over some things.
oidCacheId is pretty much already there already, isn't it?

> And, I also modified check_ownership() to eliminate objtype/object/objarg; that
> allows to invoke this function from code paths without these
> information, such as
> shdepReassignOwned() or AlterObjectNamespace_oid().

Yeah. I'm not sure I like that. It doesn't seem like a particularly
good idea to throw away the information we have about the name the
user entered and assume we'll be able to regenerate it from the system
catalogs after the fact.

>> Now, what you have here is a much broader reworking.  And that's not
>> necessarily bad, but at the moment I'm not really seeing how it
>> benefits us.
>>
> In my point, if individual object types need to have its own handler for
> alter commands, points of the code to check permissions are also
> distributed for each object types. It shall be a burden to maintain hooks
> that allows modules (e.g sepgsql) to have permission checks.

Well, it's always nicer if you can just put a call to some hook in one
place instead of many. But it's not worth sacrificing everything to
make that happen. I think we need to compare the value of only
needing a hook in one place against the disruption of changing a lot
of code that is working fine as it is. In the case of the DROP
commands, it seems to me that the refactoring you did came out a huge
win, but this doesn't seem as clear to me. Note that DROP actually
does dispatch the actual work of dropping the object to a
type-specific function, unlike what you're trying to do here.

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