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

The series of attached patches try to have refactoring on several ddl
statements; that consolidates distributed routines for each object
types into a common routine to minimize the number of upcoming hooks
for external security.
Please apply the series of patches in order of part-1, part-2, part-3
then part-4.

Part-1) DROP statement refactoring
It is a remaining portion of what I submitted in the last commit fest.
It allows object types that didn't used DropStmt in gram.y to go
through RemoveObjects(), instead of individual RemoveXXXX().

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.
In addition, it adds regression test cases for ALTER SET SCHEMA/RENAME TO.

Part-3) ALTER SET SCHEMA statement refactoring
This patch refactoring routines of ALTER SET SCHEMA implementations of
most object types; except for relations, types and extensions. This
portion was originally designed to use a common routine
(AlterObjectNamespace) via wrapper function that delivers
characteristic properties of object types, then this design was
replaced by facilities of objectaddress.c.

Part-4) ALTER RENAME TO statement refactoring
This patch refactoring routines of ALTER RENAME TO implementation of
most object types; except for databases, relations, columns, triggers
and roles.

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

On Tue, Nov 15, 2011 at 4:43 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> Part-1) DROP statement refactoring
> It is a remaining portion of what I submitted in the last commit fest.
> It allows object types that didn't used DropStmt in gram.y to go
> through RemoveObjects(), instead of individual RemoveXXXX().

Review of just this part:

- I think we can remove the special case for foreign data wrappers
because (1) the only case in which there's any behavioral difference
at all is if a superuser creates a foreign data wrapper (or the
ownership of one is reassigned to him) and he is then made not a
superuser; non-superusers can't create foreign data wrappers, and
existing foreign data wrappers can't be given to non-superusers;
moreover, (2) removing the special case causes the behavior to match
the documentation, which it currently doesn't (but only in the
aforementioned, extremely minor way).

- On the other hand, this patch blithely nukes the prohibition on
using DROP FUNCTION to remove an aggregate. I'm not sure that's a
good idea. It also eliminates the NOTICE when removing a built-in
function, which I think is OK because you don't actually get that far:

rhaas=# drop function int4pl(integer, integer);
ERROR: cannot drop function int4pl(integer,integer) because it is
required by the database system

- For some reason, we have code that causes procedural language names
to be downcased before use. Given that unquoted identifiers are
downcased anyway, this seems a bit redundant. I'm inclined to think
we don't need to preserve that behavior for DROP, especially because
other parts of the code - such as COMMENT - don't know about it
anyway. But rather than just changing it for DROP, I think we should
go through and rip out case_translate_language_name() across the
board, probably as a a separate commit.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> It also eliminates the NOTICE when removing a built-in
> function, which I think is OK because you don't actually get that far:

There are paths that can reach that notice --- I think what you have to
do is create a new function that references a built-in one. But why
we bother to warn for that isn't clear to me.

> - For some reason, we have code that causes procedural language names
> to be downcased before use.

I think this is a hangover from the fact that CREATE FUNCTION's LANGUAGE
clause used to insist on the language name being a string literal, and
of course the lexer didn't case-fold it then. That's been deprecated
for long enough that we probably don't need to have the extra case-fold
step anymore.

regards, tom lane

On Thu, Nov 17, 2011 at 1:00 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> It also eliminates the NOTICE when removing a built-in
>> function, which I think is OK because you don't actually get that far:
>
> There are paths that can reach that notice --- I think what you have to
> do is create a new function that references a built-in one.  But why
> we bother to warn for that isn't clear to me.
>
>> - For some reason, we have code that causes procedural language names
>> to be downcased before use.
>
> I think this is a hangover from the fact that CREATE FUNCTION's LANGUAGE
> clause used to insist on the language name being a string literal, and
> of course the lexer didn't case-fold it then.  That's been deprecated
> for long enough that we probably don't need to have the extra case-fold
> step anymore.

OK, great.

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

Excerpts from Robert Haas's message of jue nov 17 16:25:03 -0300 2011:
> On Thu, Nov 17, 2011 at 1:00 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:

> >> - For some reason, we have code that causes procedural language names
> >> to be downcased before use.
> >
> > I think this is a hangover from the fact that CREATE FUNCTION's LANGUAGE
> > clause used to insist on the language name being a string literal, and
> > of course the lexer didn't case-fold it then.  That's been deprecated
> > for long enough that we probably don't need to have the extra case-fold
> > step anymore.
>
> OK, great.

So the buildfarm broke due to this change, because citext does

--
-- Aggregates.
--

CREATE FUNCTION citext_smaller(citext, citext)
RETURNS citext
AS 'MODULE_PATHNAME'
LANGUAGE 'C' IMMUTABLE STRICT;

CREATE FUNCTION citext_larger(citext, citext)
RETURNS citext
AS 'MODULE_PATHNAME'
LANGUAGE 'C' IMMUTABLE STRICT;

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Thu, Nov 17, 2011 at 4:26 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
> So the buildfarm broke due to this change, because citext does

Thanks for fixing it. Should we revert the original change?

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Nov 17, 2011 at 4:26 PM, Alvaro Herrera
> <alvherre(at)commandprompt(dot)com> wrote:
>> So the buildfarm broke due to this change, because citext does

> Thanks for fixing it. Should we revert the original change?

I still think it's reasonable to remove the extra downcasing step,
but we'll have to document it as a change. For instance, spelling
C as either "C" or 'C' would work differently now. The fact that
the former is downcased seems quite surprising to me, so I don't
think anybody would say that this isn't a better definition, but
undoubtedly it could force people to change their source files.

regards, tom lane

On Thu, Nov 17, 2011 at 5:29 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Thu, Nov 17, 2011 at 4:26 PM, Alvaro Herrera
>> <alvherre(at)commandprompt(dot)com> wrote:
>>> So the buildfarm broke due to this change, because citext does
>
>> Thanks for fixing it.  Should we revert the original change?
>
> I still think it's reasonable to remove the extra downcasing step,
> but we'll have to document it as a change.  For instance, spelling
> C as either "C" or 'C' would work differently now.  The fact that
> the former is downcased seems quite surprising to me, so I don't
> think anybody would say that this isn't a better definition, but
> undoubtedly it could force people to change their source files.

So, should we add a note to all the LANGUAGE command pages in the
manual? Or just include this in the release notes?

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Nov 17, 2011 at 5:29 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I still think it's reasonable to remove the extra downcasing step,
>> but we'll have to document it as a change.

> So, should we add a note to all the LANGUAGE command pages in the
> manual? Or just include this in the release notes?

Release note seems sufficient. I looked at the text in CREATE FUNCTION
and it doesn't seem to need changing.

regards, tom lane

On Thu, Nov 17, 2011 at 11:21 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Nov 15, 2011 at 4:43 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> Part-1) DROP statement refactoring
>> It is a remaining portion of what I submitted in the last commit fest.
>> It allows object types that didn't used DropStmt in gram.y to go
>> through RemoveObjects(), instead of individual RemoveXXXX().
>
> Review of just this part:
>
> - I think we can remove the special case for foreign data wrappers
> because (1) the only case in which there's any behavioral difference
> at all is if a superuser creates a foreign data wrapper (or the
> ownership of one is reassigned to him) and he is then made not a
> superuser; non-superusers can't create foreign data wrappers, and
> existing foreign data wrappers can't be given to non-superusers;
> moreover, (2) removing the special case causes the behavior to match
> the documentation, which it currently doesn't (but only in the
> aforementioned, extremely minor way).
>
> - On the other hand, this patch blithely nukes the prohibition on
> using DROP FUNCTION to remove an aggregate.  I'm not sure that's a
> good idea.  It also eliminates the NOTICE when removing a built-in
> function, which I think is OK because you don't actually get that far:
>
> rhaas=# drop function int4pl(integer, integer);
> ERROR:  cannot drop function int4pl(integer,integer) because it is
> required by the database system
>
> - For some reason, we have code that causes procedural language names
> to be downcased before use.  Given that unquoted identifiers are
> downcased anyway, this seems a bit redundant.  I'm inclined to think
> we don't need to preserve that behavior for DROP, especially because
> other parts of the code - such as COMMENT - don't know about it
> anyway.  But rather than just changing it for DROP, I think we should
> go through and rip out case_translate_language_name() across the
> board, probably as a a separate commit.

I've committed part 1 of this patch series after correcting the above items.

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

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.
> In addition, it adds regression test cases for ALTER SET SCHEMA/RENAME TO.

This part adds a new regression test to a parallel group with the
following comments:

# NB: temp.sql does a reconnect which transiently uses 2 connections,
# so keep this parallel group to at most 19 tests

...and this would be test #20. So we either need to move it
elsewhere, or move something else elsewhere. I'm tempted to add it to
this rather scrawny-looking group, unless someone sees a reason to do
otherwise:

test: privileges security_label collate

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

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

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.

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.

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

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>

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

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>

On Sun, Nov 27, 2011 at 3:14 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> 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?

Yeah, let's try that for starters, and then see if anything further
suggests itself.

> At least, AlterObjectNamespace already consolidate the point to check
> permissions.

Right.

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

2011/11/30 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Sun, Nov 27, 2011 at 3:14 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> 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?
>
> Yeah, let's try that for starters, and then see if anything further
> suggests itself.
>
OK. I marked this patch as "Returned with Feedback".
I will try it again.
In the timeframe of this commit-fest, I'll focuse on remaining my
patches and reviewing pgsql-fdw.

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