ALTER command reworks

The attached patch is a refreshed version of ALTER command
reworks towards the latest tree. Here is few big changes except
for code integration of the code to rename event triggers.

BTW, I had to adjust between oid of pg_largeobject_metadata
and pg_largeobject on three points of this patch, like:

if (classId == LargeObjectRelationId)
classId = LargeObjectMetadataRelationId;

When we supported largeobject permission features, we put
special handling to track dependency of its ownership.
The pg_depend records oid of pg_largeobject, instead of
pg_largeobject_metadata. Thus, we cannot use classId of
ObjectAddress being returned from get_object_address()
as an argument of heap_open() as is, if it indicates oid of
pg_largeobject.

Was it a right decision to track dependency of large object using
oid of pg_largeobject, instead of pg_largeobject_metadata?
IIRC, the reason why we used oid of pg_largeobject is backward
compatibility for applications that tries to reference pg_depend
with built-in oids.

Don't we need to change the specification?
Or, do we continue to implement the code carefully?

Thanks,

2012/7/24 Kohei KaiGai <kaigai@kaigai.gr.jp>:

The attached patch is my homework at the last commit fest of v9.2.

It consolidates some similar routines into a single generic routines
that handles ALTER RENAME TO, OWNER TO and SET SCHEMA
according to the ObjectProperty in objectaddress.c, but here is
no functionality enhancement.

As existing AlterObjectNamespace() doing, this generic routine
handles only case when a catalog entry is updated without any
extra permission checks or other stuffs.
Thus, RenameSchema was not consolidated due to CREATE
permission checks towards the database, for example.

As patched chunk in commands/alter.c shows, 13 of special
routines were consolidated into AlterObjectRename, 16 of
special ones were consolidated into AlterObjectNamespace,
and 20 of special ones were consolidated into AlterObjectOwner().

My motivation is for minimization of number of places to put
object-access-hook around alter statement. Probably, it shall
be located around or just after simple_heap_update().
This is a groundwork towards ALTER command support in
sepgsql module.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

--
KaiGai Kohei <kaigai@kaigai.gr.jp>

On Mon, Aug 13, 2012 at 3:35 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

The attached patch is a refreshed version of ALTER command
reworks towards the latest tree. Here is few big changes except
for code integration of the code to rename event triggers.

This seems to have bit-rotted a bit. Please rebase.

BTW, I had to adjust between oid of pg_largeobject_metadata
and pg_largeobject on three points of this patch, like:

if (classId == LargeObjectRelationId)
classId = LargeObjectMetadataRelationId;

When we supported largeobject permission features, we put
special handling to track dependency of its ownership.
The pg_depend records oid of pg_largeobject, instead of
pg_largeobject_metadata. Thus, we cannot use classId of
ObjectAddress being returned from get_object_address()
as an argument of heap_open() as is, if it indicates oid of
pg_largeobject.

Was it a right decision to track dependency of large object using
oid of pg_largeobject, instead of pg_largeobject_metadata?
IIRC, the reason why we used oid of pg_largeobject is backward
compatibility for applications that tries to reference pg_depend
with built-in oids.

I think it was a terrible decision and I'm pretty sure I said as much
at the time, but I lost the argument, so I'm inclined to think we're
stuck with continuing to support that kludge.

Some other preliminary comments:

- Surely you need to take AccessExclusiveLock on the object being
renamed, not just ShareUpdateExclusiveLock.
- I don't think it's acceptable to assemble the object-type
"object-name" already exists message using getObjectDescription();
it's not good for translators. Use an array of messages, one per
object-type, as we have done in other cases.
- I would like to handle either the RENAME case or the ALTER OWNER
case in one patch, and the other in a follow-on patch. Can you pick
one to do first and remove everything related to the other one?

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

2012/8/30 Robert Haas <robertmhaas@gmail.com>:

On Mon, Aug 13, 2012 at 3:35 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

The attached patch is a refreshed version of ALTER command
reworks towards the latest tree. Here is few big changes except
for code integration of the code to rename event triggers.

This seems to have bit-rotted a bit. Please rebase.

BTW, I had to adjust between oid of pg_largeobject_metadata
and pg_largeobject on three points of this patch, like:

if (classId == LargeObjectRelationId)
classId = LargeObjectMetadataRelationId;

When we supported largeobject permission features, we put
special handling to track dependency of its ownership.
The pg_depend records oid of pg_largeobject, instead of
pg_largeobject_metadata. Thus, we cannot use classId of
ObjectAddress being returned from get_object_address()
as an argument of heap_open() as is, if it indicates oid of
pg_largeobject.

Was it a right decision to track dependency of large object using
oid of pg_largeobject, instead of pg_largeobject_metadata?
IIRC, the reason why we used oid of pg_largeobject is backward
compatibility for applications that tries to reference pg_depend
with built-in oids.

I think it was a terrible decision and I'm pretty sure I said as much
at the time, but I lost the argument, so I'm inclined to think we're
stuck with continuing to support that kludge.

OK, we will keep to implement carefully...

Some other preliminary comments:

- Surely you need to take AccessExclusiveLock on the object being
renamed, not just ShareUpdateExclusiveLock.
- I don't think it's acceptable to assemble the object-type
"object-name" already exists message using getObjectDescription();
it's not good for translators. Use an array of messages, one per
object-type, as we have done in other cases.
- I would like to handle either the RENAME case or the ALTER OWNER
case in one patch, and the other in a follow-on patch. Can you pick
one to do first and remove everything related to the other one?

OK, I'll split the patch into three (isn't it?) portions; RENAME, SET OWNER
and SET SCHEMA, with all above your suggestions.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

2012/8/31 Kohei KaiGai <kaigai@kaigai.gr.jp>:

2012/8/30 Robert Haas <robertmhaas@gmail.com>:

On Mon, Aug 13, 2012 at 3:35 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

The attached patch is a refreshed version of ALTER command
reworks towards the latest tree. Here is few big changes except
for code integration of the code to rename event triggers.

This seems to have bit-rotted a bit. Please rebase.

BTW, I had to adjust between oid of pg_largeobject_metadata
and pg_largeobject on three points of this patch, like:

if (classId == LargeObjectRelationId)
classId = LargeObjectMetadataRelationId;

When we supported largeobject permission features, we put
special handling to track dependency of its ownership.
The pg_depend records oid of pg_largeobject, instead of
pg_largeobject_metadata. Thus, we cannot use classId of
ObjectAddress being returned from get_object_address()
as an argument of heap_open() as is, if it indicates oid of
pg_largeobject.

Was it a right decision to track dependency of large object using
oid of pg_largeobject, instead of pg_largeobject_metadata?
IIRC, the reason why we used oid of pg_largeobject is backward
compatibility for applications that tries to reference pg_depend
with built-in oids.

I think it was a terrible decision and I'm pretty sure I said as much
at the time, but I lost the argument, so I'm inclined to think we're
stuck with continuing to support that kludge.

OK, we will keep to implement carefully...

Some other preliminary comments:

- Surely you need to take AccessExclusiveLock on the object being
renamed, not just ShareUpdateExclusiveLock.
- I don't think it's acceptable to assemble the object-type
"object-name" already exists message using getObjectDescription();
it's not good for translators. Use an array of messages, one per
object-type, as we have done in other cases.
- I would like to handle either the RENAME case or the ALTER OWNER
case in one patch, and the other in a follow-on patch. Can you pick
one to do first and remove everything related to the other one?

OK, I'll split the patch into three (isn't it?) portions; RENAME, SET OWNER
and SET SCHEMA, with all above your suggestions.

As attached, I split off the original one into three portions; for set-schema,
set-owner and rename-to. Please apply them in order of patch filename.
Regarding to the error message, RenameErrorMsgAlreadyExists() was added
to handle per object type messages in case when aclcheck_error() is not
available to utilize.
All the regression test is contained with the 1st patch to make sure the
series of reworks does not change existing behaviors.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

Excerpts from Kohei KaiGai's message of lun sep 10 08:08:32 -0300 2012:

As attached, I split off the original one into three portions; for set-schema,
set-owner and rename-to. Please apply them in order of patch filename.
Regarding to the error message, RenameErrorMsgAlreadyExists() was added
to handle per object type messages in case when aclcheck_error() is not
available to utilize.
All the regression test is contained with the 1st patch to make sure the
series of reworks does not change existing behaviors.

I checked this and noticed that the regression test has a couple of
failures -- see below. I think we should remove the test for the first
two hunks, and fix the query for the final one to remove the offending
column.

The good news is that I ran this without applying the whole patch, only
the new regression test' files. I didn't check the files in detail.

I have skimmed over the patches and they seem reasonable too; I will
look at them in more detail later. I think we should go ahead and apply
the (tweaked) regression test alone, as a first step.

*** /pgsql/source/HEAD/src/test/regress/expected/alter_generic.out	2012-09-27 17:23:05.000000000 -0300
--- /home/alvherre/Code/pgsql/build/HEAD/src/test/regress/results/alter_generic.out	2012-09-27 17:29:21.000000000 -0300
***************
*** 106,112 ****
  CREATE COLLATION alt_coll1 (locale = 'C');
  CREATE COLLATION alt_coll2 (locale = 'C');
  ALTER COLLATION alt_coll1 RENAME TO alt_coll2;  -- failed (name conflict)
! ERROR:  collation "alt_coll2" for encoding "SQL_ASCII" already exists in schema "alt_nsp1"
  ALTER COLLATION alt_coll1 RENAME TO alt_coll3;  -- OK
  ALTER COLLATION alt_coll2 OWNER TO regtest_alter_user2;  -- failed (no role membership)
  ERROR:  must be member of role "regtest_alter_user2"
--- 106,112 ----
  CREATE COLLATION alt_coll1 (locale = 'C');
  CREATE COLLATION alt_coll2 (locale = 'C');
  ALTER COLLATION alt_coll1 RENAME TO alt_coll2;  -- failed (name conflict)
! ERROR:  collation "alt_coll2" for encoding "UTF8" already exists in schema "alt_nsp1"
  ALTER COLLATION alt_coll1 RENAME TO alt_coll3;  -- OK
  ALTER COLLATION alt_coll2 OWNER TO regtest_alter_user2;  -- failed (no role membership)
  ERROR:  must be member of role "regtest_alter_user2"
***************
*** 125,131 ****
  ALTER COLLATION alt_coll3 SET SCHEMA alt_nsp2;  -- failed (not owner)
  ERROR:  must be owner of collation alt_coll3
  ALTER COLLATION alt_coll2 SET SCHEMA alt_nsp2;  -- failed (name conflict)
! ERROR:  collation "alt_coll2" for encoding "SQL_ASCII" already exists in schema "alt_nsp2"
  RESET SESSION AUTHORIZATION;
  SELECT n.nspname, c.collname, a.rolname
    FROM pg_collation c, pg_namespace n, pg_authid a
--- 125,131 ----
  ALTER COLLATION alt_coll3 SET SCHEMA alt_nsp2;  -- failed (not owner)
  ERROR:  must be owner of collation alt_coll3
  ALTER COLLATION alt_coll2 SET SCHEMA alt_nsp2;  -- failed (name conflict)
! ERROR:  collation "alt_coll2" for encoding "UTF8" already exists in schema "alt_nsp2"
  RESET SESSION AUTHORIZATION;
  SELECT n.nspname, c.collname, a.rolname
    FROM pg_collation c, pg_namespace n, pg_authid a
***************
*** 334,341 ****
    ORDER BY nspname, opfname;
   nspname  | opfname  | amname |       rolname       
  ----------+----------+--------+---------------------
!  alt_nsp1 | alt_opc1 | hash   | kaigai
!  alt_nsp1 | alt_opc2 | hash   | kaigai
   alt_nsp1 | alt_opf2 | hash   | regtest_alter_user2
   alt_nsp1 | alt_opf3 | hash   | regtest_alter_user1
   alt_nsp1 | alt_opf4 | hash   | regtest_alter_user2
--- 334,341 ----
    ORDER BY nspname, opfname;
   nspname  | opfname  | amname |       rolname       
  ----------+----------+--------+---------------------
!  alt_nsp1 | alt_opc1 | hash   | alvherre
!  alt_nsp1 | alt_opc2 | hash   | alvherre
   alt_nsp1 | alt_opf2 | hash   | regtest_alter_user2
   alt_nsp1 | alt_opf3 | hash   | regtest_alter_user1
   alt_nsp1 | alt_opf4 | hash   | regtest_alter_user2

======================================================================

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Excerpts from Kohei KaiGai's message of lun sep 10 08:08:32 -0300 2012:

As attached, I split off the original one into three portions; for set-schema,
set-owner and rename-to. Please apply them in order of patch filename.

Hmm, in the first patch, it seems to me we can simplify
AlterObjectNamespace's signature: instead of passing all the details of
the object class (cache Ids and attribute numbers and so on), just do

AlterObjectNamespace(catalog-containing-object, objectId, newNamespaceOid)

AlterObjectNamespace then looks up the catcache_oid and so on
internally. The only difference from what's happening in the submitted
patch is that in the AlterCollationNamespace case, AlterObjectNamespace
would have to look them up instead of getting them directly from the
caller as the patch currently has it.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Excerpts from Alvaro Herrera's message of vie sep 28 18:17:32 -0300 2012:

Excerpts from Kohei KaiGai's message of lun sep 10 08:08:32 -0300 2012:

As attached, I split off the original one into three portions; for set-schema,
set-owner and rename-to. Please apply them in order of patch filename.

Hmm, in the first patch, it seems to me we can simplify
AlterObjectNamespace's signature: instead of passing all the details of
the object class (cache Ids and attribute numbers and so on), just do

AlterObjectNamespace(catalog-containing-object, objectId, newNamespaceOid)

Like in the attached reworked version, in which I renamed the function
to AlterObjectNamespace_internal because the other name seemed a bit
wrong in the fact of the existing AlterObjectNamespace_oid.

I also made the ALTER FUNCTION case go through
AlterObjectNamespace_internal; it seems pointless to have a separate
code path to go through when the generic one does just fine (also, this
makes functions identical to collations in implementation). That's one
less hook point for sepgsql, I guess.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Excerpts from Kohei KaiGai's message of lun sep 10 08:08:32 -0300 2012:

As attached, I split off the original one into three portions; for set-schema,
set-owner and rename-to. Please apply them in order of patch filename.
Regarding to the error message, RenameErrorMsgAlreadyExists() was added
to handle per object type messages in case when aclcheck_error() is not
available to utilize.

I have pushed the regression tests and parts 1 and 2. Only part 3 is
missing from this series, but I'm not as sure about that one as the
other two. Not really a fan of RenameErrorMsgAlreadyExists() as coded,
but I'll think more about it.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Excerpts from Alvaro Herrera's message of mié oct 03 18:25:54 -0300 2012:

Excerpts from Kohei KaiGai's message of lun sep 10 08:08:32 -0300 2012:

As attached, I split off the original one into three portions; for set-schema,
set-owner and rename-to. Please apply them in order of patch filename.
Regarding to the error message, RenameErrorMsgAlreadyExists() was added
to handle per object type messages in case when aclcheck_error() is not
available to utilize.

I have pushed the regression tests and parts 1 and 2. Only part 3 is
missing from this series, but I'm not as sure about that one as the
other two. Not really a fan of RenameErrorMsgAlreadyExists() as coded,
but I'll think more about it.

I forgot to mention: I think with a little more effort (a callback to be
registered as the permission check to run during SET OWNER, maybe?) we
could move the foreign stuff and event triggers into the generic SET
OWNER implementation.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Kohei KaiGai escribió:

2012/8/31 Kohei KaiGai <kaigai@kaigai.gr.jp>:

2012/8/30 Robert Haas <robertmhaas@gmail.com>:

On Mon, Aug 13, 2012 at 3:35 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

Was it a right decision to track dependency of large object using
oid of pg_largeobject, instead of pg_largeobject_metadata?
IIRC, the reason why we used oid of pg_largeobject is backward
compatibility for applications that tries to reference pg_depend
with built-in oids.

I think it was a terrible decision and I'm pretty sure I said as much
at the time, but I lost the argument, so I'm inclined to think we're
stuck with continuing to support that kludge.

OK, we will keep to implement carefully...

After reviewing this patch, I think we need to revisit this decision.

OK, I'll split the patch into three (isn't it?) portions; RENAME, SET OWNER
and SET SCHEMA, with all above your suggestions.

As attached, I split off the original one into three portions; for set-schema,
set-owner and rename-to. Please apply them in order of patch filename.
Regarding to the error message, RenameErrorMsgAlreadyExists() was added
to handle per object type messages in case when aclcheck_error() is not
available to utilize.

Here's the remaining piece; the RENAME part. I have reworked it a bit,
but it needs a bit more work yet. Note this:

alvherre=# alter function foo.g(int, int) rename to g;
ERROR: function foo.g(integer,integer) already exists in schema "foo"

The previous code would have said

alvherre=# alter function foo.g(int, int) rename to g;
ERROR: function g(integer, integer) already exists in schema "foo"

Note that the function name is not qualified here. The difference is
that the original code was calling funcname_signature_string() to format
the function name, and the new code is calling format_procedure(). This
seems wrong to me; please rework.

I haven't checked other object renames, but I think they should be okay
because functions are the only objects for which we pass the OID instead
of the name to the error message function.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

2012/10/2 Alvaro Herrera <alvherre@2ndquadrant.com>:

Excerpts from Alvaro Herrera's message of vie sep 28 18:17:32 -0300 2012:

Excerpts from Kohei KaiGai's message of lun sep 10 08:08:32 -0300 2012:

As attached, I split off the original one into three portions; for set-schema,
set-owner and rename-to. Please apply them in order of patch filename.

Hmm, in the first patch, it seems to me we can simplify
AlterObjectNamespace's signature: instead of passing all the details of
the object class (cache Ids and attribute numbers and so on), just do

AlterObjectNamespace(catalog-containing-object, objectId, newNamespaceOid)

Like in the attached reworked version, in which I renamed the function
to AlterObjectNamespace_internal because the other name seemed a bit
wrong in the fact of the existing AlterObjectNamespace_oid.

I also made the ALTER FUNCTION case go through
AlterObjectNamespace_internal; it seems pointless to have a separate
code path to go through when the generic one does just fine (also, this
makes functions identical to collations in implementation). That's one
less hook point for sepgsql, I guess.

Thanks for your reviewing, and sorry for my late response.

I definitely agree with your solution. The reason why my original patch
had separate code path for function and collation was they took
additional elements (such as argument-list of function) to check
duplicate names. So, I think it is a wise idea to invoke the common
code after name duplication checks.

Best regards,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

2012/10/5 Alvaro Herrera <alvherre@2ndquadrant.com>:

Kohei KaiGai escribió:

2012/8/31 Kohei KaiGai <kaigai@kaigai.gr.jp>:

2012/8/30 Robert Haas <robertmhaas@gmail.com>:

On Mon, Aug 13, 2012 at 3:35 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

Was it a right decision to track dependency of large object using
oid of pg_largeobject, instead of pg_largeobject_metadata?
IIRC, the reason why we used oid of pg_largeobject is backward
compatibility for applications that tries to reference pg_depend
with built-in oids.

I think it was a terrible decision and I'm pretty sure I said as much
at the time, but I lost the argument, so I'm inclined to think we're
stuck with continuing to support that kludge.

OK, we will keep to implement carefully...

After reviewing this patch, I think we need to revisit this decision.

OK, I'll split the patch into three (isn't it?) portions; RENAME, SET OWNER
and SET SCHEMA, with all above your suggestions.

As attached, I split off the original one into three portions; for set-schema,
set-owner and rename-to. Please apply them in order of patch filename.
Regarding to the error message, RenameErrorMsgAlreadyExists() was added
to handle per object type messages in case when aclcheck_error() is not
available to utilize.

Here's the remaining piece; the RENAME part. I have reworked it a bit,
but it needs a bit more work yet. Note this:

alvherre=# alter function foo.g(int, int) rename to g;
ERROR: function foo.g(integer,integer) already exists in schema "foo"

The previous code would have said

alvherre=# alter function foo.g(int, int) rename to g;
ERROR: function g(integer, integer) already exists in schema "foo"

Note that the function name is not qualified here. The difference is
that the original code was calling funcname_signature_string() to format
the function name, and the new code is calling format_procedure(). This
seems wrong to me; please rework.

I haven't checked other object renames, but I think they should be okay
because functions are the only objects for which we pass the OID instead
of the name to the error message function.

The attached patch fixes the messaging issue.
I newly add func_signature_string_oid() that returns compatible function's
signature, but takes its object-id.

So, the error message is now constructed as:
+       case OBJECT_AGGREGATE:
+       case OBJECT_FUNCTION:
+           errorm = format_elog_string("function %s already exists in
schema \"%s\"",
+                                       func_signature_string_oid(objectId),
+                                       get_namespace_name(namespaceId));
+            break;

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

Kohei KaiGai escribió:

2012/10/5 Alvaro Herrera <alvherre@2ndquadrant.com>:

The attached patch fixes the messaging issue.
I newly add func_signature_string_oid() that returns compatible function's
signature, but takes its object-id.

So, the error message is now constructed as:
+       case OBJECT_AGGREGATE:
+       case OBJECT_FUNCTION:
+           errorm = format_elog_string("function %s already exists in
schema \"%s\"",
+                                       func_signature_string_oid(objectId),
+                                       get_namespace_name(namespaceId));
+            break;

Thanks, yeah, this works for me.

I am now wondering if it would make sense to merge the duplicate-name
error cases in AlterObjectNamespace_internal and
AlterObjectRename_internal. The former only works when there is a name
catcache for the object type. Maybe we can create a single function to
which we give the object type, name/args, oid, etc, and it uses a
catcache if available and falls back to get_object_address (with the
IMO ugly name list manipulations) if not.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

The attached patch is the revised version of ALTER RENAME TO
consolidation. According to the previous suggestion, it uses
a common logic to check object-naming duplication at
check_duplicate_objectname().

At the code path on AlterObjectNamespace_internal, it lost
ObjectType information to the supplied object, so I also added
get_object_type() function at objectaddress.c for inverse
translation from a couple of classId/objectId to OBJECT_* label.

Rest of parts are unchanged since the previous versions.

Thanks,

2012/10/17 Alvaro Herrera <alvherre@2ndquadrant.com>:

Kohei KaiGai escribió:

2012/10/5 Alvaro Herrera <alvherre@2ndquadrant.com>:

The attached patch fixes the messaging issue.
I newly add func_signature_string_oid() that returns compatible function's
signature, but takes its object-id.

So, the error message is now constructed as:
+       case OBJECT_AGGREGATE:
+       case OBJECT_FUNCTION:
+           errorm = format_elog_string("function %s already exists in
schema \"%s\"",
+                                       func_signature_string_oid(objectId),
+                                       get_namespace_name(namespaceId));
+            break;

Thanks, yeah, this works for me.

I am now wondering if it would make sense to merge the duplicate-name
error cases in AlterObjectNamespace_internal and
AlterObjectRename_internal. The former only works when there is a name
catcache for the object type. Maybe we can create a single function to
which we give the object type, name/args, oid, etc, and it uses a
catcache if available and falls back to get_object_address (with the
IMO ugly name list manipulations) if not.

--
Įlvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
KaiGai Kohei <kaigai@kaigai.gr.jp>

Hi,

Thanks for working on that cleanup job!

Kohei KaiGai <kaigai@kaigai.gr.jp> writes:

The attached patch is the revised version of ALTER RENAME TO
consolidation. According to the previous suggestion, it uses
a common logic to check object-naming duplication at
check_duplicate_objectname().

I think you need to better comment which object types are supported by
the new generic function and which are not in AlterObjectRename().

At the code path on AlterObjectNamespace_internal, it lost
ObjectType information to the supplied object, so I also added
get_object_type() function at objectaddress.c for inverse
translation from a couple of classId/objectId to OBJECT_* label.

I don't understand that part, and it looks like it would be way better
to find a way to pass down the information you already have earlier in
the code than to compute it again doing syscache lookups…

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Hi Dimitri,

Thanks for your checks.

2012/11/19 Dimitri Fontaine <dimitri@2ndquadrant.fr>:

Kohei KaiGai <kaigai@kaigai.gr.jp> writes:

The attached patch is the revised version of ALTER RENAME TO
consolidation. According to the previous suggestion, it uses
a common logic to check object-naming duplication at
check_duplicate_objectname().

I think you need to better comment which object types are supported by
the new generic function and which are not in AlterObjectRename().

OK, Are you suggesting to add a "generic" comments such as "Generic
function to change the name of a given object, for simple cases ...",
not a list of OBJECT_* at the head of this function, aren't you?

At the code path on AlterObjectNamespace_internal, it lost
ObjectType information to the supplied object, so I also added
get_object_type() function at objectaddress.c for inverse
translation from a couple of classId/objectId to OBJECT_* label.

I don't understand that part, and it looks like it would be way better
to find a way to pass down the information you already have earlier in
the code than to compute it again doing syscache lookups…

The pain point is AlterObjectNamespace_internal is not invoked by
only ExecAlterObjectSchemaStmt(), but AlterObjectNamespace_oid()
also.
It is the reason why I had to put get_object_type() to find out OBJECT_*
from the supplied ObjectAddress, because this code path does not
available to pass down its ObjectType from the caller.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

Kohei KaiGai <kaigai@kaigai.gr.jp> writes:

OK, Are you suggesting to add a "generic" comments such as "Generic
function to change the name of a given object, for simple cases ...",
not a list of OBJECT_* at the head of this function, aren't you?

Just something like

* Most simple objects are covered by a generic function, the other
* still need special processing.

Mainly I was surprised to see collation not supported here, but I didn't
take enough time to understand why that is the case. I will do that
later in the review process.

The pain point is AlterObjectNamespace_internal is not invoked by
only ExecAlterObjectSchemaStmt(), but AlterObjectNamespace_oid()
also.

I should remember better about that as the use case is extensions…

It is the reason why I had to put get_object_type() to find out OBJECT_*
from the supplied ObjectAddress, because this code path does not
available to pass down its ObjectType from the caller.

If we really want to do that, I think I would only do that in
AlterObjectNamespace_oid and add an argument to
AlterObjectNamespace_internal, that you already have in
ExecAlterObjectSchemaStmt().

But really, what about just removing that part of your patch instead:

@@ -396,14 +614,23 @@ AlterObjectNamespace_internal(Relation rel, Oid objid, Oid nspOid)
 	 * Check for duplicate name (more friendly than unique-index failure).
 	 * Since this is just a friendliness check, we can just skip it in cases
 	 * where there isn't a suitable syscache available.
+	 *
+	 * XXX - the caller should check object-name duplication, if the supplied
+	 * object type need to take object arguments for identification, such as
+	 * functions.
 	 */
-	if (nameCacheId >= 0 &&
-		SearchSysCacheExists2(nameCacheId, name, ObjectIdGetDatum(nspOid)))
-		ereport(ERROR,
-				(errcode(ERRCODE_DUPLICATE_OBJECT),
-				 errmsg("%s already exists in schema \"%s\"",
-						getObjectDescriptionOids(classId, objid),
-						get_namespace_name(nspOid))));
+	if (get_object_catcache_name(classId) >= 0)
+	{
+		ObjectAddress	address;
+
+		address.classId = classId;
+		address.objectId = objid;
+		address.objectSubId = 0;
+
+		objtype = get_object_type(&address);
+		check_duplicate_objectname(objtype, nspOid,
+								   NameStr(*DatumGetName(name)), NIL);
+	}

It would be much simpler to retain the old-style duplicate object check,
and compared to doing extra cache lookups, it'd still be much cleaner in
my view.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

2012/11/19 Dimitri Fontaine <dimitri@2ndquadrant.fr>:

Kohei KaiGai <kaigai@kaigai.gr.jp> writes:

OK, Are you suggesting to add a "generic" comments such as "Generic
function to change the name of a given object, for simple cases ...",
not a list of OBJECT_* at the head of this function, aren't you?

Just something like

* Most simple objects are covered by a generic function, the other
* still need special processing.

Mainly I was surprised to see collation not supported here, but I didn't
take enough time to understand why that is the case. I will do that
later in the review process.

The reason why collation is not supported is that takes special name-
duplication checks. The new collation name must have no collision on
both of current database encoding and "any" encoding.
It might be an idea to have a simple rule similar to
AlterObjectNamespace_internal(); that requires caller to check
namespace-duplication, if the given object type has no catcache-id
with name-key.

The pain point is AlterObjectNamespace_internal is not invoked by
only ExecAlterObjectSchemaStmt(), but AlterObjectNamespace_oid()
also.

I should remember better about that as the use case is extensions…

It is the reason why I had to put get_object_type() to find out OBJECT_*
from the supplied ObjectAddress, because this code path does not
available to pass down its ObjectType from the caller.

If we really want to do that, I think I would only do that in
AlterObjectNamespace_oid and add an argument to
AlterObjectNamespace_internal, that you already have in
ExecAlterObjectSchemaStmt().

But really, what about just removing that part of your patch instead:

@@ -396,14 +614,23 @@ AlterObjectNamespace_internal(Relation rel, Oid objid, Oid nspOid)
* Check for duplicate name (more friendly than unique-index failure).
* Since this is just a friendliness check, we can just skip it in cases
* where there isn't a suitable syscache available.
+        *
+        * XXX - the caller should check object-name duplication, if the supplied
+        * object type need to take object arguments for identification, such as
+        * functions.
*/
-       if (nameCacheId >= 0 &&
-               SearchSysCacheExists2(nameCacheId, name, ObjectIdGetDatum(nspOid)))
-               ereport(ERROR,
-                               (errcode(ERRCODE_DUPLICATE_OBJECT),
-                                errmsg("%s already exists in schema \"%s\"",
-                                               getObjectDescriptionOids(classId, objid),
-                                               get_namespace_name(nspOid))));
+       if (get_object_catcache_name(classId) >= 0)
+       {
+               ObjectAddress   address;
+
+               address.classId = classId;
+               address.objectId = objid;
+               address.objectSubId = 0;
+
+               objtype = get_object_type(&address);
+               check_duplicate_objectname(objtype, nspOid,
+                                                                  NameStr(*DatumGetName(name)), NIL);
+       }

It would be much simpler to retain the old-style duplicate object check,
and compared to doing extra cache lookups, it'd still be much cleaner in
my view.

Now, I get inclined to follow the manner of AlterObjectNamespace_internal
at AlterObjectRename also; that requires caller to check name duplication
in case when no catcache entry is supported.

That simplifies the logic to check name duplication, and allows to eliminate
get_object_type() here, even though RenameAggregate and
RenameFunction have to be remained.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

2012/11/19 Kohei KaiGai <kaigai@kaigai.gr.jp>:

2012/11/19 Dimitri Fontaine <dimitri@2ndquadrant.fr>:

Kohei KaiGai <kaigai@kaigai.gr.jp> writes:

OK, Are you suggesting to add a "generic" comments such as "Generic
function to change the name of a given object, for simple cases ...",
not a list of OBJECT_* at the head of this function, aren't you?

Just something like

* Most simple objects are covered by a generic function, the other
* still need special processing.

Mainly I was surprised to see collation not supported here, but I didn't
take enough time to understand why that is the case. I will do that
later in the review process.

The reason why collation is not supported is that takes special name-
duplication checks. The new collation name must have no collision on
both of current database encoding and "any" encoding.
It might be an idea to have a simple rule similar to
AlterObjectNamespace_internal(); that requires caller to check
namespace-duplication, if the given object type has no catcache-id
with name-key.

The pain point is AlterObjectNamespace_internal is not invoked by
only ExecAlterObjectSchemaStmt(), but AlterObjectNamespace_oid()
also.

I should remember better about that as the use case is extensions…

It is the reason why I had to put get_object_type() to find out OBJECT_*
from the supplied ObjectAddress, because this code path does not
available to pass down its ObjectType from the caller.

If we really want to do that, I think I would only do that in
AlterObjectNamespace_oid and add an argument to
AlterObjectNamespace_internal, that you already have in
ExecAlterObjectSchemaStmt().

But really, what about just removing that part of your patch instead:

@@ -396,14 +614,23 @@ AlterObjectNamespace_internal(Relation rel, Oid objid, Oid nspOid)
* Check for duplicate name (more friendly than unique-index failure).
* Since this is just a friendliness check, we can just skip it in cases
* where there isn't a suitable syscache available.
+        *
+        * XXX - the caller should check object-name duplication, if the supplied
+        * object type need to take object arguments for identification, such as
+        * functions.
*/
-       if (nameCacheId >= 0 &&
-               SearchSysCacheExists2(nameCacheId, name, ObjectIdGetDatum(nspOid)))
-               ereport(ERROR,
-                               (errcode(ERRCODE_DUPLICATE_OBJECT),
-                                errmsg("%s already exists in schema \"%s\"",
-                                               getObjectDescriptionOids(classId, objid),
-                                               get_namespace_name(nspOid))));
+       if (get_object_catcache_name(classId) >= 0)
+       {
+               ObjectAddress   address;
+
+               address.classId = classId;
+               address.objectId = objid;
+               address.objectSubId = 0;
+
+               objtype = get_object_type(&address);
+               check_duplicate_objectname(objtype, nspOid,
+                                                                  NameStr(*DatumGetName(name)), NIL);
+       }

It would be much simpler to retain the old-style duplicate object check,
and compared to doing extra cache lookups, it'd still be much cleaner in
my view.

Now, I get inclined to follow the manner of AlterObjectNamespace_internal
at AlterObjectRename also; that requires caller to check name duplication
in case when no catcache entry is supported.

That simplifies the logic to check name duplication, and allows to eliminate
get_object_type() here, even though RenameAggregate and
RenameFunction have to be remained.

The attached patch is a revised version.

It follows the manner in ExecAlterObjectSchemaStmt(); that tries to check
name duplication for object classes that support catcache with name-key.
Elsewhere, it calls individual routines for each object class to solve object
name and check namespace conflicts.
For example, RenameFunction() is still called from ExecRenameStmt(),
however, it looks up the given function name and checks namespace
conflict only, then it just invokes AlterObjectRename_internal() in spite
of system catalog updates by itself.
It allows to use this consolidated routine by object classes with special
rule for namespace conflicts, such as collation.
In addition, this design allowed to eliminate get_object_type() to pull
OBJECT_* enum from class_id/object_id pair.

Please check this patch.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>