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

2011/11/18 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Tue, Nov 15, 2011 at 4:43 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> Part-2) Groundworks on objectaddress.c
>> This patch adds necessary groundworks for Part-3 and Part-4.
>> It adds ObjectPropertyType of objectaddress.c index-oid and cache-id
>> for name lookup and attribute number of object name; these field is
>> used to detect namespace conflict on object_exists_namespace() that
>> shall be called on refactored ALTER SET SCHEMA/RENAME TO.
>> It also reduce some arguments of check_object_ownership(), and allows
>> to call this function without knowledge of ObjectType and text
>> representation. It allows to check ownership using this function from
>> the code path of AlterObjectNamespace_oid() that does not have (or
>> need to look up catalog to obtain) ObjectType information.
>
> I spent some time wading through the code for parts 2 through 4, and I
> guess I'm not sure this is headed in the right direction.  If we need
> this much infrastructure in order to consolidate the handling of ALTER
> BLAH .. SET SCHEMA, then maybe it's not worth doing.
>
> 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.

I think, SET SCHEMA, RENAME TO and OWNER TO are candidate to
consolidate similar codes using facilities in objectaddress.c.
Even though SET SCHEMA is mostly consolidated with AlterObjectNamespace,
RENAME TO and OWNER TO are implemented individual routines for each
object types. So, I thought it is preferable way to provide groundwork to be
applied these routines also.

In the part-2 patch, I added object_exists_namespace() to check namespace
conflicts commonly used to SET SCHEMA and RENAME TO, although we
have individual routines for RENAME TO.
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().

> The advantage of that, or so it seems to me, is that all this
> information is in a table in objectaddress.c where multiple clients
> can get at it at need, as opposed to the current system where it's
> passed as arguments to AlterObjectNamespace(), and all that would need
> to be duplicated if we had another function that did something
> similar.
>
Yes, correct.

> 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.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>