How about to have relnamespace and relrole?

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

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

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

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

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

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

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

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

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

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

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.

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

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

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.

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.

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

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

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

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

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

Hi,

Personally, I was looking for something like this as I need to see rolename
and namespace name many times in my queries rather than it's oid.
But making a JOIN expression every-time was a pain. This certainly makes it
easier. And I see most DBAs are looking for it.

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.

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

2.
+ char *role_or_oid = PG_GETARG_CSTRING(0);

Can we have variable named as role_name_or_oid, like other similar
functions?

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.

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

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.

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

Thanks

--
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Reviewed posted directly on mail thread instead of posting it on commitfest app.

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

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

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

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

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

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

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

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

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

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