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

From: Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>, PgHacker <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement
Date: 2011-11-27 20:14:10
Message-ID: CADyhKSUkvqZ6aX6zgKxDyypfMU9VxF+xzcr5tcjV_qG_67Eizg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

2011/11/21 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>> 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.
>
Yes, I agree with your opinion.
I'm also not sure whether my refactoring efforts on ALTER commands
will give us larger worth than its size of code changes, although we will
be able to consolidate the point of hooks around them.

If we add new properties that required by AlterObjectNamespace, as
you suggested, it will allow to reduce number of caller of this routine
mechanically with small changes.
Should I try this reworking with this way?
At least, AlterObjectNamespace already consolidate the point to check
permissions.

I initially thought it is too specific for AlterObjectNamespace, then I
reconsidered that we may be able to apply same properties to
ALTER RENAME TO/SET OWNER commands also, even though
these commands have completely branched routines for each object
types.

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Dimitri Fontaine 2011-11-27 20:24:39 Re: Prep object creation hooks, and related sepgsql updates
Previous Message David E. Wheeler 2011-11-27 20:12:41 Re: Patch: Perl xsubpp