Re: ALTER command reworks

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(at)kaigai(dot)gr(dot)jp>

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(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

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

On Mon, Aug 13, 2012 at 3:35 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)gmail(dot)com>:
> On Mon, Aug 13, 2012 at 3:35 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

2012/8/31 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 2012/8/30 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Mon, Aug 13, 2012 at 3:35 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>:
> > 2012/8/30 Robert Haas <robertmhaas(at)gmail(dot)com>:
> >> On Mon, Aug 13, 2012 at 3:35 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)2ndquadrant(dot)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(at)kaigai(dot)gr(dot)jp>

2012/10/5 Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>:
> Kohei KaiGai escribió:
>> 2012/8/31 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
>> > 2012/8/30 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> >> On Mon, Aug 13, 2012 at 3:35 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

Kohei KaiGai escribió:
> 2012/10/5 Alvaro Herrera <alvherre(at)2ndquadrant(dot)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(at)2ndquadrant(dot)com>:
> Kohei KaiGai escribió:
>> 2012/10/5 Alvaro Herrera <alvherre(at)2ndquadrant(dot)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(at)kaigai(dot)gr(dot)jp>

Hi,

Thanks for working on that cleanup job!

Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)2ndquadrant(dot)fr>:
> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)2ndquadrant(dot)fr>:
> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

2012/11/19 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 2012/11/19 Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>:
>> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
> The attached patch is a revised version.

I think you fixed all the problems I could see with your patch. It
applies cleanly (with some offsets), compiles cleanly (I have a custom
Makefile with CUSTOM_COPT=-Werror) and passes regression tests.

I'll go mark it as ready for commiter. Thanks!

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

I think that's much better this way.

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

Thanks for having done this refactoring.

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

Kohei KaiGai escribió:

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

I checked this patch. It needed a rebase for the changes to return
OIDs. Attached patch applies to current HEAD. In general this looks
good, with one exception: it's using getObjectDescriptionOids() to build
the messages to complain in case the object already exists in the
current schema, which results in diffs like this:

-ERROR: event trigger "regress_event_trigger2" already exists
+ERROR: event trigger regress_event_trigger2 already exists

I don't know how tense we are about keeping the quotes, but I fear there
would be complaints because it took us lots of sweat, blood and tears to
get where we are now.

If this is considered a problem, I think the way to fix it is to have a
getObjectDescriptionOids() variant that quotes the object name in the
output. This would be pretty intrusive: we'd have to change things
so that, for instance,

appendStringInfo(&buffer, _("collation %s"),
NameStr(coll->collname));

would become

if (quotes)
appendStringInfo(&buffer, _("collation \"%s\""),
NameStr(coll->collname));
else
appendStringInfo(&buffer, _("collation %s"),
NameStr(coll->collname));

Not really thrilled with this idea. Of course,
getObjectDescription() itself should keep the same API as now, to avoid
useless churn all over the place.

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

On Mon, Jan 7, 2013 at 3:43 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> Kohei KaiGai escribió:
>
>> 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.
>
> I checked this patch. It needed a rebase for the changes to return
> OIDs. Attached patch applies to current HEAD. In general this looks
> good, with one exception: it's using getObjectDescriptionOids() to build
> the messages to complain in case the object already exists in the
> current schema, which results in diffs like this:
>
> -ERROR: event trigger "regress_event_trigger2" already exists
> +ERROR: event trigger regress_event_trigger2 already exists
>
> I don't know how tense we are about keeping the quotes, but I fear there
> would be complaints because it took us lots of sweat, blood and tears to
> get where we are now.
>
> If this is considered a problem, I think the way to fix it is to have a
> getObjectDescriptionOids() variant that quotes the object name in the
> output. This would be pretty intrusive: we'd have to change things
> so that, for instance,
>
> appendStringInfo(&buffer, _("collation %s"),
> NameStr(coll->collname));
>
> would become
>
> if (quotes)
> appendStringInfo(&buffer, _("collation \"%s\""),
> NameStr(coll->collname));
> else
> appendStringInfo(&buffer, _("collation %s"),
> NameStr(coll->collname));
>
> Not really thrilled with this idea. Of course,
> getObjectDescription() itself should keep the same API as now, to avoid
> useless churn all over the place.

This sort of thing has been rejected repeatedly in the past on
translation grounds:

+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("%s already exists in schema \"%s\"",
+ getObjectDescriptionOids(classId, exists),
+ get_namespace_name(namespaceId))));

If we're going to start allowing that, there's plenty of other code
that can be hit with the same hammer, but I'm pretty sure that all
previous proposals in this area have been slapped down.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Jan 7, 2013 at 3:43 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>> I checked this patch. It needed a rebase for the changes to return
>> OIDs. Attached patch applies to current HEAD. In general this looks
>> good, with one exception: it's using getObjectDescriptionOids() to build
>> the messages to complain in case the object already exists in the
>> current schema, which results in diffs like this:
>>
>> -ERROR: event trigger "regress_event_trigger2" already exists
>> +ERROR: event trigger regress_event_trigger2 already exists
>>
>> I don't know how tense we are about keeping the quotes, but I fear there
>> would be complaints because it took us lots of sweat, blood and tears to
>> get where we are now.
>>
>> If this is considered a problem, I think the way to fix it is to have a
>> getObjectDescriptionOids() variant that quotes the object name in the
>> output.

> This sort of thing has been rejected repeatedly in the past on
> translation grounds:

Yes. I'm surprised Alvaro isn't well aware of the rules against trying
to build error messages out of sentence fragments: see first item under
http://www.postgresql.org/docs/devel/static/nls-programmer.html#NLS-GUIDELINES

Presence or absence of quotes is the very least of this code's i18n
problems.

If we had no other choice, we might consider a workaround such as that
suggested in "Assembling Error Messages"
http://www.postgresql.org/docs/devel/static/error-style-guide.html#AEN98605
but frankly I'm not convinced that this patch is attractive enough to
justify a degradation in message readability.

regards, tom lane

2013/1/7 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Mon, Jan 7, 2013 at 3:43 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>>> I checked this patch. It needed a rebase for the changes to return
>>> OIDs. Attached patch applies to current HEAD. In general this looks
>>> good, with one exception: it's using getObjectDescriptionOids() to build
>>> the messages to complain in case the object already exists in the
>>> current schema, which results in diffs like this:
>>>
>>> -ERROR: event trigger "regress_event_trigger2" already exists
>>> +ERROR: event trigger regress_event_trigger2 already exists
>>>
>>> I don't know how tense we are about keeping the quotes, but I fear there
>>> would be complaints because it took us lots of sweat, blood and tears to
>>> get where we are now.
>>>
>>> If this is considered a problem, I think the way to fix it is to have a
>>> getObjectDescriptionOids() variant that quotes the object name in the
>>> output.
>
>> This sort of thing has been rejected repeatedly in the past on
>> translation grounds:
>
> Yes. I'm surprised Alvaro isn't well aware of the rules against trying
> to build error messages out of sentence fragments: see first item under
> http://www.postgresql.org/docs/devel/static/nls-programmer.html#NLS-GUIDELINES
>
> Presence or absence of quotes is the very least of this code's i18n
> problems.
>
> If we had no other choice, we might consider a workaround such as that
> suggested in "Assembling Error Messages"
> http://www.postgresql.org/docs/devel/static/error-style-guide.html#AEN98605
> but frankly I'm not convinced that this patch is attractive enough to
> justify a degradation in message readability.
>
Sorry, I forgot Robert pointed out same thing before.
I'll reconstruct the portion to raise an error message.

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

Tom Lane escribió:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Mon, Jan 7, 2013 at 3:43 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:

> >> If this is considered a problem, I think the way to fix it is to have a
> >> getObjectDescriptionOids() variant that quotes the object name in the
> >> output.
>
> > This sort of thing has been rejected repeatedly in the past on
> > translation grounds:
>
> Yes. I'm surprised Alvaro isn't well aware of the rules against trying
> to build error messages out of sentence fragments: see first item under
> http://www.postgresql.org/docs/devel/static/nls-programmer.html#NLS-GUIDELINES

Actually I'm fully aware of the limitations; I just had a brain fade. I
knew there was something wrong with that usage of getObjectDescription,
but managed to miss what it was exactly. Doh. Thankfully there are
other hackers paying attention.

BTW this patch was correct in this regard in a previous version -- there
was one separate copy of the error message per object type. I think
it's just a matter of returning to that older coding.

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

2013/1/7 Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>:
> Tom Lane escribió:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> > On Mon, Jan 7, 2013 at 3:43 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>
>> >> If this is considered a problem, I think the way to fix it is to have a
>> >> getObjectDescriptionOids() variant that quotes the object name in the
>> >> output.
>>
>> > This sort of thing has been rejected repeatedly in the past on
>> > translation grounds:
>>
>> Yes. I'm surprised Alvaro isn't well aware of the rules against trying
>> to build error messages out of sentence fragments: see first item under
>> http://www.postgresql.org/docs/devel/static/nls-programmer.html#NLS-GUIDELINES
>
> Actually I'm fully aware of the limitations; I just had a brain fade. I
> knew there was something wrong with that usage of getObjectDescription,
> but managed to miss what it was exactly. Doh. Thankfully there are
> other hackers paying attention.
>
> BTW this patch was correct in this regard in a previous version -- there
> was one separate copy of the error message per object type. I think
> it's just a matter of returning to that older coding.
>
The attached patch is a rebased version towards the latest master
branch, and fix up the issue around error messages on name conflicts.

I could also find an abused of getObjectDescription() for error message
of namespace conflicts at AlterObjectNamespace_internal().
The newly added ereport_on_namespace_conflict() can handle the
both of cases, so I also replaced it.

As we have a discussion in another thread, we may add special case
handling for functions and collation on checks on namespace conflicts,
however, I didn't tough code path of them, right now.

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

Kohei KaiGai escribió:

> The attached patch is a rebased version towards the latest master
> branch, and fix up the issue around error messages on name conflicts.

I assume the lock.c changes are just a bollixed merge, right?

I am not sure about some of the changes in the regression test expected
output; are we really okay with losing the "in schema foo" part in some
of these cases?

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

2013/1/15 Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>:
> Kohei KaiGai escribió:
>
>> The attached patch is a rebased version towards the latest master
>> branch, and fix up the issue around error messages on name conflicts.
>
> I assume the lock.c changes are just a bollixed merge, right?
>
Yes, I'll check it and rebase it.

> I am not sure about some of the changes in the regression test expected
> output; are we really okay with losing the "in schema foo" part in some
> of these cases?
>
The changes in the expected results in regression test originated from
elimination of getObjectDescription, but "in schema foo" should be kept.
It looks like an obvious my mistake. Sorry.

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

Kohei KaiGai escribió:
> 2013/1/15 Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>:
> > Kohei KaiGai escribió:
> >
> >> The attached patch is a rebased version towards the latest master
> >> branch, and fix up the issue around error messages on name conflicts.
> >
> > I assume the lock.c changes are just a bollixed merge, right?
> >
> Yes, I'll check it and rebase it.

Wait for a bit before publishing a new version -- I'm going to push the
other patch so that you can merge on top.

Note that I'm going to commit a new function like this:

/*
* Raise an error to the effect that an object of the given name is already
* present in the given namespace.
*/
static void
report_namespace_conflict(Oid classId, const char *name, Oid nspOid)
{
char *msgfmt;

Assert(OidIsValid(nspOid));

For this patch you will need to create a separate function which does
the conflicting-name report for objects that are not in a namespace.
Mixing both in-schema and schemaless objects in the same report function
seems messy to me.

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

2013/1/15 Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>:
> Kohei KaiGai escribió:
>> 2013/1/15 Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>:
>> > Kohei KaiGai escribió:
>> >
>> >> The attached patch is a rebased version towards the latest master
>> >> branch, and fix up the issue around error messages on name conflicts.
>> >
>> > I assume the lock.c changes are just a bollixed merge, right?
>> >
>> Yes, I'll check it and rebase it.
>
> Wait for a bit before publishing a new version -- I'm going to push the
> other patch so that you can merge on top.
>
> Note that I'm going to commit a new function like this:
>
> /*
> * Raise an error to the effect that an object of the given name is already
> * present in the given namespace.
> */
> static void
> report_namespace_conflict(Oid classId, const char *name, Oid nspOid)
> {
> char *msgfmt;
>
> Assert(OidIsValid(nspOid));
>
>
> For this patch you will need to create a separate function which does
> the conflicting-name report for objects that are not in a namespace.
> Mixing both in-schema and schemaless objects in the same report function
> seems messy to me.
>
This attached patch is the rebased one towards the latest master branch.

I noticed that your proposed design also allows to unify ALTER code of
OPERATOR CLASS / FAMILY; that takes index access method for its
namespace in addition to name and namespace.
So, AlterObjectRename_internal and AlterObjectNamespace_internal have
four of special case handling to check name / namespace conflict.

The latest master lookups syscache within special case handing block
as follows:

if (classId == ProcedureRelationId)
{
HeapTuple tup;
Form_pg_proc proc;

tup = SearchSysCacheCopy1(PROCOID, ObjectIdGetDatum(objid));
if (!HeapTupleIsValid(tup))
elog(ERROR, "cache lookup failed for function %u", objid);
proc = (Form_pg_proc) GETSTRUCT(tup);

IsThereFunctionInNamespace(NameStr(proc->proname), proc->pronargs,
proc->proargtypes, nspOid);
heap_freetuple(tup);
}

But, we already pulls a relevant tuple from syscache on top of
AlterObjectNamespace_internal. So, I removed cache lookup code here.

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

Kohei KaiGai escribió:

> This attached patch is the rebased one towards the latest master branch.

Great, thanks. I played with it a bit and it looks almost done to me.
The only issue I can find is that it lets you rename an aggregate by
using ALTER FUNCTION, which is supposed to be forbidden. (Funnily
enough, renaming a non-agg function with ALTER AGGREGATE does raise an
error). Didn't immediately spot the right place to add a check.

I think these two error cases ought to have regression tests of their
own.

I attach a version with my changes.

> I noticed that your proposed design also allows to unify ALTER code of
> OPERATOR CLASS / FAMILY; that takes index access method for its
> namespace in addition to name and namespace.

Yeah, I had noticed that and was planning on implementing it later.
Thanks for saving me the work.

> So, AlterObjectRename_internal and AlterObjectNamespace_internal have
> four of special case handling to check name / namespace conflict.

Right. (I wonder if it would make sense to encapsulate all those checks
in a single function for both to call.)

> The latest master lookups syscache within special case handing block
> as follows:
> [code]
>
> But, we already pulls a relevant tuple from syscache on top of
> AlterObjectNamespace_internal. So, I removed cache lookup code here.

Silly me. Thanks.

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

2013/1/17 Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>:
> Kohei KaiGai escribió:
>
>> This attached patch is the rebased one towards the latest master branch.
>
> Great, thanks. I played with it a bit and it looks almost done to me.
> The only issue I can find is that it lets you rename an aggregate by
> using ALTER FUNCTION, which is supposed to be forbidden. (Funnily
> enough, renaming a non-agg function with ALTER AGGREGATE does raise an
> error). Didn't immediately spot the right place to add a check.
>
> I think these two error cases ought to have regression tests of their
> own.
>
> I attach a version with my changes.
>
The patch itself looks to me good.

About ALTER FUNCTION towards aggregate function, why we should raise
an error strictly? Some code allows to modify properties of aggregate function
specified with FUNCTION qualifier.

postgres=# COMMENT ON FUNCTION max(int) IS 'maximum number of integer';
COMMENT
postgres=# COMMENT ON AGGREGATE in4eq(int,int) IS 'comparison of integers';
ERROR: aggregate in4eq(integer, integer) does not exist

I think using AGGREGATE towards regular function is wrong, but it is not
100% wrong to use FUNCTION towards aggregate "function".
In addition, aggregate function and regular function share same namespace,
so it never makes problem even if we allows to identify aggregate function
using ALTER FUNCTION.

How about your opinion? Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
> About ALTER FUNCTION towards aggregate function, why we should raise
> an error strictly?

I agree we probably shouldn't --- traditionally we have allowed that,
AFAIR, so changing it would break existing applications for little
benefit.

Similarly, you should not be throwing error when ALTER TABLE is applied
to a view, sequence, etc, and the command would otherwise be sensible.

regards, tom lane

Tom Lane escribió:
> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
> > About ALTER FUNCTION towards aggregate function, why we should raise
> > an error strictly?
>
> I agree we probably shouldn't --- traditionally we have allowed that,
> AFAIR, so changing it would break existing applications for little
> benefit.

Okay, I have pushed the version I posted last week.

> Similarly, you should not be throwing error when ALTER TABLE is applied
> to a view, sequence, etc, and the command would otherwise be sensible.

As far as ALTER some-obj RENAME goes, this is already working, so I
haven't changed anything.

Thanks,

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

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> [ pgsql-v9.3-alter-reworks.3-rename.v10.patch.gz ]

Say ... I hadn't been paying too close attention to this patch, but
is there any particularly principled reason for it having unified
only 14 of the 29 object types handled by ExecRenameStmt()?
If so, how to tell which object types are supposed to be covered?

The reason I'm asking is that it's very unclear to me whether
https://commitfest.postgresql.org/action/patch_view?id=1043
(ALTER RENAME RULE) is okay in more-or-less its current form,
or whether it ought to be bounced back to be reworked for integration
in this framework.

regards, tom lane

2013/2/3 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
>> [ pgsql-v9.3-alter-reworks.3-rename.v10.patch.gz ]
>
> Say ... I hadn't been paying too close attention to this patch, but
> is there any particularly principled reason for it having unified
> only 14 of the 29 object types handled by ExecRenameStmt()?
> If so, how to tell which object types are supposed to be covered?
>
> The reason I'm asking is that it's very unclear to me whether
> https://commitfest.postgresql.org/action/patch_view?id=1043
> (ALTER RENAME RULE) is okay in more-or-less its current form,
> or whether it ought to be bounced back to be reworked for integration
> in this framework.
>
Like trigger or constraint, rule is unavailable to integrate the generic
rename logic using AlterObjectRename_internal().
So, I don't think this patch needs to take much design change.

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

Kohei KaiGai escribió:
> 2013/2/3 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> > Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> >> [ pgsql-v9.3-alter-reworks.3-rename.v10.patch.gz ]
> >
> > Say ... I hadn't been paying too close attention to this patch, but
> > is there any particularly principled reason for it having unified
> > only 14 of the 29 object types handled by ExecRenameStmt()?
> > If so, how to tell which object types are supposed to be covered?
> >
> > The reason I'm asking is that it's very unclear to me whether
> > https://commitfest.postgresql.org/action/patch_view?id=1043
> > (ALTER RENAME RULE) is okay in more-or-less its current form,
> > or whether it ought to be bounced back to be reworked for integration
> > in this framework.
> >
> Like trigger or constraint, rule is unavailable to integrate the generic
> rename logic using AlterObjectRename_internal().
> So, I don't think this patch needs to take much design change.

I did give that patch a glance last week, asked myself the same question
as Tom, and gave myself the same answer as KaiGai. Sorry I didn't post
that.

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