Thread: How about to have relnamespace and relrole?
Hello, Most of OID types has reg* OID types. Theses are very convenient when looking into system catalog/views, but there aren't OID types for userid and namespace id. What do you think about having these new OID types? The usefulness of regnamespace is doubtful but regrole should be useful because the column like 'owner' appears many places. For example this works as follows. ==== SELECT relnamespace::regnamespace, relname, relowner::regrole FROM pg_class WHERE relnamespace = 'public'::regnamespace; relnamespace | relname | relowner --------------+---------+----------public | t1 | horiguti (1 row) What do you think about this? regards, -- Kyotaro Horiguchi NTT Open Source Software Center
Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
> Most of OID types has reg* OID types. Theses are very convenient
> when looking into system catalog/views, but there aren't OID
> types for userid and namespace id.
> What do you think about having these new OID types?
I'm not really excited about that. That line of thought would imply
that we should have "reg*" types for every system catalog, which is
surely overkill.
The existing reg* types were chosen to handle the cases where objects have
schema-qualified (and/or type-overloaded) names so that correct lookup is
not merely a matter of (select oid from pg_foo where name = 'bar') or
vice versa.
I think the policy is, or should be, that we create reg* types for exactly
the cases where lookup isn't that simple.
In particular, both of the cases you mention are hardly "new". They
existed when we made the current set of reg* types and were consciously
not added then.
> SELECT relnamespace::regnamespace, relname, relowner::regrole
> FROM pg_class WHERE relnamespace = 'public'::regnamespace;
Two reasons this isn't terribly compelling are (1) it's creating a
join in a place where the planner can't possibly see it and optimize
it, and (2) you risk MVCC anomalies because the reg* output routines
would not be using the same snapshot as the calling query.
We already have problem (2) with the existing reg* functions so I'm
not that excited about doubling down on the concept.
regards, tom lane
Thank you for your comment. Sorry for the silly typo in the subject. Tue, 03 Feb 2015 10:12:12 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in <2540.1422976332@sss.pgh.pa.us> > Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes: > > Most of OID types has reg* OID types. Theses are very convenient > > when looking into system catalog/views, but there aren't OID > > types for userid and namespace id. > > > What do you think about having these new OID types? > > I'm not really excited about that. That line of thought would imply > that we should have "reg*" types for every system catalog, which is > surely overkill. Mmm. I suppose "for every OID usage", not "every system catalog". but I agree as the whole. There's no agreeable-by-all boundary. Perhaps it depends on how often the average DBA looks onto catalogs which have oids pointing another catalog which they want to see in human-readable form, without joins if possible. I very roughly counted how often the oids of catalogs referred from other catalogs. As the first step, catalogs which have oid are, select relname from pg_class where relnamespace = 'pg_catalog'::regnamespace and relhasoids = true; ... (34 rows) # Yes, regnamespace is very usable here:) I say that 34 is too many for sure to have reg* types. From the viewpoint of human-readable names, the more it enters DBA's sight, the more significance it could be said to have. Among the 34, the rough list of from which catalog they are referred to is following. pg_authid(role): pg_class, pg_type, pg_database, and many, many. pg_type: referred to from pg_operator, pg_proc, pg_cast, and many. pg_namespace: referred to from many catalogs. pg_class: pg_locks pg_database: pg_locks pg_ts_parser: pg_ts_config pg_ts_template: pg_ts_template pg_foreign_data_wrapper: pg_foreign_server pg_foreign_server: pg_user_mapping This is not an comprehensive counting but I think I can confidently say that regrole has significant meaning. (and namespace also could be said so). I would make the comprehensive one if you or others think it's needed. (altough it would be a labor) What do you think about the point of view? > The existing reg* types were chosen to handle the cases where objects have > schema-qualified (and/or type-overloaded) names so that correct lookup is > not merely a matter of (select oid from pg_foo where name = 'bar') or > vice versa. > > I think the policy is, or should be, that we create reg* types for exactly > the cases where lookup isn't that simple. Yes, I have noticed that. And I agree that it is one reasonable boundary. But the point mentioned above is also a reasonable boundary. > In particular, both of the cases you mention are hardly "new". They > existed when we made the current set of reg* types and were consciously > not added then. > > > SELECT relnamespace::regnamespace, relname, relowner::regrole > > FROM pg_class WHERE relnamespace = 'public'::regnamespace; > > Two reasons this isn't terribly compelling are (1) it's creating a > join in a place where the planner can't possibly see it and optimize > it, Maybe un-optimiz-ability is not a matter as far as they are used in one-liners for administrative operations like I mentioned above. (On the contrary, syscache is far faster than normal table lookup for most cases, but it doesn't justify to use this in OLTP jobs even ignoring the MVCC issue.) > and (2) you risk MVCC anomalies because the reg* output routines > would not be using the same snapshot as the calling query. > We already have problem (2) with the existing reg* functions so I'm > not that excited about doubling down on the concept. Surely. Then the page for the reg* in the doc (datatype-oid.html) *shoud* mention such a caveat, but the limitation would be tolerable for user(DBA)s as the same as before. Once it is stated clearly, althgouh I won't intend to make it an excuse, I think some (or one) new reg* type can be acceptable. regards, -- Kyotaro Horiguchi NTT Open Source Software Center
Hello, I changed the subject. This mail is to address the point at hand, preparing for registering this commitfest. 15 17:29:14 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20150204.172914.52110711.horiguchi.kyotaro@lab.ntt.co.jp> > Tue, 03 Feb 2015 10:12:12 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in <2540.1422976332@sss.pgh.pa.us> > > I'm not really excited about that. That line of thought would imply > > that we should have "reg*" types for every system catalog, which is > > surely overkill. > > Mmm. I suppose "for every OID usage", not "every system catalog". > but I agree as the whole. There's no agreeable-by-all > boundary. Perhaps it depends on how often the average DBA looks > onto catalogs which have oids pointing another catalog which they > want to see in human-readable form, without joins if possible. > > I very roughly counted how often the oids of catalogs referred > from other catalogs. 1. Expected frequency of use I counted how often oids of system catalogs are referred to by other system catalog/views. Among them, pg_stat_* views, are excluded since they have text representations for all oid references. The result is like this. The full result of the counting is in the Excel file but it's not at hand for now.. I'll show it here if anyone wants to see it. pg_class.oid: 27 pg_authid.oid: 33 pg_namespace.oid: 20 pg_proc.oid: 13 pg_type.oid: 15 pg_databse.oid: 5 pg_operator.oid: 5 pg_am.oid: 4 .... Among these, authid and namespace are apparently referred to frequently but don't have their reg* types but referred to from more points than proc, type, operator, am.. # By the way, I don't understand where the "reg" comes from, # REGistered? Or other origin? For that reason, although the current policy of deciding whether to have reg* types seems to be whether they have schema-qualified names, I think regrole and regnamespace are valuable to have. 2. Anticipaed un-optimizability Tom pointed that these reg* types prevents planner from optimizing the query, so we should refrain from having such machinary. It should have been a long-standing issue but reg*s sees to be rather faster than joining corresponding catalogs for moderate number of the result rows, but this raises another more annoying issue. 3. Potentially breakage of MVCC The another issue Tom pointed is potentially breakage of MVCC by using these reg* types. Syscache is invalidated on updates so this doesn't seem to be a problem on READ COMMITTED mode, but breaks SERIALIZABLE mode. But IMHO it is not so serious problem as long as such inconsistency occurs only on system catalog and it is explicitly documented togethee with the un-optimizability issue. I'll add a patch for this later. regards, -- Kyotaro Horiguchi NTT Open Source Software Center
On 2/12/15 5:28 AM, Kyotaro HORIGUCHI wrote: > Hello, I changed the subject. > > This mail is to address the point at hand, preparing for > registering this commitfest. > > 15 17:29:14 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI > <horiguchi.kyotaro@lab.ntt.co.jp> wrote in > <20150204.172914.52110711.horiguchi.kyotaro@lab.ntt.co.jp> >> Tue, 03 Feb 2015 10:12:12 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in <2540.1422976332@sss.pgh.pa.us> >>> I'm not really excited about that. That line of thought would imply >>> that we should have "reg*" types for every system catalog, which is >>> surely overkill. >> >> Mmm. I suppose "for every OID usage", not "every system catalog". >> but I agree as the whole. There's no agreeable-by-all >> boundary. Perhaps it depends on how often the average DBA looks >> onto catalogs which have oids pointing another catalog which they >> want to see in human-readable form, without joins if possible. >> >> I very roughly counted how often the oids of catalogs referred >> from other catalogs. > > 1. Expected frequency of use ... > For that reason, although the current policy of deciding whether > to have reg* types seems to be whether they have schema-qualified > names, I think regrole and regnamespace are valuable to have. Perhaps schema qualification was the original intent, but I think at this point everyone uses them for convenience. Why would I want to type out JOIN pg_namespace n ON n.oid = blah.???namespace when I could simply do ???namespace::regnamespace? > 2. Anticipaed un-optimizability > > Tom pointed that these reg* types prevents planner from > optimizing the query, so we should refrain from having such > machinary. It should have been a long-standing issue but reg*s > sees to be rather faster than joining corresponding catalogs > for moderate number of the result rows, but this raises another > more annoying issue. > > > 3. Potentially breakage of MVCC > > The another issue Tom pointed is potentially breakage of MVCC by > using these reg* types. Syscache is invalidated on updates so > this doesn't seem to be a problem on READ COMMITTED mode, but > breaks SERIALIZABLE mode. But IMHO it is not so serious problem > as long as such inconsistency occurs only on system catalog and > it is explicitly documented togethee with the un-optimizability > issue. I'll add a patch for this later. The way I look at it, all these arguments went out the window when regclass was introduced. The reality is that reg* is *way* more convenient than manual joins. The arguments about optimization and MVCC presumably apply to all reg* casts, which means that ship has long since sailed. If we offered views that made it easier to get at this data then maybe this wouldn't matter so much. But DBA's don't want to join 3 catalogs together to try and answer a simple question. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com
Hello, thank you for the comment. The current patch lacks change in documentation and dependency stuff. Current framework doesn't consider changing pg_shdepend from column default expressions so the possible measures are the followings, I think. 1. Make pg_shdepend to have refobjsubid and add some deptypes of pg_depend, specifically DEPENDENCY_NORMAL is needed. Then, change the dependency stuff. 2. Simplly inhibit specifying them in default expressions. Integer or OID can act as the replacement. =# create table t1 (a int, b regrole default 'horiguti'::regrole); ERROR: Type 'regrole' cannot have a default value 1 is quite overkill but hardly seems to be usable. So I will go on 2 and post additional patch. At Thu, 12 Feb 2015 17:12:24 -0600, Jim Nasby <Jim.Nasby@BlueTreble.com> wrote in <54DD3358.9030601@BlueTreble.com> > On 2/12/15 5:28 AM, Kyotaro HORIGUCHI wrote: > > Hello, I changed the subject. # Ouch! the subject remaines wrong.. > > 1. Expected frequency of use > ... > > For that reason, although the current policy of deciding whether > > to have reg* types seems to be whether they have schema-qualified > > names, I think regrole and regnamespace are valuable to have. > > Perhaps schema qualification was the original intent, but I think at > this point everyone uses them for convenience. Why would I want to > type out JOIN pg_namespace n ON n.oid = blah.???namespace when I could > simply do ???namespace::regnamespace? Yes, that is the most important point of this patch. > > 2. Anticipaed un-optimizability > > > > Tom pointed that these reg* types prevents planner from > > optimizing the query, so we should refrain from having such > > machinary. It should have been a long-standing issue but reg*s > > sees to be rather faster than joining corresponding catalogs > > for moderate number of the result rows, but this raises another > > more annoying issue. > > > > > > 3. Potentially breakage of MVCC > > > > The another issue Tom pointed is potentially breakage of MVCC by > > using these reg* types. Syscache is invalidated on updates so > > this doesn't seem to be a problem on READ COMMITTED mode, but > > breaks SERIALIZABLE mode. But IMHO it is not so serious problem > > as long as such inconsistency occurs only on system catalog and > > it is explicitly documented togethee with the un-optimizability > > issue. I'll add a patch for this later. > > The way I look at it, all these arguments went out the window when > regclass was introduced. > > The reality is that reg* is *way* more convenient than manual > joins. The arguments about optimization and MVCC presumably apply to > all reg* casts, which means that ship has long since sailed. I agree basically, but I think some caveat is needed. I'll include that in the coming documentation patch. > If we offered views that made it easier to get at this data then maybe > this wouldn't matter so much. But DBA's don't want to join 3 catalogs > together to try and answer a simple question. It has been annoying me these days. regards, -- Kyotaro Horiguchi NTT Open Source Software Center
Hello, this is the patchset v2 of this feature. 0001-Add-regrole.patch Adding regrole as the name says. 0002-Add-regnamespace.patch Adding regnamespace. This depends on 0001 patch. 0003-Check-new-reg-types-are-not-used-as-default-values.patch Inhibiting the new OID aliss types from being used as constantsin default values, which make dependencies on the other (existing) OID alias types. 0004-Documentation-for-new-OID-alias-types.patch Documentation patch for this new types. regards, At Tue, 17 Feb 2015 20:19:11 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20150217.201911.239516619.horiguchi.kyotaro@lab.ntt.co.jp> > Hello, thank you for the comment. > > The current patch lacks change in documentation and dependency > stuff. Current framework doesn't consider changing pg_shdepend > from column default expressions so the possible measures are the > followings, I think. > > 1. Make pg_shdepend to have refobjsubid and add some deptypes of > pg_depend, specifically DEPENDENCY_NORMAL is needed. Then, > change the dependency stuff. > > 2. Simplly inhibit specifying them in default > expressions. Integer or OID can act as the replacement. > > =# create table t1 (a int, b regrole default 'horiguti'::regrole); > ERROR: Type 'regrole' cannot have a default value > > 1 is quite overkill but hardly seems to be usable. So I will go > on 2 and post additional patch. > > > At Thu, 12 Feb 2015 17:12:24 -0600, Jim Nasby <Jim.Nasby@BlueTreble.com> wrote in <54DD3358.9030601@BlueTreble.com> > > On 2/12/15 5:28 AM, Kyotaro HORIGUCHI wrote: > > > Hello, I changed the subject. > > # Ouch! the subject remaines wrong.. > > > > 1. Expected frequency of use > > ... > > > For that reason, although the current policy of deciding whether > > > to have reg* types seems to be whether they have schema-qualified > > > names, I think regrole and regnamespace are valuable to have. > > > > Perhaps schema qualification was the original intent, but I think at > > this point everyone uses them for convenience. Why would I want to > > type out JOIN pg_namespace n ON n.oid = blah.???namespace when I could > > simply do ???namespace::regnamespace? > > Yes, that is the most important point of this patch. > > > > 2. Anticipaed un-optimizability > > > > > > Tom pointed that these reg* types prevents planner from > > > optimizing the query, so we should refrain from having such > > > machinary. It should have been a long-standing issue but reg*s > > > sees to be rather faster than joining corresponding catalogs > > > for moderate number of the result rows, but this raises another > > > more annoying issue. > > > > > > > > > 3. Potentially breakage of MVCC > > > > > > The another issue Tom pointed is potentially breakage of MVCC by > > > using these reg* types. Syscache is invalidated on updates so > > > this doesn't seem to be a problem on READ COMMITTED mode, but > > > breaks SERIALIZABLE mode. But IMHO it is not so serious problem > > > as long as such inconsistency occurs only on system catalog and > > > it is explicitly documented togethee with the un-optimizability > > > issue. I'll add a patch for this later. > > > > The way I look at it, all these arguments went out the window when > > regclass was introduced. > > > > The reality is that reg* is *way* more convenient than manual > > joins. The arguments about optimization and MVCC presumably apply to > > all reg* casts, which means that ship has long since sailed. > > I agree basically, but I think some caveat is needed. I'll > include that in the coming documentation patch. > > > If we offered views that made it easier to get at this data then maybe > > this wouldn't matter so much. But DBA's don't want to join 3 catalogs > > together to try and answer a simple question. > > It has been annoying me these days. -- Kyotaro Horiguchi NTT Open Source Software Center
Sorry, I sent the previous mail without patches by accident. The patches are attached to this mail. ==== Hello, this is the patchset v2 of this feature. 0001-Add-regrole.patch Adding regrole as the name says. 0002-Add-regnamespace.patch Adding regnamespace. This depends on 0001 patch. 0003-Check-new-reg-types-are-not-used-as-default-values.patch Inhibiting the new OID aliss types from being used as constantsin default values, which make dependencies on the other (existing) OID alias types. 0004-Documentation-for-new-OID-alias-types.patch Documentation patch for this new types. regards, At Tue, 17 Feb 2015 20:19:11 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20150217.201911.239516619.horiguchi.kyotaro@lab.ntt.co.jp> > Hello, thank you for the comment. > > The current patch lacks change in documentation and dependency > stuff. Current framework doesn't consider changing pg_shdepend > from column default expressions so the possible measures are the > followings, I think. > > 1. Make pg_shdepend to have refobjsubid and add some deptypes of > pg_depend, specifically DEPENDENCY_NORMAL is needed. Then, > change the dependency stuff. > > 2. Simplly inhibit specifying them in default > expressions. Integer or OID can act as the replacement. > > =# create table t1 (a int, b regrole default 'horiguti'::regrole); > ERROR: Type 'regrole' cannot have a default value > > 1 is quite overkill but hardly seems to be usable. So I will go > on 2 and post additional patch. > > > At Thu, 12 Feb 2015 17:12:24 -0600, Jim Nasby <Jim.Nasby@BlueTreble.com> wrote in <54DD3358.9030601@BlueTreble.com> > > On 2/12/15 5:28 AM, Kyotaro HORIGUCHI wrote: > > > Hello, I changed the subject. > > # Ouch! the subject remaines wrong.. > > > > 1. Expected frequency of use > > ... > > > For that reason, although the current policy of deciding whether > > > to have reg* types seems to be whether they have schema-qualified > > > names, I think regrole and regnamespace are valuable to have. > > > > Perhaps schema qualification was the original intent, but I think at > > this point everyone uses them for convenience. Why would I want to > > type out JOIN pg_namespace n ON n.oid = blah.???namespace when I could > > simply do ???namespace::regnamespace? > > Yes, that is the most important point of this patch. > > > > 2. Anticipaed un-optimizability > > > > > > Tom pointed that these reg* types prevents planner from > > > optimizing the query, so we should refrain from having such > > > machinary. It should have been a long-standing issue but reg*s > > > sees to be rather faster than joining corresponding catalogs > > > for moderate number of the result rows, but this raises another > > > more annoying issue. > > > > > > > > > 3. Potentially breakage of MVCC > > > > > > The another issue Tom pointed is potentially breakage of MVCC by > > > using these reg* types. Syscache is invalidated on updates so > > > this doesn't seem to be a problem on READ COMMITTED mode, but > > > breaks SERIALIZABLE mode. But IMHO it is not so serious problem > > > as long as such inconsistency occurs only on system catalog and > > > it is explicitly documented togethee with the un-optimizability > > > issue. I'll add a patch for this later. > > > > The way I look at it, all these arguments went out the window when > > regclass was introduced. > > > > The reality is that reg* is *way* more convenient than manual > > joins. The arguments about optimization and MVCC presumably apply to > > all reg* casts, which means that ship has long since sailed. > > I agree basically, but I think some caveat is needed. I'll > include that in the coming documentation patch. > > > If we offered views that made it easier to get at this data then maybe > > this wouldn't matter so much. But DBA's don't want to join 3 catalogs > > together to try and answer a simple question. > > It has been annoying me these days. -- Kyotaro Horiguchi NTT Open Source Software Center
On 2/18/15 3:44 AM, Kyotaro HORIGUCHI wrote: > Sorry, I sent the previous mail without patches by accident. The > patches are attached to this mail. > > ==== > Hello, this is the patchset v2 of this feature. > > 0001-Add-regrole.patch > Adding regrole as the name says. > > 0002-Add-regnamespace.patch > Adding regnamespace. This depends on 0001 patch. > > 0003-Check-new-reg-types-are-not-used-as-default-values.patch > Inhibiting the new OID aliss types from being used as constants > in default values, which make dependencies on the other > (existing) OID alias types. > > 0004-Documentation-for-new-OID-alias-types.patch > Documentation patch for this new types. Somehow, these patches ended up in the commit fest without an author listed. That should probably not be possible.
Hello, At Thu, 19 Feb 2015 15:30:53 -0500, Peter Eisentraut <peter_e@gmx.net> wrote in <54E647FD.5000208@gmx.net> > On 2/18/15 3:44 AM, Kyotaro HORIGUCHI wrote: > > Hello, this is the patchset v2 of this feature. > > > > 0001-Add-regrole.patch > > 0002-Add-regnamespace.patch > > 0003-Check-new-reg-types-are-not-used-as-default-values.patch > > 0004-Documentation-for-new-OID-alias-types.patch > > Somehow, these patches ended up in the commit fest without an author > listed. That should probably not be possible. Mmm.. I saw the author for this is listed here, https://commitfest.postgresql.org/4/ Did you fix that manually for me? -- Kyotaro Horiguchi NTT Open Source Software Center
On Tue, Feb 24, 2015 at 11:35 AM, Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
-- Hello,
At Thu, 19 Feb 2015 15:30:53 -0500, Peter Eisentraut <peter_e@gmx.net> wrote in <54E647FD.5000208@gmx.net>
> On 2/18/15 3:44 AM, Kyotaro HORIGUCHI wrote:
> > Hello, this is the patchset v2 of this feature.
> >
> > 0001-Add-regrole.patch
> > 0002-Add-regnamespace.patch
> > 0003-Check-new-reg-types-are-not-used-as-default-values.patch
> > 0004-Documentation-for-new-OID-alias-types.patch
>
> Somehow, these patches ended up in the commit fest without an author
> listed. That should probably not be possible.
Mmm.. I saw the author for this is listed here,
https://commitfest.postgresql.org/4/
Did you fix that manually for me?
Looking at the log entry:
2015-02-19 21:11:08 Michael Paquier (michael-kun) Changed authors to Kyotaro Horiguchi (horiguti)
2015-02-19 21:11:08 Michael Paquier (michael-kun) Changed authors to Kyotaro Horiguchi (horiguti)
I do a pass from time to on the patches...
Michael
At Tue, 24 Feb 2015 14:11:28 +0900, Michael Paquier <michael.paquier@gmail.com> wrote in <CAB7nPqT6Ox3MV++hgmbd3YDU_5-1y5HCDDmsTK+QDYA_MjpFVg@mail.gmail.com> > > https://commitfest.postgresql.org/4/ > > > > Did you fix that manually for me? > > > > Looking at the log entry: > 2015-02-19 21:11:08 Michael Paquier (michael-kun) Changed authors to > Kyotaro Horiguchi (horiguti) > I do a pass from time to on the patches... Ah, I found it. Thank you. -- Kyotaro Horiguchi NTT Open Source Software Center
<div dir="ltr">Hi,<br /><br />Personally, I was looking for something like this as I need to see rolename<br />and namespacename many times in my queries rather than it's oid.<br />But making a JOIN expression every-time was a pain. Thiscertainly makes it<br />easier. And I see most DBAs are looking for it.<br /><br />I agree on Tom's concern on MVCC issue,but we already hit that when we<br />introduced regclass and others. So I see no additional issue with these<br />changesas such. About planner slowness, I guess updated documentation looks<br />perfect for that.<br /><br />So I wentahead reviewing these patches.<br /><br />All patches are straight forward and since we have similar code already<br/>exists, I did not find any issue at code level. They are consistent with other<br />functions.<br /><br />Patchesapplies with patch -p1. make, make install, make check has<br />no issues. Testing was fine too.<br /><br />Howeverhere are few review comments on the patches attached:<br /><br />Review points on 0001-Add-regrole.patch<br />---<br/>1.<br /><span style="font-family:monospace,monospace">+#include "utils/acl.h" <br /></span><br />Can you pleaseadd it at appropriate place as #include list is an ordered list<br /><br />2.<br /><span style="font-family:monospace,monospace">+ char *role_or_oid = PG_GETARG_CSTRING(0);<br /></span><br />Can we havevariable named as role_name_or_oid, like other similar functions?<br /><br />3.<br /><span style="font-family:monospace,monospace">+ /* <br />+ * Normal case: parse the name into components and see if it matchesany <br />+ * pg_role entries in the current search path. <br />+ */<br /></span><br />I believe, roles arenever searched per search path. Thus need to update<br />comments here.<br /><br /><br />Review points on 0002-Add-regnamespace.patch<br/>---<br />1.<br /><span style="font-family:monospace,monospace">+ * regnamespacein - converts "classname" to class OID <br /></span><br />Typos. Should be nspname instead of classname and namespase OIDinstead of<br />class OID<br /><br /><br />Review points on 0003-Check-new....patch<br />---<br />1.<br /><span style="font-family:monospace,monospace">@@-1860,6 +1860,7 @@ StoreAttrDefault(Relation rel, AttrNumber attnum, <br /> Oid attrdefOid; <br /> ObjectAddress colobject, <br /> defobject; <br />+ Oid exprtype; <br /></span><br />This seems unrelated. Please remove.<br /><br /><br />Apart from this, it willbe good if you have ONLY two patches,<br />(1) For regrole and (2) For regnamespace specific<br />With all related changesinto one patch. I mean, all role related changes i.e.<br />0001 + 0003 role related changes + 0004 role related changesand docs update<br />AND<br />0002 + 0003 nsp related changes + 0004 nsp related changes<br /><br />Thanks<br /><br/><div class="gmail_extra">-- <br /><div class="gmail_signature"><div dir="ltr">Jeevan B Chalke<br />Principal SoftwareEngineer, Product Development<br />EnterpriseDB Corporation<br />The Enterprise PostgreSQL Company<br /><br /></div></div></div></div>
Reviewed posted directly on mail thread instead of posting it on commitfest app.
Hello, thank you for reviewing. The attatched are the third version of this patch. 0001-Add-regrole_v3.patch 0002-Add-regnamespace_v3.patch - Rearranged into regrole patch and regnamespace patch as seen above, each of them consists of changes for code, docs, regtests.regnamespace patch depends on the another patch. - Removed the irrelevant change and corrected mistakes in comments. - Renamed role_or_oid to role_name_or_oid in regrolein(). - Changed the example name for regrole in the docmentation to 'smithee' as an impossible-in-reality-but-well-known name,from 'john', the most common in US (according to Wikipedia). At Tue, 24 Feb 2015 16:30:44 +0530, Jeevan Chalke <jeevan.chalke@enterprisedb.com> wrote in <CAM2+6=V-fwQFkwF0Pi3Dacq-ZRY5WxcPLVLuqFKf2Mf57_6inA@mail.gmail.com> > I agree on Tom's concern on MVCC issue, but we already hit that when we > introduced regclass and others. So I see no additional issue with these > changes as such. About planner slowness, I guess updated documentation looks > perfect for that. > > So I went ahead reviewing these patches. > > All patches are straight forward and since we have similar code already > exists, I did not find any issue at code level. They are consistent with > other > functions. One concern is about arbitrary allocation of OIDs for the new objects - types, functions. They are isolated from other similar reg* types, but there's no help for it without global renumbering of static(system) OIDs. > Patches applies with patch -p1. make, make install, make check has > no issues. Testing was fine too. > > However here are few review comments on the patches attached: > > Review points on 0001-Add-regrole.patch > --- > 1. > +#include "utils/acl.h" > > Can you please add it at appropriate place as #include list is an ordered > list regrolein calls reg_role_oid in acl.c, which is declared in acl.h. > 2. > + char *role_or_oid = PG_GETARG_CSTRING(0); > > Can we have variable named as role_name_or_oid, like other similar > functions? I might somehow have thought it a bit long. Fixed. > 3. > + /* > + * Normal case: parse the name into components and see if it matches > any > + * pg_role entries in the current search path. > + */ > > I believe, roles are never searched per search path. Thus need to update > comments here. Ouch. I forgot to edit it properly. I edit it. The correct catalog name is pg_authid. + /* Normal case: see if the name matches any pg_authid entry. */ I also edited comments for regnamespacein. > Review points on 0002-Add-regnamespace.patch > --- > 1. > + * regnamespacein - converts "classname" to class OID > > Typos. Should be nspname instead of classname and namespase OID instead of > class OID Thank you for pointing out. I fixed the same mistake in regrolein, p_ts_dict was there instead of pg_authid.. The names of oid kinds appear in multiple forms, that is, oprname and opr_name. Although I don't understand the reason, I followed the convention. > Review points on 0003-Check-new....patch > --- > 1. > @@ -1860,6 +1860,7 @@ StoreAttrDefault(Relation rel, AttrNumber attnum, > Oid attrdefOid; > ObjectAddress colobject, > defobject; > + Oid exprtype; > > This seems unrelated. Please remove. It's a trace of the previous code to ruling out the new oid types. Removed. > Apart from this, it will be good if you have ONLY two patches, > (1) For regrole and (2) For regnamespace specific > With all related changes into one patch. I mean, all role related changes > i.e. > 0001 + 0003 role related changes + 0004 role related changes and docs update > AND > 0002 + 0003 nsp related changes + 0004 nsp related changes I prudently separated it since I wasn't confident on the pertinence of ruling out. I rearranged them as your advise including docs. regards, -- Kyotaro Horiguchi NTT Open Source Software Center
Sorry, I fixed a silly typo in documentation in the previous version. - of theses types has a significance... + of these types has a significance... # My fingers frequently slip as above.. I incremented the version of this revised patch to get rid of confusion. ======= Hello, thank you for reviewing. The attatched are the fourth version of this patch. 0001-Add-regrole_v4.patch 0002-Add-regnamespace_v4.patch - Rearranged into regrole patch and regnamespace patch as seen above, each of them consists of changes for code, docs, regtests.regnamespace patch depends on the another patch. - Removed the irrelevant change and corrected mistakes in comments. - Renamed role_or_oid to role_name_or_oid in regrolein(). - Changed the example name for regrole in the docmentation to 'smithee' as an impossible-in-reality-but-well-known name,from 'john', the most common in US (according to Wikipedia). At Tue, 24 Feb 2015 16:30:44 +0530, Jeevan Chalke <jeevan.chalke@enterprisedb.com> wrote in <CAM2+6=V-fwQFkwF0Pi3Dacq-ZRY5WxcPLVLuqFKf2Mf57_6inA@mail.gmail.com> > I agree on Tom's concern on MVCC issue, but we already hit that when we > introduced regclass and others. So I see no additional issue with these > changes as such. About planner slowness, I guess updated documentation looks > perfect for that. > > So I went ahead reviewing these patches. > > All patches are straight forward and since we have similar code already > exists, I did not find any issue at code level. They are consistent with > other > functions. One concern is about arbitrary allocation of OIDs for the new objects - types, functions. They are isolated from other similar reg* types, but there's no help for it without global renumbering of static(system) OIDs. > Patches applies with patch -p1. make, make install, make check has > no issues. Testing was fine too. > > However here are few review comments on the patches attached: > > Review points on 0001-Add-regrole.patch > --- > 1. > +#include "utils/acl.h" > > Can you please add it at appropriate place as #include list is an ordered > list regrolein calls reg_role_oid in acl.c, which is declared in acl.h. > 2. > + char *role_or_oid = PG_GETARG_CSTRING(0); > > Can we have variable named as role_name_or_oid, like other similar > functions? I might somehow have thought it a bit long. Fixed. > 3. > + /* > + * Normal case: parse the name into components and see if it matches > any > + * pg_role entries in the current search path. > + */ > > I believe, roles are never searched per search path. Thus need to update > comments here. Ouch. I forgot to edit it properly. I edit it. The correct catalog name is pg_authid. + /* Normal case: see if the name matches any pg_authid entry. */ I also edited comments for regnamespacein. > Review points on 0002-Add-regnamespace.patch > --- > 1. > + * regnamespacein - converts "classname" to class OID > > Typos. Should be nspname instead of classname and namespase OID instead of > class OID Thank you for pointing out. I fixed the same mistake in regrolein, p_ts_dict was there instead of pg_authid.. The names of oid kinds appear in multiple forms, that is, oprname and opr_name. Although I don't understand the reason, I followed the convention. > Review points on 0003-Check-new....patch > --- > 1. > @@ -1860,6 +1860,7 @@ StoreAttrDefault(Relation rel, AttrNumber attnum, > Oid attrdefOid; > ObjectAddress colobject, > defobject; > + Oid exprtype; > > This seems unrelated. Please remove. It's a trace of the previous code to ruling out the new oid types. Removed. > Apart from this, it will be good if you have ONLY two patches, > (1) For regrole and (2) For regnamespace specific > With all related changes into one patch. I mean, all role related changes > i.e. > 0001 + 0003 role related changes + 0004 role related changes and docs update > AND > 0002 + 0003 nsp related changes + 0004 nsp related changes I prudently separated it since I wasn't confident on the pertinence of ruling out. I rearranged them as your advise including docs. regards, -- Kyotaro Horiguchi NTT Open Source Software Center
The attatched are the fourth version of this patch.
0001-Add-regrole_v4.patch
0002-Add-regnamespace_v4.patch
Seems like you have missed to attach both the patches.
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
On Tue, Feb 3, 2015 at 10:12 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Two reasons this isn't terribly compelling are (1) it's creating a > join in a place where the planner can't possibly see it and optimize > it, and (2) you risk MVCC anomalies because the reg* output routines > would not be using the same snapshot as the calling query. > > We already have problem (2) with the existing reg* functions so I'm > not that excited about doubling down on the concept. I think I agree. I mean, I agree that this notation is more convenient, but I don't really want to add a whole new slough of types --- these will certainly not be the only ones we want once we go down this path --- to the default install just for notational convenience. It's arguable, of course, but I guess I'm going to vote against this patch. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2015-03-02 16:42:35 -0500, Robert Haas wrote: > On Tue, Feb 3, 2015 at 10:12 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Two reasons this isn't terribly compelling are (1) it's creating a > > join in a place where the planner can't possibly see it and optimize > > it, and (2) you risk MVCC anomalies because the reg* output routines > > would not be using the same snapshot as the calling query. > > > > We already have problem (2) with the existing reg* functions so I'm > > not that excited about doubling down on the concept. > > I think I agree. I mean, I agree that this notation is more > convenient, but I don't really want to add a whole new slough of types > --- these will certainly not be the only ones we want once we go down > this path --- to the default install just for notational convenience. > It's arguable, of course, but I guess I'm going to vote against this > patch. That's a justifyable position. I don't think there are other catalogs referenced as pervasively in the catalog though. There's one additional point: Using reg* types in the catalog tables themselves can make them *much* easier to read. I personally do look at the catalogs a awful lot, and seing names instead of oids makes it much easier. And adding regtype/role would allow to cover nearly all types containing oids. Incidentally I've started working on a replacement for the bki DATA stuff (http://archives.postgresql.org/message-id/20150220234142.GH12653%40awork2.anarazel.de) and of the things that makes the biggest difference in editing based on my experience is not to have to list endless columns of oids that you have to figure out by hand. Replacing e.g. prorettype/proallargtypes oids by their names made it much, much easier to read. And my initial implementation simply supports that based on the column type... So adding regnamespace/regrole actually might help there. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 3/2/15 3:56 PM, Andres Freund wrote: > On 2015-03-02 16:42:35 -0500, Robert Haas wrote: >> >On Tue, Feb 3, 2015 at 10:12 AM, Tom Lane<tgl@sss.pgh.pa.us> wrote: >>> > >Two reasons this isn't terribly compelling are (1) it's creating a >>> > >join in a place where the planner can't possibly see it and optimize >>> > >it, and (2) you risk MVCC anomalies because the reg* output routines >>> > >would not be using the same snapshot as the calling query. >>> > > >>> > >We already have problem (2) with the existing reg* functions so I'm >>> > >not that excited about doubling down on the concept. >> > >> >I think I agree. I mean, I agree that this notation is more >> >convenient, but I don't really want to add a whole new slough of types >> >--- these will certainly not be the only ones we want once we go down >> >this path --- to the default install just for notational convenience. >> >It's arguable, of course, but I guess I'm going to vote against this >> >patch. > That's a justifyable position. I don't think there are other catalogs > referenced as pervasively in the catalog though. > > There's one additional point: Using reg* types in the catalog tables > themselves can make them*much* easier to read. I personally do look at > the catalogs a awful lot, and seing names instead of oids makes it much > easier. And adding regtype/role would allow to cover nearly all types > containing oids. +1. Constantly joining catalog tables together is a royal PITA, and regnamespace is the biggest missing part of this (I typically don't look at roles too much, but I can certainly see it's importance). If we had more user friendly views on the catalogs maybe this wouldn't be an issue... but that's a much larger project. BTW, I think the potential for MVCC issues should be mentioned in the docs (http://www.postgresql.org/docs/devel/static/datatype-oid.html). -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com
Hello, I attached the latest patches missing in the previous mail. Thanks for pointing Jeevan. 0001-Add-regrole_v4.patch 0002-Add-regnamespace_v4.patch Jim> BTW, I think the potential for MVCC issues should be mentioned in the Jim> docs (http://www.postgresql.org/docs/devel/static/datatype-oid.html). The first patch of the aboves contains doc patch which adds the following note to html/datatype-oid.html. Does it make sense? > Note: The OID alias types don't sctrictly comply the transaction > isolation rules so do not use them where exact transaction > isolation on the values of these types has a > significance. Likewise, since they look as simple constants to > planner so you might get slower plans than the queries joining > the system tables correnspond to the OID types. regards, At Mon, 2 Mar 2015 17:50:37 -0600, Jim Nasby <Jim.Nasby@BlueTreble.com> wrote in <54F4F74D.8000003@BlueTreble.com> > On 3/2/15 3:56 PM, Andres Freund wrote: > > On 2015-03-02 16:42:35 -0500, Robert Haas wrote: > >> >On Tue, Feb 3, 2015 at 10:12 AM, Tom Lane<tgl@sss.pgh.pa.us> wrote: > >>> > >Two reasons this isn't terribly compelling are (1) it's creating a > >>> > >join in a place where the planner can't possibly see it and optimize > >>> > >it, and (2) you risk MVCC anomalies because the reg* output routines > >>> > >would not be using the same snapshot as the calling query. > >>> > > > >>> > >We already have problem (2) with the existing reg* functions so I'm > >>> > >not that excited about doubling down on the concept. > >> > > >> >I think I agree. I mean, I agree that this notation is more > >> >convenient, but I don't really want to add a whole new slough of types > >> >--- these will certainly not be the only ones we want once we go down > >> >this path --- to the default install just for notational convenience. > >> >It's arguable, of course, but I guess I'm going to vote against this > >> >patch. > > That's a justifyable position. I don't think there are other catalogs > > referenced as pervasively in the catalog though. > > > > There's one additional point: Using reg* types in the catalog tables > > themselves can make them*much* easier to read. I personally do look at > > the catalogs a awful lot, and seing names instead of oids makes it > > much > > easier. And adding regtype/role would allow to cover nearly all types > > containing oids. > > +1. Constantly joining catalog tables together is a royal PITA, and > regnamespace is the biggest missing part of this (I typically don't > look at roles too much, but I can certainly see it's importance). > > If we had more user friendly views on the catalogs maybe this wouldn't > be an issue... but that's a much larger project. > > BTW, I think the potential for MVCC issues should be mentioned in the > docs (http://www.postgresql.org/docs/devel/static/datatype-oid.html). -- Kyotaro Horiguchi NTT Open Source Software Center
On 3/3/15 8:04 PM, Kyotaro HORIGUCHI wrote: >> >Note: The OID alias types don't sctrictly comply the transaction >> > isolation rules so do not use them where exact transaction >> > isolation on the values of these types has a >> > significance. Likewise, since they look as simple constants to >> > planner so you might get slower plans than the queries joining >> > the system tables correnspond to the OID types. Might I suggest: Note: The OID alias types do not completely follow transaction isolation rules. The planner also treats them as simple constants, which may result in sub-optimal planning. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation: tested, passed > 1. > +#include "utils/acl.h" > > Can you please add it at appropriate place as #include list is an ordered >list > regrolein calls reg_role_oid in acl.c, which is declared in acl.h. Well. I was suggesting that putting #include "utils/acl.h" line after #include "parser/parse_type.h" and before #include "utils/builtins.h" so that they will be in order. I understand that it is needed for reg_role_oid() call. Review comments on *_v4 patches: 1. + The OID alias types don't sctrictly comply the transaction isolation Typo. sctrictly => strictly 2. + joining the system tables correnspond to the OID types. Typo. correnspond => correspond Apart from these typos, I see no issues. However, you can have a look over Jim's suggestion on doc wordings. If you feel comfortable, I have no issues if you mark this as "Ready for Committer" once you fix the typos and doc wordings.
Thank you for the correction. At Wed, 4 Mar 2015 01:01:48 -0600, Jim Nasby <Jim.Nasby@BlueTreble.com> wrote in <54F6ADDC.8030201@BlueTreble.com> > On 3/3/15 8:04 PM, Kyotaro HORIGUCHI wrote: > >> >Note: The OID alias types don't sctrictly comply the transaction > >> > isolation rules so do not use them where exact transaction > >> > isolation on the values of these types has a > >> > significance. Likewise, since they look as simple constants to > >> > planner so you might get slower plans than the queries joining > >> > the system tables correnspond to the OID types. > > Might I suggest: > > Note: The OID alias types do not completely follow transaction > isolation rules. The planner also treats them as simple constants, > which may result in sub-optimal planning. Looks far simple and enough. The note has been replaced with your sentence in the attached patch. regards, -- Kyotaro Horiguchi NTT Open Source Software Center
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation: tested, passed Looks good. Passing it to committer. The new status of this patch is: Ready for Committer
Hello, Thank you for reviewing, and sorry to have overlooked this. # I hope the CF app to add the author as a receiver when issueing # a mail. regards, At Thu, 12 Mar 2015 11:06:29 +0000, Jeevan Chalke <jeevan.chalke@gmail.com> wrote in <20150312110629.2540.70807.pgcf@coridan.postgresql.org> > The following review has been posted through the commitfest application: > make installcheck-world: tested, passed > Implements feature: tested, passed > Spec compliant: tested, passed > Documentation: tested, passed > > Looks good. Passing it to committer. > > The new status of this patch is: Ready for Committer -- Kyotaro Horiguchi NTT Open Source Software Center
Kyotaro HORIGUCHI wrote: > # I hope the CF app to add the author as a receiver when issueing > # a mail. Moreover, it should add everyone who was in To, From, CC in the email that the commitfest app is replying to; if the patch authors are not listed, add them as well. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
At Thu, 19 Mar 2015 11:17:34 -0300, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in <20150319141734.GE3636@alvh.no-ip.org> > Kyotaro HORIGUCHI wrote: > > > # I hope the CF app to add the author as a receiver when issueing > > # a mail. > > Moreover, it should add everyone who was in To, From, CC in the email > that the commitfest app is replying to; if the patch authors are not > listed, add them as well. It should do what ordinary mailers do for recipients list, as it already does for References: and In-reply-to:. -- Kyotaro Horiguchi NTT Open Source Software Center
On 03/10/2015 04:42 AM, Kyotaro HORIGUCHI wrote: > Thank you for the correction. > > At Wed, 4 Mar 2015 01:01:48 -0600, Jim Nasby <Jim.Nasby@BlueTreble.com> wrote in <54F6ADDC.8030201@BlueTreble.com> >> On 3/3/15 8:04 PM, Kyotaro HORIGUCHI wrote: >>>>> Note: The OID alias types don't sctrictly comply the transaction >>>>> isolation rules so do not use them where exact transaction >>>>> isolation on the values of these types has a >>>>> significance. Likewise, since they look as simple constants to >>>>> planner so you might get slower plans than the queries joining >>>>> the system tables correnspond to the OID types. >> Might I suggest: >> >> Note: The OID alias types do not completely follow transaction >> isolation rules. The planner also treats them as simple constants, >> which may result in sub-optimal planning. > Looks far simple and enough. > The note has been replaced with your sentence in the attached patch. > > I have just claimed this as committer in the CF, but on reviewing the emails it looks like there is disagreement about the need for it at all, especially from Tom and Robert. I confess I have often wanted regnamespace, particularly, and occasionally regrole, simply as a convenience. But I'm not going to commit it against substantial opposition. Do we need a vote? cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> I have just claimed this as committer in the CF, but on reviewing the
> emails it looks like there is disagreement about the need for it at all,
> especially from Tom and Robert.
> I confess I have often wanted regnamespace, particularly, and
> occasionally regrole, simply as a convenience. But I'm not going to
> commit it against substantial opposition.
> Do we need a vote?
My concern about it is basically that I don't see where we stop.
The existing regFOO alias types are provided for object classes which
have nontrivial naming conventions (schema qualification, overloaded
argument types, etc), so that you can't just do "select ... from
catalog where objectname = 'blah'". That doesn't apply to namespaces
or roles. So I'm afraid that once this precedent is established,
there will be demands for regFOO for every object class we have,
and I don't want that much clutter.
It may be that these two cases are so much more useful than any other
conceivable cases that we can do them and stop, but I don't think that
argument has been made convincingly.
regards, tom lane
On 3/29/15 1:55 PM, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> I have just claimed this as committer in the CF, but on reviewing the >> emails it looks like there is disagreement about the need for it at all, >> especially from Tom and Robert. > >> I confess I have often wanted regnamespace, particularly, and >> occasionally regrole, simply as a convenience. But I'm not going to >> commit it against substantial opposition. > >> Do we need a vote? > > My concern about it is basically that I don't see where we stop. > The existing regFOO alias types are provided for object classes which > have nontrivial naming conventions (schema qualification, overloaded > argument types, etc), so that you can't just do "select ... from > catalog where objectname = 'blah'". That doesn't apply to namespaces > or roles. So I'm afraid that once this precedent is established, > there will be demands for regFOO for every object class we have, > and I don't want that much clutter. IMHO the real issue here is it's just a royal PITA to query the catalogs in so many cases, especially on an ad-hoc basis. information_schema helps in many cases, but it sometimes doesn't have PG-specific stuff that you need. And if you're writing code you can't depend on it actually being there. (I've also heard it's horribly slow...) I don't see a good way to really solve that other than reviving pg_sysview and pulling that in. BTW, it would arguably be better if we just exposed the function that deals with quoting and schemas; IIRC it doesn't actually need the catalog. > It may be that these two cases are so much more useful than any other > conceivable cases that we can do them and stop, but I don't think that > argument has been made convincingly. Short of fixing the underlying problem I think regnamspace would absolutely be worth it. I find myself wanting that all the time. I generally don't care about roles too much, but maybe that's just me. FWIW, (and off-topic...) the other thing I find very painful is dealing with ACLs. has_*_privilege does what it does well, but if you want to do anything else (such as ensure permissions on an object are specified as you expect) you're stuck resorting to things like NOT EXISTS( SELECT has_*_privilege() FROM pg_roles WHERE rolname NOT IN() ) which is awkward because you actually need one of those for every permission you want to check. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com
On 03/29/2015 02:55 PM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> I have just claimed this as committer in the CF, but on reviewing the
>> emails it looks like there is disagreement about the need for it at all,
>> especially from Tom and Robert.
>> I confess I have often wanted regnamespace, particularly, and
>> occasionally regrole, simply as a convenience. But I'm not going to
>> commit it against substantial opposition.
>> Do we need a vote?
> My concern about it is basically that I don't see where we stop.
> The existing regFOO alias types are provided for object classes which
> have nontrivial naming conventions (schema qualification, overloaded
> argument types, etc), so that you can't just do "select ... from
> catalog where objectname = 'blah'". That doesn't apply to namespaces
> or roles. So I'm afraid that once this precedent is established,
> there will be demands for regFOO for every object class we have,
> and I don't want that much clutter.
>
> It may be that these two cases are so much more useful than any other
> conceivable cases that we can do them and stop, but I don't think that
> argument has been made convincingly.
Well, here's a list of all the fooname attributes in the catalog, which
I guess are the prime candidates for regfoo pseudotypes. Besides those
we already have and the two proposed here, I'm not sure there will be
huge demand for others - tablespace maybe, trigger doesn't seem very
practicable, and I could just see suggestions for collation and
conversion, but those seem pretty marginal, and that seems to be about
it, to me.
attrelid | attname
--------------------------------------------+-------------------------- pg_proc |
proname pg_type | typname pg_attribute |
attname pg_class | relname pg_constraint |
conname pg_operator | oprname pg_opfamily |
opfname pg_opclass | opcname pg_am | amname
pg_language | lanname pg_rewrite | rulename
pg_trigger | tgname pg_event_trigger | evtname
pg_namespace | nspname pg_conversion | conname
pg_database | datname pg_tablespace | spcname
pg_pltemplate | tmplname pg_authid | rolname
pg_ts_config | cfgname pg_ts_dict | dictname
pg_ts_parser | prsname pg_ts_template | tmplname
pg_extension | extname pg_foreign_data_wrapper | fdwname
pg_foreign_server | srvname pg_policy | polname
pg_collation | collname
cheers
andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> On 03/29/2015 02:55 PM, Tom Lane wrote:
>> It may be that these two cases are so much more useful than any other
>> conceivable cases that we can do them and stop, but I don't think that
>> argument has been made convincingly.
> Well, here's a list of all the fooname attributes in the catalog, which
> I guess are the prime candidates for regfoo pseudotypes. Besides those
> we already have and the two proposed here, I'm not sure there will be
> huge demand for others - tablespace maybe, trigger doesn't seem very
> practicable, and I could just see suggestions for collation and
> conversion, but those seem pretty marginal, and that seems to be about
> it, to me.
Hmm. We can ignore pg_attribute and pg_pltemplate, which don't have OIDs
and thus aren't candidates anyway. And we can ignore the ones
corresponding to the already-existing regFOO types. That leaves
> pg_am | amname
> pg_authid | rolname (*)
> pg_collation | collname
> pg_constraint | conname
> pg_conversion | conname
> pg_database | datname
> pg_event_trigger | evtname
> pg_extension | extname
> pg_foreign_data_wrapper | fdwname
> pg_foreign_server | srvname
> pg_language | lanname
> pg_namespace | nspname (*)
> pg_opclass | opcname
> pg_opfamily | opfname
> pg_policy | polname
> pg_rewrite | rulename
> pg_tablespace | spcname
> pg_trigger | tgname
> pg_ts_parser | prsname
> pg_ts_template | tmplname
of which the proposed patch covers the two starred ones.
OTOH, looking at this list, there are already numerous cases where
the object identity is more than just a name (eg, collations have
schema-qualified names, opfamilies are not only schema-qualified
but are per-index-AM as well, triggers and constraints are named
per-table, etc). So it's clear that we've already been applying
a "usefulness" criterion rather than just "does it have a
multi-component name" when choosing which objects to provide
regFOO types for.
In view of that, you could certainly argue that if someone's bothered
to make a patch to add a new regFOO type, it's useful enough. I don't
want to end up with thirtysomething of them, but we don't seem to be
trending in that direction.
Or in short, objection withdrawn. (As to the concept, anyway.
I've not read the patch...)
regards, tom lane
Hello, At Tue, 31 Mar 2015 16:48:18 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <26969.1427834898@sss.pgh.pa.us> > Hmm. We can ignore pg_attribute and pg_pltemplate, which don't have OIDs > and thus aren't candidates anyway. And we can ignore the ones > corresponding to the already-existing regFOO types. That leaves > > > pg_am | amname > > pg_authid | rolname (*) > > pg_collation | collname > > pg_constraint | conname > > pg_conversion | conname > > pg_database | datname > > pg_event_trigger | evtname > > pg_extension | extname > > pg_foreign_data_wrapper | fdwname > > pg_foreign_server | srvname > > pg_language | lanname > > pg_namespace | nspname (*) > > pg_opclass | opcname > > pg_opfamily | opfname > > pg_policy | polname > > pg_rewrite | rulename > > pg_tablespace | spcname > > pg_trigger | tgname > > pg_ts_parser | prsname > > pg_ts_template | tmplname > > of which the proposed patch covers the two starred ones. > > OTOH, looking at this list, there are already numerous cases where > the object identity is more than just a name (eg, collations have > schema-qualified names, opfamilies are not only schema-qualified > but are per-index-AM as well, triggers and constraints are named > per-table, etc). So it's clear that we've already been applying > a "usefulness" criterion rather than just "does it have a > multi-component name" when choosing which objects to provide > regFOO types for. As I wrote before, the criteria I selected for choosing these ones was how often the oid is referred to. The attached excel file shows the complehensive list of reference counts. Each cells is marked 'x' if the catalog of the row referrs to the oid of the catalog on the column. So the numbers in the row 2 represents how mane times the oid of the catalog on the column is referred to from other catalogs. Adding all catalog having tuple oid and sorting by the number they are ordered as below. (The upper cased 'X' in the HASOID column indicates that the viewexposes the oid of underlying table and identifying therows inthe view) (-) in the list below is the regFOO types already exists and the second column is the number of other catalogs refers to the oid. > pg_authid | 33 | rolname (*) + pg_class | 27 | relname (-) > pg_namespace | 20 | nspname (*) + pg_type | 15 | typname (-) + pg_proc | 13 | proname (-) + pg_operator | 5 | oprname (-) > pg_database | 5 | datname > pg_am | 4 | amname > pg_collation | 4 | collname > pg_tablespace | 4 | spcname > pg_foreign_server | 3 | srvname > pg_opfamily | 3 | opfname > pg_opclass | 2 | opcname > pg_constraint | 1 | conname > pg_foreign_data_wrapper | 1 | fdwname > pg_language | 1 | lanname + pg_largeobject_metadata | 1 | - > pg_policy | 1 | polname > pg_rewrite | 1 | rulename + pg_ts_config | 1 | cfgname (-) + pg_ts_dict | 1 | dictname (-) > pg_ts_parser | 1 | prsname > pg_ts_template | 1 | tmplname + pg_user_mapping | 1 | - + pg_aggregate | 0 | - All of amop, amproc, attrdef, cast, conversion, default_acl, enum, event_trigger, extension, group, roles, shadow, trigger, user are not referred to from any other catalogs. > In view of that, you could certainly argue that if someone's bothered > to make a patch to add a new regFOO type, it's useful enough. I don't > want to end up with thirtysomething of them, but we don't seem to be > trending in that direction. pg_authid and pg_namespace are obviously win the race but haven't got the prize. database to tablespace are in a gray zone but I think they need highly significant reason to have regFOO type for themselves. On the other hand, regconfig(pg_ts_config) and regdictionary(pg_ts_dist) have far less significance but I don't assert they should be removed since they are there now. > Or in short, objection withdrawn. (As to the concept, anyway. > I've not read the patch...) Thank you for your acceptance. regards, -- Kyotaro Horiguchi NTT Open Source Software Center
On Sat, Mar 28, 2015 at 6:59 PM, Andrew Dunstan <andrew@dunslane.net> wrote: > I have just claimed this as committer in the CF, but on reviewing the emails > it looks like there is disagreement about the need for it at all, especially > from Tom and Robert. > > I confess I have often wanted regnamespace, particularly, and occasionally > regrole, simply as a convenience. But I'm not going to commit it against > substantial opposition. > > Do we need a vote? Seeing this committed this wouldn't be my first choice, but I can live with it, as long as the patch is good technically. As useful as these sorts of types are, I'm not convinced that notational convenience for people steeped in backend internals is a sufficiently-good reason to clutter the system with more built-in types. But I'm probably in the minority on that; and it's clearly a judgement call anyway. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 03/31/2015 04:48 PM, Tom Lane wrote: > In view of that, you could certainly argue that if someone's bothered > to make a patch to add a new regFOO type, it's useful enough. I don't > want to end up with thirtysomething of them, but we don't seem to be > trending in that direction. > > Or in short, objection withdrawn. (As to the concept, anyway. > I've not read the patch...) > > The only possible issue I see on reading the patches is that these are treated differently for dependencies than other regFOO types. Rather than create a dependency if a value is used in a default expression, an error is raised if one is found. Are we OK with that? cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> The only possible issue I see on reading the patches is that these are
> treated differently for dependencies than other regFOO types. Rather
> than create a dependency if a value is used in a default expression, an
> error is raised if one is found. Are we OK with that?
Why would it be a good idea to act differently from the others?
regards, tom lane
On 04/01/2015 12:13 PM, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> The only possible issue I see on reading the patches is that these are >> treated differently for dependencies than other regFOO types. Rather >> than create a dependency if a value is used in a default expression, an >> error is raised if one is found. Are we OK with that? > Why would it be a good idea to act differently from the others? > > I have no idea. It was mentioned here <http://www.postgresql.org/message-id/20150218.174231.125293096.horiguchi.kyotaro@lab.ntt.co.jp> but nobody seems to have commented. I'm not sure why it was done like this. Adding the dependencies seems to be no harder than raising the exception. I think we can kick this back to the author to fix. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes: > On 04/01/2015 12:13 PM, Tom Lane wrote: >> Andrew Dunstan <andrew@dunslane.net> writes: >>> The only possible issue I see on reading the patches is that these are >>> treated differently for dependencies than other regFOO types. Rather >>> than create a dependency if a value is used in a default expression, an >>> error is raised if one is found. Are we OK with that? >> Why would it be a good idea to act differently from the others? > I have no idea. > It was mentioned here > <http://www.postgresql.org/message-id/20150218.174231.125293096.horiguchi.kyotaro@lab.ntt.co.jp> > but nobody seems to have commented. I'm not sure why it was done like > this. Adding the dependencies seems to be no harder than raising the > exception. I think we can kick this back to the author to fix. After a bit more thought it occurred to me that a dependency on a role would need to be a shared dependency, and the existing infrastructure for recordDependencyOnExpr() wouldn't support that. I'm not sure that it's worth adding the complexity to allow shared dependencies along with normal ones there. This might be a reason to reject the regrole part of the patch, as requiring more complexity than it's worth. But in any case I cannot see a reason to treat regnamespace differently from the existing types on this point. regards, tom lane
On 04/01/2015 12:53 PM, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> On 04/01/2015 12:13 PM, Tom Lane wrote: >>> Andrew Dunstan <andrew@dunslane.net> writes: >>>> The only possible issue I see on reading the patches is that these are >>>> treated differently for dependencies than other regFOO types. Rather >>>> than create a dependency if a value is used in a default expression, an >>>> error is raised if one is found. Are we OK with that? >>> Why would it be a good idea to act differently from the others? >> I have no idea. >> It was mentioned here >> <http://www.postgresql.org/message-id/20150218.174231.125293096.horiguchi.kyotaro@lab.ntt.co.jp> >> but nobody seems to have commented. I'm not sure why it was done like >> this. Adding the dependencies seems to be no harder than raising the >> exception. I think we can kick this back to the author to fix. > After a bit more thought it occurred to me that a dependency on a role > would need to be a shared dependency, and the existing infrastructure > for recordDependencyOnExpr() wouldn't support that. > > I'm not sure that it's worth adding the complexity to allow shared > dependencies along with normal ones there. This might be a reason > to reject the regrole part of the patch, as requiring more complexity > than it's worth. > > But in any case I cannot see a reason to treat regnamespace differently > from the existing types on this point. > > Good points. I agree re namespace. And I also agree that shared dependency support is not worth the trouble, especially not just to support regrole. I'm not sure that's a reason to reject regrole entirely, though. However, I also think there is a significantly less compelling case for it than for regnamespace, based on the number of times I have wanted each. Anybody else have thoughts on this? cheers andrew
Hello, sorry for the absence. I changed the regnamespace's
behavior as the same as the other reg* types. And I attached a
patch as separate one that fixes regroleout to do the same as the
other reg* types, because I have
0001-Add-regrole_v6.patch : fix regnamespace to behave as the same as the other reg* types.
0001-Make-regnamespace-behave-as-the-same-as-other-reg-ty.patch: Diff from v5 to v6, only for convinience.
0001-Fix-regroleout-s-behavior-following-other-out-functi.patch: Fixes regroleout so as to behave as the same as other
reg*types, but might be a bit too large.
At Wed, 01 Apr 2015 14:46:01 -0400, Andrew Dunstan <andrew@dunslane.net> wrote in <551C3CE9.4080503@dunslane.net>
> > But in any case I cannot see a reason to treat regnamespace
> > differently
> > from the existing types on this point.
> >
> >
>
> Good points.
>
> I agree re namespace. And I also agree that shared dependency support
> is not worth the trouble, especially not just to support regrole. I'm
> not sure that's a reason to reject regrole entirely, though. However,
> I also think there is a significantly less compelling case for it than
> for regnamespace, based on the number of times I have wanted each.
>
> Anybody else have thoughts on this?
Yeah, that's my thinko. regnamespace in the attached patch
behaves as the same to other reg* types. On the way fixing it, I
found a bug that regnamespaceout returns NULL for removed shema
name. I fixed it, too.
Addition to that, regroleout raises exception for invalid role
oids. This is unwanted behavior but GetUserNameFromId is needed
to have one extra parameter 'noerr' to fix this. This fix is
attached as another patch.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/doc/src/sgml/datatype.sgml b/doc/src/sgml/datatype.sgml
index da1f25f..6d1f02d 100644
--- a/doc/src/sgml/datatype.sgml
+++ b/doc/src/sgml/datatype.sgml
@@ -4321,7 +4321,8 @@ SET xmloption TO { DOCUMENT | CONTENT }; an object identifier. There are also several alias
typesfor <type>oid</>: <type>regproc</>, <type>regprocedure</>, <type>regoper</>, <type>regoperator</>,
<type>regclass</>,
- <type>regtype</>, <type>regconfig</>, and <type>regdictionary</>.
+ <type>regtype</>, <type>regrole</>, <type>regnamespace</>,
+ <type>regconfig</>, and <type>regdictionary</>. <xref linkend="datatype-oid-table"> shows an overview.
</para>
@@ -4431,6 +4432,20 @@ SELECT * FROM pg_attribute </row> <row>
+ <entry><type>regrole</></entry>
+ <entry><structname>pg_authid</></entry>
+ <entry>role name</entry>
+ <entry><literal>smithee</></entry>
+ </row>
+
+ <row>
+ <entry><type>regnamespace</></entry>
+ <entry><structname>pg_namespace</></entry>
+ <entry>namespace name</entry>
+ <entry><literal>pg_catalog</></entry>
+ </row>
+
+ <row> <entry><type>regconfig</></entry> <entry><structname>pg_ts_config</></entry>
<entry>textsearch configuration</entry>
@@ -4448,29 +4463,37 @@ SELECT * FROM pg_attribute </table> <para>
- All of the OID alias types accept schema-qualified names, and will
- display schema-qualified names on output if the object would not
- be found in the current search path without being qualified.
- The <type>regproc</> and <type>regoper</> alias types will only
- accept input names that are unique (not overloaded), so they are
- of limited use; for most uses <type>regprocedure</> or
+ All of the OID alias types for objects grouped by namespace accept
+ schema-qualified names, and will display schema-qualified names on output
+ if the object would not be found in the current search path without being
+ qualified. The <type>regproc</> and <type>regoper</> alias types will
+ only accept input names that are unique (not overloaded), so they are of
+ limited use; for most uses <type>regprocedure</> or <type>regoperator</> are more appropriate. For
<type>regoperator</>, unary operators are identified by writing <literal>NONE</> for the unused operand.
</para>
+ <para> An additional property of most of the OID alias types is the
+ creation of dependencies. If a constant of one of these types
+ appears in a stored expression (such as a column default
+ expression or view), it creates a dependency on the referenced
+ object. For example, if a column has a default expression
+ <literal>nextval('my_seq'::regclass)</>,
+ <productname>PostgreSQL</productname> understands that the default
+ expression depends on the sequence <literal>my_seq</>; the system
+ will not let the sequence be dropped without first removing the
+ default expression. <type>regrole</> is the only exception for the
+ property. Constants of this type is not allowed in such
+ expressions. </para>
+
+ <note> <para>
- An additional property of the OID alias types is the creation of
- dependencies. If a
- constant of one of these types appears in a stored expression
- (such as a column default expression or view), it creates a dependency
- on the referenced object. For example, if a column has a default
- expression <literal>nextval('my_seq'::regclass)</>,
- <productname>PostgreSQL</productname>
- understands that the default expression depends on the sequence
- <literal>my_seq</>; the system will not let the sequence be dropped
- without first removing the default expression.
+ The OID alias types do not completely follow transaction isolation
+ rules. The planner also treats them as simple constants, which may
+ result in sub-optimal planning. </para>
+ </note> <para> Another identifier type used by the system is <type>xid</>, or transaction
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index ad49964..f30d5e6 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -113,6 +113,10 @@ static const struct typinfo TypInfo[] = { F_REGPROCIN, F_REGPROCOUT}, {"regtype",
REGTYPEOID,0, 4, true, 'i', 'p', InvalidOid, F_REGTYPEIN, F_REGTYPEOUT},
+ {"regrole", REGROLEOID, 0, 4, true, 'i', 'p', InvalidOid,
+ F_REGROLEIN, F_REGROLEOUT},
+ {"regnamespace", REGNAMESPACEOID, 0, 4, true, 'i', 'p', InvalidOid,
+ F_REGNAMESPACEIN, F_REGNAMESPACEOUT}, {"text", TEXTOID, 0, -1, false, 'i', 'x', DEFAULT_COLLATION_OID,
F_TEXTIN,F_TEXTOUT}, {"oid", OIDOID, 0, 4, true, 'i', 'p', InvalidOid,
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index bacb242..858d52f 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -1597,6 +1597,24 @@ find_expr_references_walker(Node *node, add_object_address(OCLASS_TSDICT,
objoid,0, context->addrs); break;
+
+ case REGNAMESPACEOID:
+ objoid = DatumGetObjectId(con->constvalue);
+ if (SearchSysCacheExists1(NAMESPACEOID,
+ ObjectIdGetDatum(objoid)))
+ add_object_address(OCLASS_SCHEMA, objoid, 0,
+ context->addrs);
+ break;
+
+ /*
+ * Dependencies for regrole is shared among all databases, so
+ * explicitly inhibit to have dependencies.
+ */
+ case REGROLEOID:
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("constant of the type \'regrole\' cannot be used here")));
+ break; } } return false;
diff --git a/src/backend/utils/adt/regproc.c b/src/backend/utils/adt/regproc.c
index 11d663b..a41f577 100644
--- a/src/backend/utils/adt/regproc.c
+++ b/src/backend/utils/adt/regproc.c
@@ -40,6 +40,7 @@#include "utils/lsyscache.h"#include "utils/syscache.h"#include "utils/tqual.h"
+#include "utils/acl.h"static char *format_operator_internal(Oid operator_oid, bool force_qualify);static char
*format_procedure_internal(Oidprocedure_oid, bool force_qualify);
@@ -1553,6 +1554,199 @@ regdictionarysend(PG_FUNCTION_ARGS) return oidsend(fcinfo);}
+/*
+ * regrolein - converts "rolename" to role OID
+ *
+ * We also accept a numeric OID, for symmetry with the output routine.
+ *
+ * '-' signifies unknown (OID 0). In all other cases, the input must
+ * match an existing pg_authid entry.
+ *
+ * This function is not needed in bootstrap mode, so we don't worry about
+ * making it work then.
+ */
+Datum
+regrolein(PG_FUNCTION_ARGS)
+{
+ char *role_name_or_oid = PG_GETARG_CSTRING(0);
+ Oid result;
+
+ /* '-' ? */
+ if (strcmp(role_name_or_oid, "-") == 0)
+ PG_RETURN_OID(InvalidOid);
+
+ /* Numeric OID? */
+ if (role_name_or_oid[0] >= '0' &&
+ role_name_or_oid[0] <= '9' &&
+ strspn(role_name_or_oid, "0123456789") == strlen(role_name_or_oid))
+ {
+ result = DatumGetObjectId(DirectFunctionCall1(oidin,
+ CStringGetDatum(role_name_or_oid)));
+ PG_RETURN_OID(result);
+ }
+
+ /* Normal case: see if the name matches any pg_authid entry. */
+ result = get_role_oid(role_name_or_oid, false);
+
+ PG_RETURN_OID(result);
+}
+
+/*
+ * to_regrole - converts "rolename" to role OID
+ *
+ * If the name is not found, we return NULL.
+ */
+Datum
+to_regrole(PG_FUNCTION_ARGS)
+{
+ char *role_name = PG_GETARG_CSTRING(0);
+ Oid result;
+
+ result = get_role_oid(role_name, true);
+
+ if (OidIsValid(result))
+ PG_RETURN_OID(result);
+ else
+ PG_RETURN_NULL();
+}
+
+/*
+ * regroleout - converts role OID to "role_name"
+ */
+Datum
+regroleout(PG_FUNCTION_ARGS)
+{
+ Oid roleoid = PG_GETARG_OID(0);
+ char *result;
+
+
+ if (roleoid == InvalidOid)
+ {
+ result = pstrdup("-");
+ PG_RETURN_CSTRING(result);
+ }
+
+ result = GetUserNameFromId(roleoid);
+ PG_RETURN_CSTRING(result);
+}
+
+/*
+ * regrolerecv - converts external binary format to regrole
+ */
+Datum
+regrolerecv(PG_FUNCTION_ARGS)
+{
+ /* Exactly the same as oidrecv, so share code */
+ return oidrecv(fcinfo);
+}
+
+/*
+ * regrolesend - converts regrole to binary format
+ */
+Datum
+regrolesend(PG_FUNCTION_ARGS)
+{
+ /* Exactly the same as oidsend, so share code */
+ return oidsend(fcinfo);
+}
+
+/*
+ * regnamespacein - converts "nspname" to namespace OID
+ *
+ * We also accept a numeric OID, for symmetry with the output routine.
+ *
+ * '-' signifies unknown (OID 0). In all other cases, the input must
+ * match an existing pg_namespace entry.
+ */
+Datum
+regnamespacein(PG_FUNCTION_ARGS)
+{
+ char *nsp_name_or_oid = PG_GETARG_CSTRING(0);
+ Oid result = InvalidOid;
+
+ /* '-' ? */
+ if (strcmp(nsp_name_or_oid, "-") == 0)
+ PG_RETURN_OID(InvalidOid);
+
+ /* Numeric OID? */
+ if (nsp_name_or_oid[0] >= '0' &&
+ nsp_name_or_oid[0] <= '9' &&
+ strspn(nsp_name_or_oid, "0123456789") == strlen(nsp_name_or_oid))
+ {
+ result = DatumGetObjectId(DirectFunctionCall1(oidin,
+ CStringGetDatum(nsp_name_or_oid)));
+ PG_RETURN_OID(result);
+ }
+
+ /* Normal case: see if the name matches any pg_namespace entry. */
+ result = get_namespace_oid(nsp_name_or_oid, false);
+
+ PG_RETURN_OID(result);
+}
+
+/*
+ * to_regnamespace - converts "nspname" to namespace OID
+ *
+ * If the name is not found, we return NULL.
+ */
+Datum
+to_regnamespace(PG_FUNCTION_ARGS)
+{
+ char *nsp_name = PG_GETARG_CSTRING(0);
+ Oid result;
+
+ result = get_namespace_oid(nsp_name, true);
+
+ if (OidIsValid(result))
+ PG_RETURN_OID(result);
+ else
+ PG_RETURN_NULL();
+}
+
+/*
+ * regnamespaceout - converts namespace OID to "nsp_name"
+ */
+Datum
+regnamespaceout(PG_FUNCTION_ARGS)
+{
+ Oid nspid = PG_GETARG_OID(0);
+ char *result;
+
+ if (nspid == InvalidOid)
+ {
+ result = pstrdup("-");
+ PG_RETURN_CSTRING(result);
+ }
+
+ result = get_namespace_name(nspid);
+ if (!result)
+ {
+ /* If OID doesn't match any namespace, return it numerically */
+ result = (char *) palloc(NAMEDATALEN);
+ snprintf(result, NAMEDATALEN, "%u", nspid);
+ }
+ PG_RETURN_CSTRING(result);
+}
+
+/*
+ * regnamespacerecv - converts external binary format to regnamespace
+ */
+Datum
+regnamespacerecv(PG_FUNCTION_ARGS)
+{
+ /* Exactly the same as oidrecv, so share code */
+ return oidrecv(fcinfo);
+}
+
+/*
+ * regnamespacesend - converts regnamespace to binary format
+ */
+Datum
+regnamespacesend(PG_FUNCTION_ARGS)
+{
+ /* Exactly the same as oidsend, so share code */
+ return oidsend(fcinfo);
+}/* * text_regclass: convert text to regclass
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index 4dd3f9f..91399f7 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -3619,6 +3619,8 @@ convert_to_scalar(Datum value, Oid valuetypid, double *scaledvalue, case REGTYPEOID:
case REGCONFIGOID: case REGDICTIONARYOID:
+ case REGROLEOID:
+ case REGNAMESPACEOID: *scaledvalue = convert_numeric_to_scalar(value, valuetypid);
*scaledlobound= convert_numeric_to_scalar(lobound, boundstypid); *scaledhibound =
convert_numeric_to_scalar(hibound,boundstypid);
@@ -3724,6 +3726,8 @@ convert_numeric_to_scalar(Datum value, Oid typid) case REGTYPEOID: case
REGCONFIGOID: case REGDICTIONARYOID:
+ case REGROLEOID:
+ case REGNAMESPACEOID: /* we can treat OIDs as integers... */ return (double)
DatumGetObjectId(value); }
diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index 1af43c6..5bb03dd 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -150,6 +150,8 @@ GetCCHashEqFuncs(Oid keytype, PGFunction *hashfunc, RegProcedure *eqfunc) case REGTYPEOID:
case REGCONFIGOID: case REGDICTIONARYOID:
+ case REGROLEOID:
+ case REGNAMESPACEOID: *hashfunc = hashoid; *eqfunc = F_OIDEQ;
diff --git a/src/include/catalog/pg_cast.h b/src/include/catalog/pg_cast.h
index c49fe26..d1e303f 100644
--- a/src/include/catalog/pg_cast.h
+++ b/src/include/catalog/pg_cast.h
@@ -210,6 +210,20 @@ DATA(insert ( 3769 20 1288 a f ));DATA(insert ( 3769 23 0 a b ));DATA(insert ( 25
22051079 i f ));DATA(insert ( 1043 2205 1079 i f ));
+DATA(insert ( 26 4096 0 i b ));
+DATA(insert ( 4096 26 0 i b ));
+DATA(insert ( 20 4096 1287 i f ));
+DATA(insert ( 21 4096 313 i f ));
+DATA(insert ( 23 4096 0 i b ));
+DATA(insert ( 4096 20 1288 a f ));
+DATA(insert ( 4096 23 0 a b ));
+DATA(insert ( 26 4089 0 i b ));
+DATA(insert ( 4089 26 0 i b ));
+DATA(insert ( 20 4089 1287 i f ));
+DATA(insert ( 21 4089 313 i f ));
+DATA(insert ( 23 4089 0 i b ));
+DATA(insert ( 4089 20 1288 a f ));
+DATA(insert ( 4089 23 0 a b ));/* * String category
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 62187fb..42d7269 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -3469,6 +3469,18 @@ DESCR("convert type name to regtype");DATA(insert OID = 1079 ( regclass PGNSP PGUID
121 0 0 0 f f f f t f s 1 0 2205 "25" _null_ _null_ _null_ _null_ text_regclass _null_ _null_ _null_
));DESCR("converttext to regclass");
+DATA(insert OID = 4091 ( regrolein PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 4096 "2275" _null_ _null_
_null__null_ regrolein _null_ _null_ _null_ ));
+DESCR("I/O");
+DATA(insert OID = 4092 ( regroleout PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 2275 "4096" _null_ _null_ _null_
_null_regroleout _null_ _null_ _null_ ));
+DESCR("I/O");
+DATA(insert OID = 4093 ( to_regrole PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 4096 "2275" _null_ _null_ _null_
_null_to_regrole _null_ _null_ _null_ ));
+DESCR("convert role name to regrole");
+DATA(insert OID = 4084 ( regnamespacein PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 4089 "2275" _null_ _null_ _null_
_null_regnamespacein _null_ _null_ _null_ ));
+DESCR("I/O");
+DATA(insert OID = 4085 ( regnamespaceout PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 2275 "4089" _null_ _null_ _null_
_null_regnamespaceout _null_ _null_ _null_ ));
+DESCR("I/O");
+DATA(insert OID = 4086 ( to_regnamespace PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 4089 "2275" _null_ _null_ _null_
_null_to_regnamespace _null_ _null_ _null_ ));
+DESCR("convert namespace name to regnamespace");DATA(insert OID = 2246 ( fmgr_internal_validator PGNSP PGUID 12 1 0 0
0f f f f t f s 1 0 2278 "26" _null_ _null_ _null_ _null_ fmgr_internal_validator _null_ _null_ _null_
));DESCR("(internal)");DATA(insertOID = 2247 ( fmgr_c_validator PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 2278 "26"
_null__null_ _null_ _null_ fmgr_c_validator _null_ _null_ _null_ ));
@@ -3864,6 +3876,14 @@ DATA(insert OID = 2454 ( regtyperecv PGNSP PGUID 12 1 0 0 0 f f f f t f i
1DESCR("I/O");DATA(insertOID = 2455 ( regtypesend PGNSP PGUID 12 1 0 0 0 f f f f t f i 1 0 17 "2206" _null_
_null__null_ _null_ regtypesend _null_ _null_ _null_ ));DESCR("I/O");
+DATA(insert OID = 4094 ( regrolerecv PGNSP PGUID 12 1 0 0 0 f f f f t f i 1 0 4096 "2281" _null_ _null_
_null__null_ regrolerecv _null_ _null_ _null_ ));
+DESCR("I/O");
+DATA(insert OID = 4095 ( regrolesend PGNSP PGUID 12 1 0 0 0 f f f f t f i 1 0 17 "4096" _null_ _null_
_null__null_ regrolesend _null_ _null_ _null_ ));
+DESCR("I/O");
+DATA(insert OID = 4087 ( regnamespacerecv PGNSP PGUID 12 1 0 0 0 f f f f t f i 1 0 4089 "2281" _null_ _null_
_null__null_ regnamespacerecv _null_ _null_ _null_ ));
+DESCR("I/O");
+DATA(insert OID = 4088 ( regnamespacesend PGNSP PGUID 12 1 0 0 0 f f f f t f i 1 0 17 "4089" _null_ _null_
_null__null_ regnamespacesend _null_ _null_ _null_ ));
+DESCR("I/O");DATA(insert OID = 2456 ( bit_recv PGNSP PGUID 12 1 0 0 0 f f f f t f i 3 0 1560 "2281 26
23"_null_ _null_ _null_ _null_ bit_recv _null_ _null_ _null_ ));DESCR("I/O");DATA(insert OID = 2457 ( bit_send
PGNSP PGUID 12 1 0 0 0 f f f f t f i 1 0 17 "1560" _null_ _null_ _null_ _null_ bit_send _null_ _null_ _null_
));
diff --git a/src/include/catalog/pg_type.h b/src/include/catalog/pg_type.h
index 0a900dd..2493353 100644
--- a/src/include/catalog/pg_type.h
+++ b/src/include/catalog/pg_type.h
@@ -564,12 +564,22 @@ DATA(insert OID = 2206 ( regtype PGNSP PGUID 4 t b N f t \054 0 0 2211
regtyDESCR("registeredtype");#define REGTYPEOID 2206
+DATA(insert OID = 4096 ( regrole PGNSP PGUID 4 t b N f t \054 0 0 4097 regrolein regroleout regrolerecv
regrolesend- - - i p f 0 -1 0 0 _null_ _null_ _null_ ));
+DESCR("registered role");
+#define REGROLEOID 4096
+
+DATA(insert OID = 4089 ( regnamespace PGNSP PGUID 4 t b N f t \054 0 0 4090 regnamespacein regnamespaceout
regnamespacerecvregnamespacesend - - - i p f 0 -1 0 0 _null_ _null_ _null_ ));
+DESCR("registered namespace");
+#define REGNAMESPACEOID 4089
+DATA(insert OID = 2207 ( _regprocedure PGNSP PGUID -1 f b A f t \054 0 2202 0 array_in array_out array_recv array_send
-- array_typanalyze i x f 0 -1 0 0 _null_ _null_ _null_ ));DATA(insert OID = 2208 ( _regoper PGNSP PGUID -1 f b A
ft \054 0 2203 0 array_in array_out array_recv array_send - - array_typanalyze i x f 0 -1 0 0 _null_ _null_ _null_
));DATA(insertOID = 2209 ( _regoperator PGNSP PGUID -1 f b A f t \054 0 2204 0 array_in array_out array_recv
array_send- - array_typanalyze i x f 0 -1 0 0 _null_ _null_ _null_ ));DATA(insert OID = 2210 ( _regclass PGNSP
PGUID-1 f b A f t \054 0 2205 0 array_in array_out array_recv array_send - - array_typanalyze i x f 0 -1 0 0 _null_
_null__null_ ));DATA(insert OID = 2211 ( _regtype PGNSP PGUID -1 f b A f t \054 0 2206 0 array_in array_out
array_recvarray_send - - array_typanalyze i x f 0 -1 0 0 _null_ _null_ _null_ ));#define REGTYPEARRAYOID 2211
+DATA(insert OID = 4097 ( _regrole PGNSP PGUID -1 f b A f t \054 0 4096 0 array_in array_out array_recv array_send
-- array_typanalyze i x f 0 -1 0 0 _null_ _null_ _null_ ));
+DATA(insert OID = 4090 ( _regnamespace PGNSP PGUID -1 f b A f t \054 0 4089 0 array_in array_out array_recv array_send
-- array_typanalyze i x f 0 -1 0 0 _null_ _null_ _null_ ));/* uuid */DATA(insert OID = 2950 ( uuid PGNSP
PGUID16 f b U f t \054 0 0 2951 uuid_in uuid_out uuid_recv uuid_send - - - c p f 0 -1 0 0 _null_ _null_ _null_ ));
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index 33a453f..9e2ee82 100644
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -630,6 +630,16 @@ extern Datum regtypeout(PG_FUNCTION_ARGS);extern Datum regtyperecv(PG_FUNCTION_ARGS);extern Datum
regtypesend(PG_FUNCTION_ARGS);externDatum to_regtype(PG_FUNCTION_ARGS);
+extern Datum regrolein(PG_FUNCTION_ARGS);
+extern Datum regroleout(PG_FUNCTION_ARGS);
+extern Datum regrolerecv(PG_FUNCTION_ARGS);
+extern Datum regrolesend(PG_FUNCTION_ARGS);
+extern Datum to_regrole(PG_FUNCTION_ARGS);
+extern Datum regnamespacein(PG_FUNCTION_ARGS);
+extern Datum regnamespaceout(PG_FUNCTION_ARGS);
+extern Datum regnamespacerecv(PG_FUNCTION_ARGS);
+extern Datum regnamespacesend(PG_FUNCTION_ARGS);
+extern Datum to_regnamespace(PG_FUNCTION_ARGS);extern Datum regconfigin(PG_FUNCTION_ARGS);extern Datum
regconfigout(PG_FUNCTION_ARGS);externDatum regconfigrecv(PG_FUNCTION_ARGS);
diff --git a/src/test/regress/expected/regproc.out b/src/test/regress/expected/regproc.out
index 3342129..8c734f4 100644
--- a/src/test/regress/expected/regproc.out
+++ b/src/test/regress/expected/regproc.out
@@ -2,6 +2,7 @@-- regproc--/* If objects exist, return oids */
+CREATE ROLE regtestrole;-- without schemanameSELECT regoper('||/'); regoper
@@ -39,6 +40,18 @@ SELECT regtype('int4'); integer(1 row)
+SELECT regrole('regtestrole');
+ regrole
+-------------
+ regtestrole
+(1 row)
+
+SELECT regnamespace('pg_catalog');
+ regnamespace
+--------------
+ pg_catalog
+(1 row)
+SELECT to_regoper('||/'); to_regoper ------------
@@ -75,6 +88,18 @@ SELECT to_regtype('int4'); integer(1 row)
+SELECT to_regrole('regtestrole');
+ to_regrole
+-------------
+ regtestrole
+(1 row)
+
+SELECT to_regnamespace('pg_catalog');
+ to_regnamespace
+-----------------
+ pg_catalog
+(1 row)
+-- with schemanameSELECT regoper('pg_catalog.||/'); regoper
@@ -143,10 +168,11 @@ SELECT to_regtype('pg_catalog.int4');(1 row)/* If objects don't exist, raise errors. */
+DROP ROLE regtestrole;-- without schemanameSELECT regoper('||//');ERROR: operator does not exist: ||//
-LINE 3: SELECT regoper('||//');
+LINE 1: SELECT regoper('||//'); ^SELECT regoperator('++(int4,int4)');ERROR: operator does not
exist:++(int4,int4)
@@ -168,6 +194,14 @@ SELECT regtype('int3');ERROR: type "int3" does not existLINE 1: SELECT regtype('int3');
^
+SELECT regrole('regtestrole');
+ERROR: role "regtestrole" does not exist
+LINE 1: SELECT regrole('regtestrole');
+ ^
+SELECT regnamespace('nonexistent');
+ERROR: schema "nonexistent" does not exist
+LINE 1: SELECT regnamespace('nonexistent');
+ ^-- with schemanameSELECT regoper('ng_catalog.||/');ERROR: schema "ng_catalog" does not
exist
@@ -231,6 +265,18 @@ SELECT to_regtype('int3'); (1 row)
+SELECT to_regrole('regtestrole');
+ to_regrole
+------------
+
+(1 row)
+
+SELECT to_regnamespace('nonexistent');
+ to_regnamespace
+-----------------
+
+(1 row)
+-- with schemanameSELECT to_regoper('ng_catalog.||/'); to_regoper
diff --git a/src/test/regress/sql/regproc.sql b/src/test/regress/sql/regproc.sql
index cc90838..8edaf15 100644
--- a/src/test/regress/sql/regproc.sql
+++ b/src/test/regress/sql/regproc.sql
@@ -4,6 +4,7 @@/* If objects exist, return oids */
+CREATE ROLE regtestrole;-- without schemanameSELECT regoper('||/');
@@ -12,6 +13,8 @@ SELECT regproc('now');SELECT regprocedure('abs(numeric)');SELECT regclass('pg_class');SELECT
regtype('int4');
+SELECT regrole('regtestrole');
+SELECT regnamespace('pg_catalog');SELECT to_regoper('||/');SELECT to_regoperator('+(int4,int4)');
@@ -19,6 +22,8 @@ SELECT to_regproc('now');SELECT to_regprocedure('abs(numeric)');SELECT to_regclass('pg_class');SELECT
to_regtype('int4');
+SELECT to_regrole('regtestrole');
+SELECT to_regnamespace('pg_catalog');-- with schemaname
@@ -37,6 +42,8 @@ SELECT to_regtype('pg_catalog.int4');/* If objects don't exist, raise errors. */
+DROP ROLE regtestrole;
+-- without schemanameSELECT regoper('||//');
@@ -45,6 +52,8 @@ SELECT regproc('know');SELECT regprocedure('absinthe(numeric)');SELECT regclass('pg_classes');SELECT
regtype('int3');
+SELECT regrole('regtestrole');
+SELECT regnamespace('nonexistent');-- with schemaname
@@ -65,6 +74,8 @@ SELECT to_regproc('know');SELECT to_regprocedure('absinthe(numeric)');SELECT
to_regclass('pg_classes');SELECTto_regtype('int3');
+SELECT to_regrole('regtestrole');
+SELECT to_regnamespace('nonexistent');-- with schemaname
I sent the previous mail unfinished. At Thu, 09 Apr 2015 17:25:10 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20150409.172510.29010318.horiguchi.kyotaro@lab.ntt.co.jp> > Hello, sorry for the absence. I changed the regnamespace's > behavior as the same as the other reg* types. And I attached a > patch as separate one that fixes regroleout to do the same as the > other reg* types, because I have because, I doubt that it is appropriate way to do so. > 0001-Add-regrole_v6.patch : fix regnamespace to behave as the > same as the other reg* types. > > 0001-Make-regnamespace-behave-as-the-same-as-other-reg-ty.patch: > Diff from v5 to v6, only for convinience. > > 0001-Fix-regroleout-s-behavior-following-other-out-functi.patch: > Fixes regroleout so as to behave as the same as other reg* > types, but might be a bit too large. -- Kyotaro Horiguchi NTT Open Source Software Center
Hello, I fonund that pg_proc.h got modified so rebased and rearranged the patchset merging the recent fixes. regards, > I sent the previous mail unfinished. > > At Thu, 09 Apr 2015 17:25:10 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in<20150409.172510.29010318.horiguchi.kyotaro@lab.ntt.co.jp> > > Hello, sorry for the absence. I changed the regnamespace's > > behavior as the same as the other reg* types. And I attached a > > patch as separate one that fixes regroleout to do the same as the > > other reg* types, because I have > > because, I doubt that it is appropriate way to do so. > > > 0001-Add-regrole_v6.patch : fix regnamespace to behave as the > > same as the other reg* types. > > > > 0001-Make-regnamespace-behave-as-the-same-as-other-reg-ty.patch: > > Diff from v5 to v6, only for convinience. > > > > 0001-Fix-regroleout-s-behavior-following-other-out-functi.patch: > > Fixes regroleout so as to behave as the same as other reg* > > types, but might be a bit too large. -- Kyotaro Horiguchi NTT Open Source Software Center