Re: [HACKERS] ALTER command reworks
Kohei KaiGai escribió: > 2013/2/3 Tom Lane : > > Alvaro Herrera 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
2013/2/3 Tom Lane : > Alvaro Herrera 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 -- 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 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
Tom Lane escribió: > Kohei KaiGai 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
Kohei KaiGai 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
2013/1/17 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. > 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 -- 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 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
Kohei KaiGai escribió:
> 2013/1/15 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?
> >
> 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
2013/1/15 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? > 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 -- 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 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
Tom Lane escribió: > Robert Haas writes: > > On Mon, Jan 7, 2013 at 3:43 PM, Alvaro Herrera > > 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
2013/1/7 Tom Lane : > Robert Haas writes: >> On Mon, Jan 7, 2013 at 3:43 PM, Alvaro Herrera >> 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 -- 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 writes: > On Mon, Jan 7, 2013 at 3:43 PM, Alvaro Herrera > 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
On Mon, Jan 7, 2013 at 3:43 PM, Alvaro Herrera 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
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 */
! namestrcpy(&(((Form_pg_proc) GETSTRUCT(tup))->proname), newname);
! simple_heap_update(rel, &tup->t_self, tup);
! Catal
Re: [HACKERS] ALTER command reworks
Kohei KaiGai 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
2012/11/19 Dimitri Fontaine :
> Kohei KaiGai 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
--
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 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
Hi Dimitri, Thanks for your checks. 2012/11/19 Dimitri Fontaine : > Kohei KaiGai 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 -- 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
Hi, Thanks for working on that cleanup job! Kohei KaiGai 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 escribió:
> 2012/10/5 Alvaro Herrera :
> 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
2012/10/2 Alvaro Herrera : > 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 -- 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 escribió:
> 2012/8/31 Kohei KaiGai :
> > 2012/8/30 Robert Haas :
> >> On Mon, Aug 13, 2012 at 3:35 PM, Kohei KaiGai 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 */
- namestrcpy(&(((Form_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()
Re: [HACKERS] ALTER command reworks
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
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
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
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
2012/8/30 Robert Haas : > On Mon, Aug 13, 2012 at 3:35 PM, Kohei KaiGai 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 -- 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
On Mon, Aug 13, 2012 at 3:35 PM, Kohei KaiGai 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
