Re: [HACKERS] ALTER command reworks

[Alvaro Herrera]

Kohei KaiGai escribió:
 2013/2/3 Tom Lane t...@sss.pgh.pa.us:
  Alvaro Herrera alvhe...@2ndquadrant.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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Tom Lane]

Alvaro Herrera alvhe...@2ndquadrant.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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Kohei KaiGai]

2013/2/3 Tom Lane t...@sss.pgh.pa.us:
 Alvaro Herrera alvhe...@2ndquadrant.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 kai...@kaigai.gr.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Alvaro Herrera]

Tom Lane escribió:
 Kohei KaiGai kai...@kaigai.gr.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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Tom Lane]

Kohei KaiGai kai...@kaigai.gr.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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Kohei KaiGai]

2013/1/17 Alvaro Herrera alvhe...@2ndquadrant.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 kai...@kaigai.gr.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Alvaro Herrera]

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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


pgsql-v9.3-alter-reworks.3-rename.v10.patch.gz
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Alvaro Herrera]

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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Kohei KaiGai]

2013/1/15 Alvaro Herrera alvhe...@2ndquadrant.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 kai...@kaigai.gr.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Alvaro Herrera]

Kohei KaiGai escribió:
 2013/1/15 Alvaro Herrera alvhe...@2ndquadrant.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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Alvaro Herrera]

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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services
*** a/src/backend/commands/aggregatecmds.c
--- b/src/backend/commands/aggregatecmds.c
***
*** 29,34 
--- 29,35 
  #include catalog/pg_aggregate.h
  #include catalog/pg_proc.h
  #include catalog/pg_type.h
+ #include commands/alter.h
  #include commands/defrem.h
  #include miscadmin.h
  #include parser/parse_func.h
***
*** 240,254  RenameAggregate(List *name, List *args, const char *newname)
HeapTuple   tup;
Form_pg_proc procForm;
Relationrel;
-   AclResult   aclresult;
  
rel = heap_open(ProcedureRelationId, RowExclusiveLock);
  
/* Look up function and make sure it's an aggregate */
procOid = LookupAggNameTypeNames(name, args, false);
  
!   tup = SearchSysCacheCopy1(PROCOID, ObjectIdGetDatum(procOid));
!   if (!HeapTupleIsValid(tup)) /* should not happen */
elog(ERROR, cache lookup failed for function %u, procOid);
procForm = (Form_pg_proc) GETSTRUCT(tup);
  
--- 241,254 
HeapTuple   tup;
Form_pg_proc procForm;
Relationrel;
  
rel = heap_open(ProcedureRelationId, RowExclusiveLock);
  
/* Look up function and make sure it's an aggregate */
procOid = LookupAggNameTypeNames(name, args, false);
  
!   tup = SearchSysCache1(PROCOID, ObjectIdGetDatum(procOid));
!   if (!HeapTupleIsValid(tup)) /* should not happen */
elog(ERROR, cache lookup failed for function %u, procOid);
procForm = (Form_pg_proc) GETSTRUCT(tup);
  
***
*** 268,291  RenameAggregate(List *name, List *args, const char *newname)

   procForm-proargtypes.values),

get_namespace_name(namespaceOid;
  
!   /* must be owner */
!   if (!pg_proc_ownercheck(procOid, GetUserId()))
!   aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_PROC,
!  NameListToString(name));
! 
!   /* must have CREATE privilege on namespace */
!   aclresult = pg_namespace_aclcheck(namespaceOid, GetUserId(), 
ACL_CREATE);
!   if (aclresult != ACLCHECK_OK)
!   aclcheck_error(aclresult, ACL_KIND_NAMESPACE,
!  get_namespace_name(namespaceOid));
! 
!   /* rename */
!   namestrcpyForm_pg_proc) GETSTRUCT(tup))-proname), newname);
!   simple_heap_update(rel, tup-t_self, tup);
!   CatalogUpdateIndexes(rel, tup);
  
heap_close(rel, 

Re: [HACKERS] ALTER command reworks

[Robert Haas]

On Mon, Jan 7, 2013 at 3:43 PM, Alvaro Herrera alvhe...@2ndquadrant.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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Tom Lane]

Robert Haas robertmh...@gmail.com writes:
 On Mon, Jan 7, 2013 at 3:43 PM, Alvaro Herrera alvhe...@2ndquadrant.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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Kohei KaiGai]

2013/1/7 Tom Lane t...@sss.pgh.pa.us:
 Robert Haas robertmh...@gmail.com writes:
 On Mon, Jan 7, 2013 at 3:43 PM, Alvaro Herrera alvhe...@2ndquadrant.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 kai...@kaigai.gr.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Alvaro Herrera]

Tom Lane escribió:
 Robert Haas robertmh...@gmail.com writes:
  On Mon, Jan 7, 2013 at 3:43 PM, Alvaro Herrera alvhe...@2ndquadrant.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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Dimitri Fontaine]

Kohei KaiGai kai...@kaigai.gr.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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Dimitri Fontaine]

Hi,

Thanks for working on that cleanup job!

Kohei KaiGai kai...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Kohei KaiGai]

Hi Dimitri,

Thanks for your checks.

2012/11/19 Dimitri Fontaine dimi...@2ndquadrant.fr:
 Kohei KaiGai kai...@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 kai...@kaigai.gr.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Dimitri Fontaine]

Kohei KaiGai kai...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Kohei KaiGai]

2012/11/19 Dimitri Fontaine dimi...@2ndquadrant.fr:
 Kohei KaiGai kai...@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 kai...@kaigai.gr.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Alvaro Herrera]

Kohei KaiGai escribió:
 2012/10/5 Alvaro Herrera alvhe...@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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Kohei KaiGai]

2012/10/2 Alvaro Herrera alvhe...@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 kai...@kaigai.gr.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Alvaro Herrera]

Kohei KaiGai escribió:
 2012/8/31 Kohei KaiGai kai...@kaigai.gr.jp:
  2012/8/30 Robert Haas robertmh...@gmail.com:
  On Mon, Aug 13, 2012 at 3:35 PM, Kohei KaiGai kai...@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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services
*** a/src/backend/commands/aggregatecmds.c
--- b/src/backend/commands/aggregatecmds.c
***
*** 206,269  DefineAggregate(List *name, List *args, bool oldstyle, List *parameters)
  	transTypeId,	/* transition data type */
  	initval);	/* initial condition */
  }
- 
- 
- /*
-  * RenameAggregate
-  *		Rename an aggregate.
-  */
- void
- RenameAggregate(List *name, List *args, const char *newname)
- {
- 	Oid			procOid;
- 	Oid			namespaceOid;
- 	HeapTuple	tup;
- 	Form_pg_proc procForm;
- 	Relation	rel;
- 	AclResult	aclresult;
- 
- 	rel = heap_open(ProcedureRelationId, RowExclusiveLock);
- 
- 	/* Look up function and make sure it's an aggregate */
- 	procOid = LookupAggNameTypeNames(name, args, false);
- 
- 	tup = SearchSysCacheCopy1(PROCOID, ObjectIdGetDatum(procOid));
- 	if (!HeapTupleIsValid(tup)) /* should not happen */
- 		elog(ERROR, cache lookup failed for function %u, procOid);
- 	procForm = (Form_pg_proc) GETSTRUCT(tup);
- 
- 	namespaceOid = procForm-pronamespace;
- 
- 	/* make sure the new name doesn't exist */
- 	if (SearchSysCacheExists3(PROCNAMEARGSNSP,
- 			  CStringGetDatum(newname),
- 			  PointerGetDatum(procForm-proargtypes),
- 			  ObjectIdGetDatum(namespaceOid)))
- 		ereport(ERROR,
- (errcode(ERRCODE_DUPLICATE_FUNCTION),
-  errmsg(function %s already exists in schema \%s\,
- 		funcname_signature_string(newname,
-   procForm-pronargs,
-   NIL,
- 			   procForm-proargtypes.values),
- 		get_namespace_name(namespaceOid;
- 
- 	/* must be owner */
- 	if (!pg_proc_ownercheck(procOid, GetUserId()))
- 		aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_PROC,
- 	   NameListToString(name));
- 
- 	/* must have CREATE privilege on namespace */
- 	aclresult = pg_namespace_aclcheck(namespaceOid, GetUserId(), ACL_CREATE);
- 	if (aclresult != ACLCHECK_OK)
- 		aclcheck_error(aclresult, ACL_KIND_NAMESPACE,
- 	   get_namespace_name(namespaceOid));
- 
- 	/* rename */
- 	namestrcpyForm_pg_proc) GETSTRUCT(tup))-proname), newname);
- 	simple_heap_update(rel, tup-t_self, tup);
- 	CatalogUpdateIndexes(rel, tup);
- 
- 	heap_close(rel, NoLock);
- 	heap_freetuple(tup);
- }
--- 206,208 
*** a/src/backend/commands/alter.c
--- b/src/backend/commands/alter.c
***
*** 45,50 
--- 45,261 
  #include utils/syscache.h
  #include utils/tqual.h
  
+ static HeapTuple get_catalog_object_by_oid(Relation catalog, Oid objectId);
+ 
+ /*
+  * errmsg_obj_already_exists
+  *
+  * Returns an error message, to be used as errmsg(), indicating that the
+  * supplied object name conflicts with an existing object in the given
+  * namespace, depending on the given object type.
+  *
+  * Because of message translatability, we don't use getObjectDescription()
+  * here.
+  */
+ 

Re: [HACKERS] ALTER command reworks

[Alvaro Herrera]

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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Alvaro Herrera]

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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Alvaro Herrera]

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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Alvaro Herrera]

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.0 -0300
--- 
/home/alvherre/Code/pgsql/build/HEAD/src/test/regress/results/alter_generic.out 
2012-09-27 17:29:21.0 -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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Robert Haas]

On Mon, Aug 13, 2012 at 3:35 PM, Kohei KaiGai kai...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER command reworks

[Kohei KaiGai]

2012/8/30 Robert Haas robertmh...@gmail.com:
 On Mon, Aug 13, 2012 at 3:35 PM, Kohei KaiGai kai...@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 kai...@kaigai.gr.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers