Re: Fwd: Proposal: variant of regclass

I would like to propose to add a variant of regclass.

Background:
Pgpool-II (http://www.pgpool.net) needs to get information of tables
by querying PostgreSQL's system catalog. For efficiency and
correctness of the info (search path consideration), pgpool-II issues
such queries piggy packing the user's connection to PostgreSQL and
regclass is frequently used in the queries.

One problem with stock regclass is, it raises exception when a target
tables is not found, which breaks user's transaction currently running
on the session. For a workaround, pgpool-II ships non-error-raising
version of regclass, called pgpool_regclass. However it's not perfect
solution because people (or even distributions/packagers) forget to
install it [1]. Another problem is, pgpool_regclass heavily depends on
the internals of PostgreSQL, which has been changed version to
versions and pgpool developers need to spend some efforts to adopt the
changes.

Someone suggested before that pgpool_regclass could be implemented as
a pl function, but I think it is unacceptable because 1) the function
is heavily used and using pl will cause performance problem, 2) it
does solve the problem I said in [1].

Proposal:
I would like to add a variant of regclass, which is exactly same as
current regclass except it does not raise an error when the target
table is not found. Instead it returns InvalidOid (0).

Pgpool-II is being shipped with various distributions and used by many
companies including EnterpriseDB, VMWare, SRA OSS and so on. IMO this
small enhancement will benefit many PostgreSQL users by small changes
to PostgreSQL.
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp

Tatsuo Ishii <ishii(at)sraoss(dot)co(dot)jp> writes:
> I would like to add a variant of regclass, which is exactly same as
> current regclass except it does not raise an error when the target
> table is not found. Instead it returns InvalidOid (0).

I've sometimes thought we should just make all the reg* input converters
act that way. It's not terribly consistent that they'll happily take
numeric inputs that don't correspond to any existing OID. And more
often than not, I've found the throw-an-error behavior to be annoying
not helpful.

In any case, -1 for dealing with this only for regclass and not the
other ones.

regards, tom lane

> Tatsuo Ishii <ishii(at)sraoss(dot)co(dot)jp> writes:
>> I would like to add a variant of regclass, which is exactly same as
>> current regclass except it does not raise an error when the target
>> table is not found. Instead it returns InvalidOid (0).
>
> I've sometimes thought we should just make all the reg* input converters
> act that way. It's not terribly consistent that they'll happily take
> numeric inputs that don't correspond to any existing OID. And more
> often than not, I've found the throw-an-error behavior to be annoying
> not helpful.
>
> In any case, -1 for dealing with this only for regclass and not the
> other ones.

I'm happy with changing reg* at the same time. Will come up with the
modified proposal.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp

Hello, Tom.

You wrote:

TL> Tatsuo Ishii <ishii(at)sraoss(dot)co(dot)jp> writes:
>> I would like to add a variant of regclass, which is exactly same as
>> current regclass except it does not raise an error when the target
>> table is not found. Instead it returns InvalidOid (0).

TL> I've sometimes thought we should just make all the reg* input converters
TL> act that way.

Absolutely agree. I cannot see the case whn error is the appropriate
solution. Casting nonexistent objects to NULL is the way to go for me.

TL> It's not terribly consistent that they'll happily take
TL> numeric inputs that don't correspond to any existing OID. And more
TL> often than not, I've found the throw-an-error behavior to be annoying
TL> not helpful.

TL> In any case, -1 for dealing with this only for regclass and not the
TL> other ones.

TL> regards, tom lane

--
With best wishes,
Pavel mailto:pavel(at)gf(dot)microolap(dot)com

On 2013-12-04 20:25:53 -0500, Tom Lane wrote:
> Tatsuo Ishii <ishii(at)sraoss(dot)co(dot)jp> writes:
> > I would like to add a variant of regclass, which is exactly same as
> > current regclass except it does not raise an error when the target
> > table is not found. Instead it returns InvalidOid (0).
>
> I've sometimes thought we should just make all the reg* input converters
> act that way. It's not terribly consistent that they'll happily take
> numeric inputs that don't correspond to any existing OID. And more
> often than not, I've found the throw-an-error behavior to be annoying
> not helpful.

I find that to be a bit of a scary change. I have seen application check
for the existance of tables using the error thrown by ::regclass. Now,
they could change that to check for IS NULL which would be better for
them performancewise, but the likelihood they will notice in time seems
small.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

2013/12/5 Andres Freund <andres(at)2ndquadrant(dot)com>

> On 2013-12-04 20:25:53 -0500, Tom Lane wrote:
> > Tatsuo Ishii <ishii(at)sraoss(dot)co(dot)jp> writes:
> > > I would like to add a variant of regclass, which is exactly same as
> > > current regclass except it does not raise an error when the target
> > > table is not found. Instead it returns InvalidOid (0).
> >
> > I've sometimes thought we should just make all the reg* input converters
> > act that way. It's not terribly consistent that they'll happily take
> > numeric inputs that don't correspond to any existing OID. And more
> > often than not, I've found the throw-an-error behavior to be annoying
> > not helpful.
>
> I find that to be a bit of a scary change. I have seen application check
> for the existance of tables using the error thrown by ::regclass. Now,
> they could change that to check for IS NULL which would be better for
> them performancewise, but the likelihood they will notice in time seems
> small.
>
>
this change can break some applications

but personally I like this change.

We can introduce some assert polymorphic function

CREATE OR REPLACE FUNCTION notnull(any, message text) RETURNS any, that can
be used for check inside SQL

Regards

Pavel

> Greetings,
>
> Andres Freund
>
> --
> Andres Freund http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

On 2013-12-05 11:54:20 +0100, Pavel Stehule wrote:
> 2013/12/5 Andres Freund <andres(at)2ndquadrant(dot)com>
> We can introduce some assert polymorphic function
>
> CREATE OR REPLACE FUNCTION notnull(any, message text) RETURNS any, that can
> be used for check inside SQL

Uh. How is that going to help applications that upgraded, without having
noticed a pretty obscure notice in the release notes?

If this were day one, I would agree we should go that way, but today...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

2013/12/5 Andres Freund <andres(at)2ndquadrant(dot)com>

> On 2013-12-05 11:54:20 +0100, Pavel Stehule wrote:
> > 2013/12/5 Andres Freund <andres(at)2ndquadrant(dot)com>
> > We can introduce some assert polymorphic function
> >
> > CREATE OR REPLACE FUNCTION notnull(any, message text) RETURNS any, that
> can
> > be used for check inside SQL
>
> Uh. How is that going to help applications that upgraded, without having
> noticed a pretty obscure notice in the release notes?
>

this function doesn't replace a "obscure notice in the release notes".

On second hand is better to throw unpractically designed feature early
than hold it forever.

If there was not too aversion against GUC, I can say, so for some time GUC
can be solution. But it isnot

Regards

Pavel

>
> If this were day one, I would agree we should go that way, but today...
>
> Greetings,
>
> Andres Freund
>
> --
> Andres Freund http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>

Hello, Andres.

You wrote:

AF> On 2013-12-04 20:25:53 -0500, Tom Lane wrote:
>> Tatsuo Ishii <ishii(at)sraoss(dot)co(dot)jp> writes:
>> > I would like to add a variant of regclass, which is exactly same as
>> > current regclass except it does not raise an error when the target
>> > table is not found. Instead it returns InvalidOid (0).
>>
>> I've sometimes thought we should just make all the reg* input converters
>> act that way. It's not terribly consistent that they'll happily take
>> numeric inputs that don't correspond to any existing OID. And more
>> often than not, I've found the throw-an-error behavior to be annoying
>> not helpful.

AF> I find that to be a bit of a scary change. I have seen application check
AF> for the existance of tables using the error thrown by ::regclass. Now,
AF> they could change that to check for IS NULL which would be better for
AF> them performancewise, but the likelihood they will notice in time seems
AF> small.

I personally see two approaches:
1. Implement GUC variable controling this behaviour per session
2. Introduce new safe reg* variables, e.g. "sregclass", "sregtype" etc.

AF> Greetings,

AF> Andres Freund

AF> --
AF> Andres Freund http://www.2ndQuadrant.com/
AF> PostgreSQL Development, 24x7 Support, Training & Services

--
With best wishes,
Pavel mailto:pavel(at)gf(dot)microolap(dot)com

Pavel Golub <pavel(at)microolap(dot)com> writes:
> I personally see two approaches:
> 1. Implement GUC variable controling this behaviour per session
> 2. Introduce new safe reg* variables, e.g. "sregclass", "sregtype" etc.

I don't think new types are a good idea. If we are afraid to change
the behavior of the input converters, what we should do is introduce
new functions, eg "toregclass(text) returns regclass".

regards, tom lane

On 12/5/13, 9:41 AM, Tom Lane wrote:
> Pavel Golub <pavel(at)microolap(dot)com> writes:
>> I personally see two approaches:
>> 1. Implement GUC variable controling this behaviour per session
>> 2. Introduce new safe reg* variables, e.g. "sregclass", "sregtype" etc.
>
> I don't think new types are a good idea. If we are afraid to change
> the behavior of the input converters, what we should do is introduce
> new functions, eg "toregclass(text) returns regclass".

We could invent some sneaky syntax variants, like 'pg_klass'::regclass
errors, but '?pg_klass'::regclass does not.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> We could invent some sneaky syntax variants, like 'pg_klass'::regclass
> errors, but '?pg_klass'::regclass does not.

Hmm ... cute idea, but shoehorning it into regoperator might be
problematic. You'd have to pick a flag character that wasn't a
valid operator character, which lets out '?' as well as a lot
of the other mnemonically-reasonable choices.

regards, tom lane

On Thu, Dec 5, 2013 at 9:41 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Pavel Golub <pavel(at)microolap(dot)com> writes:
>> I personally see two approaches:
>> 1. Implement GUC variable controling this behaviour per session
>> 2. Introduce new safe reg* variables, e.g. "sregclass", "sregtype" etc.
>
> I don't think new types are a good idea. If we are afraid to change
> the behavior of the input converters, what we should do is introduce
> new functions, eg "toregclass(text) returns regclass".

That seems like a pretty reasonable approach.

I don't have a strong opinion on whether it's worth the
backward-compatibility break that would ensue from just changing this
outright. I admit that I've been annoyed by this behavior more than
once, but I've also been beaten by customers enough times to know that
backward-compatibility has value to other people in some cases where
it does not have such value to me.

Another advantage of this approach is that, IIUC, type input functions
can't return a NULL value. So 'pg_klass'::regclass could return 0,
but not NULL. On the other hand, toregclass('pg_klass') *could*
return NULL, which seems conceptually cleaner.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Dec 5, 2013 at 9:41 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I don't think new types are a good idea. If we are afraid to change
>> the behavior of the input converters, what we should do is introduce
>> new functions, eg "toregclass(text) returns regclass".

> That seems like a pretty reasonable approach.

> I don't have a strong opinion on whether it's worth the
> backward-compatibility break that would ensue from just changing this
> outright. I admit that I've been annoyed by this behavior more than
> once, but I've also been beaten by customers enough times to know that
> backward-compatibility has value to other people in some cases where
> it does not have such value to me.

I'm getting less enamored of just-change-the-input-behavior myself.
The case that occurred to me is, suppose somebody's got a table containing
a regclass or regproc column, and he dumps and reloads it. If the input
converter silently replaces unknown names by 0, he's at risk of unexpected
data loss, if the reload is done before he's created all the referenced
objects.

> Another advantage of this approach is that, IIUC, type input functions
> can't return a NULL value. So 'pg_klass'::regclass could return 0,
> but not NULL. On the other hand, toregclass('pg_klass') *could*
> return NULL, which seems conceptually cleaner.

Yeah, I was thinking of that too.

regards, tom lane

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Another advantage of this approach is that, IIUC, type input functions
>> can't return a NULL value. So 'pg_klass'::regclass could return 0,
>> but not NULL. On the other hand, toregclass('pg_klass') *could*
>> return NULL, which seems conceptually cleaner.

BTW, another arguable advantage of fixing this via new functions is
that users could write equivalent (though no doubt slower) functions
for use in pre-9.4 releases, and thus not need to maintain multiple
versions of app code that relies on this behavior.

regards, tom lane

On 12/5/13, 10:08 AM, Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>> We could invent some sneaky syntax variants, like 'pg_klass'::regclass
>> errors, but '?pg_klass'::regclass does not.
>
> Hmm ... cute idea, but shoehorning it into regoperator might be
> problematic. You'd have to pick a flag character that wasn't a
> valid operator character, which lets out '?' as well as a lot
> of the other mnemonically-reasonable choices.

Well, you could pick any letter, I suppose.

On Dec 5, 2013, at 7:52 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> BTW, another arguable advantage of fixing this via new functions is
> that users could write equivalent (though no doubt slower) functions
> for use in pre-9.4 releases, and thus not need to maintain multiple
> versions of app code that relies on this behavior.

+1 to this idea. Feels cleanest.

David

> I'm getting less enamored of just-change-the-input-behavior myself.
> The case that occurred to me is, suppose somebody's got a table containing
> a regclass or regproc column, and he dumps and reloads it. If the input
> converter silently replaces unknown names by 0, he's at risk of unexpected
> data loss, if the reload is done before he's created all the referenced
> objects.
>
>> Another advantage of this approach is that, IIUC, type input functions
>> can't return a NULL value. So 'pg_klass'::regclass could return 0,
>> but not NULL. On the other hand, toregclass('pg_klass') *could*
>> return NULL, which seems conceptually cleaner.
>
> Yeah, I was thinking of that too.

Can I make sure that we want to keep the current behavior:

test=# SELECT 'pg_klass'::regclass;
ERROR: relation "pg_klass" does not exist
LINE 1: SELECT 'pg_klass'::regclass;
^

Or do we want the SELECT to return 0 in the case above?
I'm asking because of this:
>> So 'pg_klass'::regclass could return 0,
>> but not NULL.

In the mean time I agree the idea that we add:
toregclass(text) returns regclass
and friends.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp

Tatsuo Ishii <ishii(at)postgresql(dot)org> writes:
> Can I make sure that we want to keep the current behavior:

> test=# SELECT 'pg_klass'::regclass;
> ERROR: relation "pg_klass" does not exist

Yeah, I think the consensus is to not change the behavior of the input
functions, just add some new ones.

regards, tom lane

> Tatsuo Ishii <ishii(at)postgresql(dot)org> writes:
>> Can I make sure that we want to keep the current behavior:
>
>> test=# SELECT 'pg_klass'::regclass;
>> ERROR: relation "pg_klass" does not exist
>
> Yeah, I think the consensus is to not change the behavior of the input
> functions, just add some new ones.

Ok, here is the conceptual patch to implement "toregclass" (only for
now). If my direction is ok, I'll come up with complete patches to
implement more "to*" functions. Any advice will be appreciated.

Here is a sample session:

test=# select toregclass('foo');
toregclass
------------
-
(1 row)

test=# select toregclass('pg_class');
toregclass
------------
pg_class
(1 row)

test=# select toregclass('pg_class')::oid;
toregclass
------------
1259
(1 row)

test=# select toregclass('foo')::oid;
toregclass
------------
0
(1 row)

Implementation notes:

To implement toregclass, which does not throw errors when invalid
argument is given, src/backend/utils/adt/regproc.c is modified. I
added two static functions:

static Datum regclass_gut(char *class_name_or_oid, bool raiseError);
static List *stringToQualifiedNameList_gut(const char *string, bool raiseError);

regclass_gut is called from regclassin and toregclass and do the most
job before regclassin did. "raiseError" flag controls whether an error
is raised or not when an invalid argument (for example non existent
relation) is given. For this purpose, regclass_gut wraps the call to
oidin using a PG_TRY block.

Secondly, when called as bootstap and raiseError is true, returns
InvalidOid instead of raising an error "relation XXX does not
exist". However, I doubt there's no customer who calls regclass_gut
with raiseError is false in the bootstrap.

Thirdly, stringToQualifiedNameList_gut is added to replace
stringToQualifiedNameList. The reason why I don't use PG_TRY block is,
I need to free some memory allocated inside the function in an
error condition.

Finially I modified the call to RangeVarGetRelid to switch
"missing_ok" flag to reflect raiseError argument.

One thing I need to further is modifying makeRangeVarFromNameList. If
strange schema qualified name like "a.b.c.d.e.f" is given, still an
error raises.

So, any advice will be appreciated.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp

Tatsuo Ishii <ishii(at)postgresql(dot)org> writes:
> To implement toregclass, which does not throw errors when invalid
> argument is given, src/backend/utils/adt/regproc.c is modified. I
> added two static functions:

> static Datum regclass_gut(char *class_name_or_oid, bool raiseError);
> static List *stringToQualifiedNameList_gut(const char *string, bool raiseError);

Please spell that as "guts" not "gut".

> regclass_gut is called from regclassin and toregclass and do the most
> job before regclassin did. "raiseError" flag controls whether an error
> is raised or not when an invalid argument (for example non existent
> relation) is given. For this purpose, regclass_gut wraps the call to
> oidin using a PG_TRY block.

I do not think that use of PG_TRY is either necessary or acceptable
--- for example, it will happily trap and discard query-cancel errors,
as well as other errors that are not necessarily safe to ignore.
If you don't want to risk an error on an invalid numeric string,
how about parsing the integer yourself with sscanf?

More generally, though, I don't see a great need to try to promise
that this function *never* throws any errors, and so I'm also suspicious
of the hacking you've done on stringToQualifiedNameList. I'm even
less happy about the idea that this patch might start reaching into
things like makeRangeVarFromNameList. I think it's sufficient if it
doesn't throw an error on name-not-found; you don't have to try to
prevent weird syntax problems from throwing errors.

regards, tom lane

>> static Datum regclass_gut(char *class_name_or_oid, bool raiseError);
>> static List *stringToQualifiedNameList_gut(const char *string, bool raiseError);
>
> Please spell that as "guts" not "gut".

Thanks. I see.

>> regclass_gut is called from regclassin and toregclass and do the most
>> job before regclassin did. "raiseError" flag controls whether an error
>> is raised or not when an invalid argument (for example non existent
>> relation) is given. For this purpose, regclass_gut wraps the call to
>> oidin using a PG_TRY block.
>
> I do not think that use of PG_TRY is either necessary or acceptable
> --- for example, it will happily trap and discard query-cancel errors,
> as well as other errors that are not necessarily safe to ignore.
> If you don't want to risk an error on an invalid numeric string,
> how about parsing the integer yourself with sscanf?

Fair enough. I will remove the part.

> More generally, though, I don't see a great need to try to promise
> that this function *never* throws any errors, and so I'm also suspicious
> of the hacking you've done on stringToQualifiedNameList. I'm even
> less happy about the idea that this patch might start reaching into
> things like makeRangeVarFromNameList. I think it's sufficient if it
> doesn't throw an error on name-not-found; you don't have to try to
> prevent weird syntax problems from throwing errors.

For the pgpool-II use case, I'm happy to follow you because pgpool-II
always does a grammatical check (using raw parser) on a query first
and such syntax problem will be pointed out, thus never reaches to
the state where calling toregclass.

One concern is, other users expect toregclass to detect such syntax
problems. Anybody?

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp

On Mon, Dec 16, 2013 at 8:22 PM, Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:
>>> static Datum regclass_gut(char *class_name_or_oid, bool raiseError);
>>> static List *stringToQualifiedNameList_gut(const char *string, bool raiseError);
>>
>> Please spell that as "guts" not "gut".
>
> Thanks. I see.
>
>>> regclass_gut is called from regclassin and toregclass and do the most
>>> job before regclassin did. "raiseError" flag controls whether an error
>>> is raised or not when an invalid argument (for example non existent
>>> relation) is given. For this purpose, regclass_gut wraps the call to
>>> oidin using a PG_TRY block.
>>
>> I do not think that use of PG_TRY is either necessary or acceptable
>> --- for example, it will happily trap and discard query-cancel errors,
>> as well as other errors that are not necessarily safe to ignore.
>> If you don't want to risk an error on an invalid numeric string,
>> how about parsing the integer yourself with sscanf?
>
> Fair enough. I will remove the part.
>
>> More generally, though, I don't see a great need to try to promise
>> that this function *never* throws any errors, and so I'm also suspicious
>> of the hacking you've done on stringToQualifiedNameList. I'm even
>> less happy about the idea that this patch might start reaching into
>> things like makeRangeVarFromNameList. I think it's sufficient if it
>> doesn't throw an error on name-not-found; you don't have to try to
>> prevent weird syntax problems from throwing errors.
>
> For the pgpool-II use case, I'm happy to follow you because pgpool-II
> always does a grammatical check (using raw parser) on a query first
> and such syntax problem will be pointed out, thus never reaches to
> the state where calling toregclass.
>
> One concern is, other users expect toregclass to detect such syntax
> problems. Anybody?

It seems fine to me if the new function ignores the specific error of
"relation does not exist" while continuing to throw other errors.

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

>> For the pgpool-II use case, I'm happy to follow you because pgpool-II
>> always does a grammatical check (using raw parser) on a query first
>> and such syntax problem will be pointed out, thus never reaches to
>> the state where calling toregclass.
>>
>> One concern is, other users expect toregclass to detect such syntax
>> problems. Anybody?
>
> It seems fine to me if the new function ignores the specific error of
> "relation does not exist" while continuing to throw other errors.

Thanks. Here is the revised conceptual patch. I'm going to post a
concrete patch and register it to 2014-01 Commit Fest.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp

>> It seems fine to me if the new function ignores the specific error of
>> "relation does not exist" while continuing to throw other errors.
>
> Thanks. Here is the revised conceptual patch. I'm going to post a
> concrete patch and register it to 2014-01 Commit Fest.

Before proceeding the work, I would like to make sure that followings
are complete list of new functions. Inside parentheses are
corresponding original functions.

toregproc (regproc)
toregoper (regoper)
toregclass (regclass)
toregtype (regtype)

In addition to above, we have regprocedure, regoperator, regconfig,
pg_ts_config and regdictionary. However I don't see much use
case/users for new functions corresponding to them. Opinions?

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp

On 12/31/2013 02:38 AM, Tatsuo Ishii wrote:
> Before proceeding the work, I would like to make sure that followings
> are complete list of new functions. Inside parentheses are
> corresponding original functions.
>
> toregproc (regproc)
> toregoper (regoper)
> toregclass (regclass)
> toregtype (regtype)

Pardon the bikeshedding, but those are hard to read for me. I would
prefer to go with the to_timestamp() model and add an underscore to
those names.

--
Vik

> On 12/31/2013 02:38 AM, Tatsuo Ishii wrote:
>> Before proceeding the work, I would like to make sure that followings
>> are complete list of new functions. Inside parentheses are
>> corresponding original functions.
>>
>> toregproc (regproc)
>> toregoper (regoper)
>> toregclass (regclass)
>> toregtype (regtype)
>
> Pardon the bikeshedding, but those are hard to read for me. I would
> prefer to go with the to_timestamp() model and add an underscore to
> those names.

I have no problem with adding "to_". Objection?

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp

2013/12/31 Tatsuo Ishii <ishii(at)postgresql(dot)org>

> > On 12/31/2013 02:38 AM, Tatsuo Ishii wrote:
> >> Before proceeding the work, I would like to make sure that followings
> >> are complete list of new functions. Inside parentheses are
> >> corresponding original functions.
> >>
> >> toregproc (regproc)
> >> toregoper (regoper)
> >> toregclass (regclass)
> >> toregtype (regtype)
> >
> > Pardon the bikeshedding, but those are hard to read for me. I would
> > prefer to go with the to_timestamp() model and add an underscore to
> > those names.
>
> I have no problem with adding "to_". Objection?
>

I like to_reg* too

Regards

Pavel

>
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese: http://www.sraoss.co.jp
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Here is the patch to implement to_regclass, to_regproc, to_regoper,
and to_regtype. They are new functions similar to regclass, regproc,
regoper, and regtype except that if requested object is not found,
returns InvalidOid, rather than raises an error.

On Tue, 31 Dec 2013 12:10:56 +0100
Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:

> 2013/12/31 Tatsuo Ishii <ishii(at)postgresql(dot)org>
>
> > > On 12/31/2013 02:38 AM, Tatsuo Ishii wrote:
> > >> Before proceeding the work, I would like to make sure that followings
> > >> are complete list of new functions. Inside parentheses are
> > >> corresponding original functions.
> > >>
> > >> toregproc (regproc)
> > >> toregoper (regoper)
> > >> toregclass (regclass)
> > >> toregtype (regtype)
> > >
> > > Pardon the bikeshedding, but those are hard to read for me. I would
> > > prefer to go with the to_timestamp() model and add an underscore to
> > > those names.
> >
> > I have no problem with adding "to_". Objection?
> >
>
> I like to_reg* too
>
> Regards
>
> Pavel
>
>
> >
> > Best regards,
> > --
> > Tatsuo Ishii
> > SRA OSS, Inc. Japan
> > English: http://www.sraoss.co.jp/index_en.php
> > Japanese: http://www.sraoss.co.jp
> >
> >
> > --
> > Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> > To make changes to your subscription:
> > http://www.postgresql.org/mailpref/pgsql-hackers
> >

--
Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>

Hi,

On Tue, Jan 14, 2014 at 4:28 PM, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
> Here is the patch to implement to_regclass, to_regproc, to_regoper,
> and to_regtype. They are new functions similar to regclass, regproc,
> regoper, and regtype except that if requested object is not found,
> returns InvalidOid, rather than raises an error.

You should add this patch to the upcoming commit fest (beginning
tomorrow actually), I am not seeing it in the list:
https://commitfest.postgresql.org/action/commitfest_view?id=21
Thanks,
--
Michael

> On Tue, Jan 14, 2014 at 4:28 PM, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
>> Here is the patch to implement to_regclass, to_regproc, to_regoper,
>> and to_regtype. They are new functions similar to regclass, regproc,
>> regoper, and regtype except that if requested object is not found,
>> returns InvalidOid, rather than raises an error.
>
> You should add this patch to the upcoming commit fest (beginning
> tomorrow actually), I am not seeing it in the list:
> https://commitfest.postgresql.org/action/commitfest_view?id=21
> Thanks,
> --
> Michael

Of course. Problem is, in order to add to commit fest, patch's URL is
needed in the first place. And the URL is not defined until a posting
is made.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp

Hi,

I have begun the review as part of the commitfest. Below are my comments
....

------------

*_guts() functions are defined as returning Datum, while they are actually
returning Oid. They should be defined as returning Oid.
Also the PG_RETURN_OID() has been still used in some of the *_guts()
functions. They should use 'return';

------------

In pg_proc.h, to_regproc() has been defined as function returning
*regclass* type.

------------

I, as a user would be happier if we also have to_regprocedure() and
to_regoperator(). The following query looks a valid use-case where one
needs to find if a particular function exists. Using to_regproc('sum') does
not make sense here because it will return InvalidOid, which will not tell
us whether that is because there is no such function or whether there are
duplicate function names.
select * from pg_proc where oid = to_regprocedure('sum(int)');

------------

The changes in parse_type.c look all good, except some cosmetic changes:

The parameters are not aligned one below the other for these two lines :
typenameTypeIdAndModMissingOk(ParseState *pstate, const TypeName *typeName,
Oid *typeid_p, int32 *typmod_p)

Similar thing for typenameTypeIdAndMod_guts().

--------------

Please also add some testcases in the regression tests.

On 14 January 2014 12:58, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:

> Here is the patch to implement to_regclass, to_regproc, to_regoper,
> and to_regtype. They are new functions similar to regclass, regproc,
> regoper, and regtype except that if requested object is not found,
> returns InvalidOid, rather than raises an error.
>
> On Tue, 31 Dec 2013 12:10:56 +0100
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>
> > 2013/12/31 Tatsuo Ishii <ishii(at)postgresql(dot)org>
> >
> > > > On 12/31/2013 02:38 AM, Tatsuo Ishii wrote:
> > > >> Before proceeding the work, I would like to make sure that
> followings
> > > >> are complete list of new functions. Inside parentheses are
> > > >> corresponding original functions.
> > > >>
> > > >> toregproc (regproc)
> > > >> toregoper (regoper)
> > > >> toregclass (regclass)
> > > >> toregtype (regtype)
> > > >
> > > > Pardon the bikeshedding, but those are hard to read for me. I would
> > > > prefer to go with the to_timestamp() model and add an underscore to
> > > > those names.
> > >
> > > I have no problem with adding "to_". Objection?
> > >
> >
> > I like to_reg* too
> >
> > Regards
> >
> > Pavel
> >
> >
> > >
> > > Best regards,
> > > --
> > > Tatsuo Ishii
> > > SRA OSS, Inc. Japan
> > > English: http://www.sraoss.co.jp/index_en.php
> > > Japanese: http://www.sraoss.co.jp
> > >
> > >
> > > --
> > > Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> > > To make changes to your subscription:
> > > http://www.postgresql.org/mailpref/pgsql-hackers
> > >
>
>
> --
> Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

> I, as a user would be happier if we also have to_regprocedure() and
> to_regoperator(). The following query looks a valid use-case where one
> needs to find if a particular function exists. Using to_regproc('sum') does
> not make sense here because it will return InvalidOid, which will not tell
> us whether that is because there is no such function or whether there are
> duplicate function names.
> select * from pg_proc where oid = to_regprocedure('sum(int)');

I doubt the value of the use case above. Hasn't psql already done an
excellent job?

test=# \df sum
List of functions
Schema | Name | Result data type | Argument data types | Type
------------+------+------------------+---------------------+------
pg_catalog | sum | numeric | bigint | agg
pg_catalog | sum | double precision | double precision | agg
pg_catalog | sum | bigint | integer | agg
pg_catalog | sum | interval | interval | agg
pg_catalog | sum | money | money | agg
pg_catalog | sum | numeric | numeric | agg
pg_catalog | sum | real | real | agg
pg_catalog | sum | bigint | smallint | agg
(8 rows)

If you need simliar functionality in the backend, you could always
define a view using the query generated by psql.

********* QUERY **********
SELECT n.nspname as "Schema",
p.proname as "Name",
pg_catalog.pg_get_function_result(p.oid) as "Result data type",
pg_catalog.pg_get_function_arguments(p.oid) as "Argument data types",
CASE
WHEN p.proisagg THEN 'agg'
WHEN p.proiswindow THEN 'window'
WHEN p.prorettype = 'pg_catalog.trigger'::pg_catalog.regtype THEN 'trigger'
ELSE 'normal'
END as "Type"
FROM pg_catalog.pg_proc p
LEFT JOIN pg_catalog.pg_namespace n ON n.oid = p.pronamespace
WHERE p.proname ~ '^(sum)$'
AND pg_catalog.pg_function_is_visible(p.oid)
ORDER BY 1, 2, 4;
**************************

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp

On Tue, Jan 14, 2014 at 9:28 AM, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
> Here is the patch to implement to_regclass, to_regproc, to_regoper,
> and to_regtype.

+ static Datum regclass_guts(char *class_name_or_oid, bool raiseError);

Minor bikeshedding, a lot of code currently uses an argument named
"missing_ok" for this purpose (with inverse meaning of course). Any
reasons why you chose "raiseError" instead?

I only had a brief look at the patch, so maybe I'm missing something.
But I don't think you should create 3 variants of these functions:
* parseTypeString calls parseTypeString_guts with false
* parseTypeStringMissingOk calls parseTypeString_guts with true
* parseTypeString_guts

And this is just silly:

if (raiseError)
parseTypeString(typ_name_or_oid, &result, &typmod);
else
parseTypeStringMissingOk(typ_name_or_oid, &result, &typmod);

Just add an argument to parseTypeString and patch all the callers.

> if requested object is not found,
> returns InvalidOid, rather than raises an error.

I thought the consensus was that returning NULL is better than
InvalidOid? From an earlier message:

On Thu, Dec 5, 2013 at 5:25 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Another advantage of this approach is that, IIUC, type input functions
> can't return a NULL value. So 'pg_klass'::regclass could return 0,
> but not NULL. On the other hand, toregclass('pg_klass') *could*
> return NULL, which seems conceptually cleaner.

Regards,
Marti

> On Tue, Jan 14, 2014 at 9:28 AM, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
>> Here is the patch to implement to_regclass, to_regproc, to_regoper,
>> and to_regtype.
>
> + static Datum regclass_guts(char *class_name_or_oid, bool raiseError);
>
> Minor bikeshedding, a lot of code currently uses an argument named
> "missing_ok" for this purpose (with inverse meaning of course). Any
> reasons why you chose "raiseError" instead?

Originally the proposal checks errors like syntactical one in addition
to missing objects. So I think "raiseError" was more appropriate at
that time. Now they only check missing objects. So renaming to
"missing_ok" could be more appropriate.

> I only had a brief look at the patch, so maybe I'm missing something.
> But I don't think you should create 3 variants of these functions:
> * parseTypeString calls parseTypeString_guts with false
> * parseTypeStringMissingOk calls parseTypeString_guts with true
> * parseTypeString_guts
>
> And this is just silly:
>
> if (raiseError)
> parseTypeString(typ_name_or_oid, &result, &typmod);
> else
> parseTypeStringMissingOk(typ_name_or_oid, &result, &typmod);
>
> Just add an argument to parseTypeString and patch all the callers.

Leave the disccusion to Yugo..

>> if requested object is not found,
>> returns InvalidOid, rather than raises an error.
>
> I thought the consensus was that returning NULL is better than
> InvalidOid? From an earlier message:
>
> On Thu, Dec 5, 2013 at 5:25 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> Another advantage of this approach is that, IIUC, type input functions
>> can't return a NULL value. So 'pg_klass'::regclass could return 0,
>> but not NULL. On the other hand, toregclass('pg_klass') *could*
>> return NULL, which seems conceptually cleaner.

Not sure. There's already at least one counter example:

pg_my_temp_schema() oid OID of session's temporary schema, or 0 if none

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp

On Wed, 22 Jan 2014 20:04:12 +0900 (JST)
Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:

> > On Tue, Jan 14, 2014 at 9:28 AM, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
> >> Here is the patch to implement to_regclass, to_regproc, to_regoper,
> >> and to_regtype.
> >
> > + static Datum regclass_guts(char *class_name_or_oid, bool raiseError);
> >
> > Minor bikeshedding, a lot of code currently uses an argument named
> > "missing_ok" for this purpose (with inverse meaning of course). Any
> > reasons why you chose "raiseError" instead?
>
> Originally the proposal checks errors like syntactical one in addition
> to missing objects. So I think "raiseError" was more appropriate at
> that time. Now they only check missing objects. So renaming to
> "missing_ok" could be more appropriate.
>
> > I only had a brief look at the patch, so maybe I'm missing something.
> > But I don't think you should create 3 variants of these functions:
> > * parseTypeString calls parseTypeString_guts with false
> > * parseTypeStringMissingOk calls parseTypeString_guts with true
> > * parseTypeString_guts
> >
> > And this is just silly:
> >
> > if (raiseError)
> > parseTypeString(typ_name_or_oid, &result, &typmod);
> > else
> > parseTypeStringMissingOk(typ_name_or_oid, &result, &typmod);
> >
> > Just add an argument to parseTypeString and patch all the callers.
>
> Leave the disccusion to Yugo..

parseTypeString() is called by some other functions and I avoided
influences of modifying the definition on them, since this should
raise errors in most cases. This is same reason for other *MissingOk
functions in parse_type.c.

Is it better to write definitions of these function and all there callers?

>
> >> if requested object is not found,
> >> returns InvalidOid, rather than raises an error.
> >
> > I thought the consensus was that returning NULL is better than
> > InvalidOid? From an earlier message:
> >
> > On Thu, Dec 5, 2013 at 5:25 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> >> Another advantage of this approach is that, IIUC, type input functions
> >> can't return a NULL value. So 'pg_klass'::regclass could return 0,
> >> but not NULL. On the other hand, toregclass('pg_klass') *could*
> >> return NULL, which seems conceptually cleaner.
>
> Not sure. There's already at least one counter example:
>
> pg_my_temp_schema() oid OID of session's temporary schema, or 0 if none
>
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese: http://www.sraoss.co.jp

--
Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>

On Wed, Jan 22, 2014 at 1:44 PM, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
> On Wed, 22 Jan 2014 20:04:12 +0900 (JST)
> Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:
> parseTypeString() is called by some other functions and I avoided
> influences of modifying the definition on them, since this should
> raise errors in most cases. This is same reason for other *MissingOk
> functions in parse_type.c.
>
> Is it better to write definitions of these function and all there callers?

Yes, for parseTypeString certainly. There have been many refactorings
like that in the past and all of them use this pattern.

typenameTypeIdAndMod is less clear since the code paths differ so
much, maybe keep 2 versions (merging back to 1 function is OK too, but
in any case you don't need 3).

typenameTypeIdAndModMissingOk(...)
{
Type tup = LookupTypeName(pstate, typeName, typmod_p);
if (tup == NULL || !((Form_pg_type) GETSTRUCT(tup))->typisdefined)
*typeid_p = InvalidOid;
else
*typeid_p = HeapTupleGetOid(tup);

if (tup)
ReleaseSysCache(tup);
}
typenameTypeIdAndMod(...)
{
Type tup = typenameType(pstate, typeName, typmod_p);
*typeid_p = HeapTupleGetOid(tup);
ReleaseSysCache(tup);
}

----

Also, there's no need for "else" here:
if (raiseError)
ereport(ERROR, ...);
else
return InvalidOid;

Regards,
Marti

On 22 January 2014 13:09, Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:

> > I, as a user would be happier if we also have to_regprocedure() and
> > to_regoperator(). The following query looks a valid use-case where one
> > needs to find if a particular function exists. Using to_regproc('sum')
> does
> > not make sense here because it will return InvalidOid, which will not
> tell
> > us whether that is because there is no such function or whether there are
> > duplicate function names.
> > select * from pg_proc where oid = to_regprocedure('sum(int)');
>
> I doubt the value of the use case above. Hasn't psql already done an
> excellent job?
>
> test=# \df sum
> List of functions
> Schema | Name | Result data type | Argument data types | Type
> ------------+------+------------------+---------------------+------
> pg_catalog | sum | numeric | bigint | agg
> pg_catalog | sum | double precision | double precision | agg
> pg_catalog | sum | bigint | integer | agg
> pg_catalog | sum | interval | interval | agg
> pg_catalog | sum | money | money | agg
> pg_catalog | sum | numeric | numeric | agg
> pg_catalog | sum | real | real | agg
> pg_catalog | sum | bigint | smallint | agg
> (8 rows)
>
> If you need simliar functionality in the backend, you could always
> define a view using the query generated by psql.
>
> ********* QUERY **********
> SELECT n.nspname as "Schema",
> p.proname as "Name",
> pg_catalog.pg_get_function_result(p.oid) as "Result data type",
> pg_catalog.pg_get_function_arguments(p.oid) as "Argument data types",
> CASE
> WHEN p.proisagg THEN 'agg'
> WHEN p.proiswindow THEN 'window'
> WHEN p.prorettype = 'pg_catalog.trigger'::pg_catalog.regtype THEN
> 'trigger'
> ELSE 'normal'
> END as "Type"
> FROM pg_catalog.pg_proc p
> LEFT JOIN pg_catalog.pg_namespace n ON n.oid = p.pronamespace
> WHERE p.proname ~ '^(sum)$'
> AND pg_catalog.pg_function_is_visible(p.oid)
> ORDER BY 1, 2, 4;
> **************************
>

I thought the general use case is to be able to use such a functionality
using SQL queries (as against \df), so that the DBA can automate things,
without having to worry about the query returning error. And hence, I
thought to_regprocedure() can be used in a query just like how
::regprocedure is used.

> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese: http://www.sraoss.co.jp
>

On Thu, 23 Jan 2014 13:19:37 +0200
Marti Raudsepp <marti(at)juffo(dot)org> wrote:

> Resending to Tatsuo Ishii and Yugo Nagata, your email server was
> having problems yesterday:

Thanks for resending!

>
> This is the mail system at host sraigw2.sra.co.jp.
>
> <yugo-n(at)sranhm(dot)sra(dot)co(dot)jp>: mail for srasce.sra.co.jp loops back to myself
> <t-ishii(at)sra(dot)co(dot)jp>: mail for srasce.sra.co.jp loops back to myself
>
> ---------- Forwarded message ----------
> From: Marti Raudsepp <marti(at)juffo(dot)org>
> Date: Thu, Jan 23, 2014 at 3:39 AM
> Subject: Re: [HACKERS] Proposal: variant of regclass
> To: Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>
> Cc: Tatsuo Ishii <ishii(at)postgresql(dot)org>, pgsql-hackers
> <pgsql-hackers(at)postgresql(dot)org>, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>,
> Robert Haas <robertmhaas(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>,
> Pavel Golub <pavel(at)gf(dot)microolap(dot)com>, Pavel Golub
> <pavel(at)microolap(dot)com>, Andres Freund <andres(at)2ndquadrant(dot)com>, Pavel
> Stěhule <pavel(dot)stehule(at)gmail(dot)com>
>
>
> On Wed, Jan 22, 2014 at 1:44 PM, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
> > On Wed, 22 Jan 2014 20:04:12 +0900 (JST)
> > Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:
> > parseTypeString() is called by some other functions and I avoided
> > influences of modifying the definition on them, since this should
> > raise errors in most cases. This is same reason for other *MissingOk
> > functions in parse_type.c.
> >
> > Is it better to write definitions of these function and all there callers?
>
> Yes, for parseTypeString certainly. There have been many refactorings
> like that in the past and all of them use this pattern.

Ok. I'll rewrite the definition and there callers.

>
> typenameTypeIdAndMod is less clear since the code paths differ so
> much, maybe keep 2 versions (merging back to 1 function is OK too, but
> in any case you don't need 3).

I'll also fix this in either way to not use typenameTypeIdAndMod_guts.

>
> typenameTypeIdAndModMissingOk(...)
> {
> Type tup = LookupTypeName(pstate, typeName, typmod_p);
> if (tup == NULL || !((Form_pg_type) GETSTRUCT(tup))->typisdefined)
> *typeid_p = InvalidOid;
> else
> *typeid_p = HeapTupleGetOid(tup);
>
> if (tup)
> ReleaseSysCache(tup);
> }
> typenameTypeIdAndMod(...)
> {
> Type tup = typenameType(pstate, typeName, typmod_p);
> *typeid_p = HeapTupleGetOid(tup);
> ReleaseSysCache(tup);
> }
>
> ----
>
> Also, there's no need for "else" here:
> if (raiseError)
> ereport(ERROR, ...);
> else
> return InvalidOid;
>
> Regards,
> Marti

--
Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>

Hi Amit and Marti,

I revised the patch. Could you please review this?

The fixes include:

- Fix *_guts() function definition to return Oid instead of Datum

- Fix to_regproc() definition in pg_proc.h

- Fix some indentation

- Add regression test

- Fix to use missing_ok instead of raiseError

- Merge parseTypeString

- Remove *_guts() and *MissingOk() and merge to one function about
parseTypeString and typenameTypeIdAndMod

- Fix to not raise error even when schema name doesn't exist

This is a new from the previous patch. In previous, specifying wrong schema
name raises an error like:

=# SELECT to_regproc('ng_catalog.now');
ERROR : schema "ng_catalog" doew not exist

On Fri, 24 Jan 2014 12:35:27 +0900
Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:

> On Thu, 23 Jan 2014 13:19:37 +0200
> Marti Raudsepp <marti(at)juffo(dot)org> wrote:
>
> > Resending to Tatsuo Ishii and Yugo Nagata, your email server was
> > having problems yesterday:
>
> Thanks for resending!
>
> >
> > This is the mail system at host sraigw2.sra.co.jp.
> >
> > <yugo-n(at)sranhm(dot)sra(dot)co(dot)jp>: mail for srasce.sra.co.jp loops back to myself
> > <t-ishii(at)sra(dot)co(dot)jp>: mail for srasce.sra.co.jp loops back to myself
> >
> > ---------- Forwarded message ----------
> > From: Marti Raudsepp <marti(at)juffo(dot)org>
> > Date: Thu, Jan 23, 2014 at 3:39 AM
> > Subject: Re: [HACKERS] Proposal: variant of regclass
> > To: Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>
> > Cc: Tatsuo Ishii <ishii(at)postgresql(dot)org>, pgsql-hackers
> > <pgsql-hackers(at)postgresql(dot)org>, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>,
> > Robert Haas <robertmhaas(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>,
> > Pavel Golub <pavel(at)gf(dot)microolap(dot)com>, Pavel Golub
> > <pavel(at)microolap(dot)com>, Andres Freund <andres(at)2ndquadrant(dot)com>, Pavel
> > Stěhule <pavel(dot)stehule(at)gmail(dot)com>
> >
> >
> > On Wed, Jan 22, 2014 at 1:44 PM, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
> > > On Wed, 22 Jan 2014 20:04:12 +0900 (JST)
> > > Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:
> > > parseTypeString() is called by some other functions and I avoided
> > > influences of modifying the definition on them, since this should
> > > raise errors in most cases. This is same reason for other *MissingOk
> > > functions in parse_type.c.
> > >
> > > Is it better to write definitions of these function and all there callers?
> >
> > Yes, for parseTypeString certainly. There have been many refactorings
> > like that in the past and all of them use this pattern.
>
> Ok. I'll rewrite the definition and there callers.
>
> >
> > typenameTypeIdAndMod is less clear since the code paths differ so
> > much, maybe keep 2 versions (merging back to 1 function is OK too, but
> > in any case you don't need 3).
>
> I'll also fix this in either way to not use typenameTypeIdAndMod_guts.
>
> >
> > typenameTypeIdAndModMissingOk(...)
> > {
> > Type tup = LookupTypeName(pstate, typeName, typmod_p);
> > if (tup == NULL || !((Form_pg_type) GETSTRUCT(tup))->typisdefined)
> > *typeid_p = InvalidOid;
> > else
> > *typeid_p = HeapTupleGetOid(tup);
> >
> > if (tup)
> > ReleaseSysCache(tup);
> > }
> > typenameTypeIdAndMod(...)
> > {
> > Type tup = typenameType(pstate, typeName, typmod_p);
> > *typeid_p = HeapTupleGetOid(tup);
> > ReleaseSysCache(tup);
> > }
> >
> > ----
> >
> > Also, there's no need for "else" here:
> > if (raiseError)
> > ereport(ERROR, ...);
> > else
> > return InvalidOid;
> >
> > Regards,
> > Marti
>
>
> --
> Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--
Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>

There are duplicate oids in pg_proc.h :

make[3]: Entering directory `/tmp/git-pg/src/backend/catalog'
cd ../../../src/include/catalog && '/usr/bin/X11/perl' ./duplicate_oids
3180
3195
3196
3197

-------------

There is a whitespace diff in regoperatorin and regprocedurein() definition.

------------

Other than this, there are no more issues from my side. I have only checked
the part of the patch that was modified as per *my* review comments. I will
leave the rest of the review for the other reviewer.

On 28 January 2014 14:08, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:

> Hi Amit and Marti,
>
> I revised the patch. Could you please review this?
>
> The fixes include:
>
> - Fix *_guts() function definition to return Oid instead of Datum
>
> - Fix to_regproc() definition in pg_proc.h
>
> - Fix some indentation
>
> - Add regression test
>
> - Fix to use missing_ok instead of raiseError
>
> - Merge parseTypeString
>
> - Remove *_guts() and *MissingOk() and merge to one function about
> parseTypeString and typenameTypeIdAndMod
>
> - Fix to not raise error even when schema name doesn't exist
>
> This is a new from the previous patch. In previous, specifying wrong
> schema
> name raises an error like:
>
> =# SELECT to_regproc('ng_catalog.now');
> ERROR : schema "ng_catalog" doew not exist
>
>
> On Fri, 24 Jan 2014 12:35:27 +0900
> Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
>
> > On Thu, 23 Jan 2014 13:19:37 +0200
> > Marti Raudsepp <marti(at)juffo(dot)org> wrote:
> >
> > > Resending to Tatsuo Ishii and Yugo Nagata, your email server was
> > > having problems yesterday:
> >
> > Thanks for resending!
> >
> > >
> > > This is the mail system at host sraigw2.sra.co.jp.
> > >
> > > <yugo-n(at)sranhm(dot)sra(dot)co(dot)jp>: mail for srasce.sra.co.jp loops back to
> myself
> > > <t-ishii(at)sra(dot)co(dot)jp>: mail for srasce.sra.co.jp loops back to myself
> > >
> > > ---------- Forwarded message ----------
> > > From: Marti Raudsepp <marti(at)juffo(dot)org>
> > > Date: Thu, Jan 23, 2014 at 3:39 AM
> > > Subject: Re: [HACKERS] Proposal: variant of regclass
> > > To: Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>
> > > Cc: Tatsuo Ishii <ishii(at)postgresql(dot)org>, pgsql-hackers
> > > <pgsql-hackers(at)postgresql(dot)org>, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>,
> > > Robert Haas <robertmhaas(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>,
> > > Pavel Golub <pavel(at)gf(dot)microolap(dot)com>, Pavel Golub
> > > <pavel(at)microolap(dot)com>, Andres Freund <andres(at)2ndquadrant(dot)com>, Pavel
> > > Stěhule <pavel(dot)stehule(at)gmail(dot)com>
> > >
> > >
> > > On Wed, Jan 22, 2014 at 1:44 PM, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>
> wrote:
> > > > On Wed, 22 Jan 2014 20:04:12 +0900 (JST)
> > > > Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:
> > > > parseTypeString() is called by some other functions and I avoided
> > > > influences of modifying the definition on them, since this should
> > > > raise errors in most cases. This is same reason for other *MissingOk
> > > > functions in parse_type.c.
> > > >
> > > > Is it better to write definitions of these function and all there
> callers?
> > >
> > > Yes, for parseTypeString certainly. There have been many refactorings
> > > like that in the past and all of them use this pattern.
> >
> > Ok. I'll rewrite the definition and there callers.
> >
> > >
> > > typenameTypeIdAndMod is less clear since the code paths differ so
> > > much, maybe keep 2 versions (merging back to 1 function is OK too, but
> > > in any case you don't need 3).
> >
> > I'll also fix this in either way to not use typenameTypeIdAndMod_guts.
> >
> > >
> > > typenameTypeIdAndModMissingOk(...)
> > > {
> > > Type tup = LookupTypeName(pstate, typeName, typmod_p);
> > > if (tup == NULL || !((Form_pg_type) GETSTRUCT(tup))->typisdefined)
> > > *typeid_p = InvalidOid;
> > > else
> > > *typeid_p = HeapTupleGetOid(tup);
> > >
> > > if (tup)
> > > ReleaseSysCache(tup);
> > > }
> > > typenameTypeIdAndMod(...)
> > > {
> > > Type tup = typenameType(pstate, typeName, typmod_p);
> > > *typeid_p = HeapTupleGetOid(tup);
> > > ReleaseSysCache(tup);
> > > }
> > >
> > > ----
> > >
> > > Also, there's no need for "else" here:
> > > if (raiseError)
> > > ereport(ERROR, ...);
> > > else
> > > return InvalidOid;
> > >
> > > Regards,
> > > Marti
> >
> >
> > --
> > Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>
> >
> >
> > --
> > Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> > To make changes to your subscription:
> > http://www.postgresql.org/mailpref/pgsql-hackers
>
>
> --
> Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

Hi Amit,

Thanks for your reviewing. I updated the patch.
I fixed the oids and removed the witespace.

On Fri, 31 Jan 2014 16:09:35 +0530
Amit Khandekar <amit(dot)khandekar(at)enterprisedb(dot)com> wrote:

> There are duplicate oids in pg_proc.h :
>
> make[3]: Entering directory `/tmp/git-pg/src/backend/catalog'
> cd ../../../src/include/catalog && '/usr/bin/X11/perl' ./duplicate_oids
> 3180
> 3195
> 3196
> 3197
>
> -------------
>
> There is a whitespace diff in regoperatorin and regprocedurein() definition.
>
> ------------
>
> Other than this, there are no more issues from my side. I have only checked
> the part of the patch that was modified as per *my* review comments. I will
> leave the rest of the review for the other reviewer.
>
>
>
>
> On 28 January 2014 14:08, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
>
> > Hi Amit and Marti,
> >
> > I revised the patch. Could you please review this?
> >
> > The fixes include:
> >
> > - Fix *_guts() function definition to return Oid instead of Datum
> >
> > - Fix to_regproc() definition in pg_proc.h
> >
> > - Fix some indentation
> >
> > - Add regression test
> >
> > - Fix to use missing_ok instead of raiseError
> >
> > - Merge parseTypeString
> >
> > - Remove *_guts() and *MissingOk() and merge to one function about
> > parseTypeString and typenameTypeIdAndMod
> >
> > - Fix to not raise error even when schema name doesn't exist
> >
> > This is a new from the previous patch. In previous, specifying wrong
> > schema
> > name raises an error like:
> >
> > =# SELECT to_regproc('ng_catalog.now');
> > ERROR : schema "ng_catalog" doew not exist
> >
> >
> > On Fri, 24 Jan 2014 12:35:27 +0900
> > Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
> >
> > > On Thu, 23 Jan 2014 13:19:37 +0200
> > > Marti Raudsepp <marti(at)juffo(dot)org> wrote:
> > >
> > > > Resending to Tatsuo Ishii and Yugo Nagata, your email server was
> > > > having problems yesterday:
> > >
> > > Thanks for resending!
> > >
> > > >
> > > > This is the mail system at host sraigw2.sra.co.jp.
> > > >
> > > > <yugo-n(at)sranhm(dot)sra(dot)co(dot)jp>: mail for srasce.sra.co.jp loops back to
> > myself
> > > > <t-ishii(at)sra(dot)co(dot)jp>: mail for srasce.sra.co.jp loops back to myself
> > > >
> > > > ---------- Forwarded message ----------
> > > > From: Marti Raudsepp <marti(at)juffo(dot)org>
> > > > Date: Thu, Jan 23, 2014 at 3:39 AM
> > > > Subject: Re: [HACKERS] Proposal: variant of regclass
> > > > To: Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>
> > > > Cc: Tatsuo Ishii <ishii(at)postgresql(dot)org>, pgsql-hackers
> > > > <pgsql-hackers(at)postgresql(dot)org>, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>,
> > > > Robert Haas <robertmhaas(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>,
> > > > Pavel Golub <pavel(at)gf(dot)microolap(dot)com>, Pavel Golub
> > > > <pavel(at)microolap(dot)com>, Andres Freund <andres(at)2ndquadrant(dot)com>, Pavel
> > > > Stěhule <pavel(dot)stehule(at)gmail(dot)com>
> > > >
> > > >
> > > > On Wed, Jan 22, 2014 at 1:44 PM, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>
> > wrote:
> > > > > On Wed, 22 Jan 2014 20:04:12 +0900 (JST)
> > > > > Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:
> > > > > parseTypeString() is called by some other functions and I avoided
> > > > > influences of modifying the definition on them, since this should
> > > > > raise errors in most cases. This is same reason for other *MissingOk
> > > > > functions in parse_type.c.
> > > > >
> > > > > Is it better to write definitions of these function and all there
> > callers?
> > > >
> > > > Yes, for parseTypeString certainly. There have been many refactorings
> > > > like that in the past and all of them use this pattern.
> > >
> > > Ok. I'll rewrite the definition and there callers.
> > >
> > > >
> > > > typenameTypeIdAndMod is less clear since the code paths differ so
> > > > much, maybe keep 2 versions (merging back to 1 function is OK too, but
> > > > in any case you don't need 3).
> > >
> > > I'll also fix this in either way to not use typenameTypeIdAndMod_guts.
> > >
> > > >
> > > > typenameTypeIdAndModMissingOk(...)
> > > > {
> > > > Type tup = LookupTypeName(pstate, typeName, typmod_p);
> > > > if (tup == NULL || !((Form_pg_type) GETSTRUCT(tup))->typisdefined)
> > > > *typeid_p = InvalidOid;
> > > > else
> > > > *typeid_p = HeapTupleGetOid(tup);
> > > >
> > > > if (tup)
> > > > ReleaseSysCache(tup);
> > > > }
> > > > typenameTypeIdAndMod(...)
> > > > {
> > > > Type tup = typenameType(pstate, typeName, typmod_p);
> > > > *typeid_p = HeapTupleGetOid(tup);
> > > > ReleaseSysCache(tup);
> > > > }
> > > >
> > > > ----
> > > >
> > > > Also, there's no need for "else" here:
> > > > if (raiseError)
> > > > ereport(ERROR, ...);
> > > > else
> > > > return InvalidOid;
> > > >
> > > > Regards,
> > > > Marti
> > >
> > >
> > > --
> > > Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>
> > >
> > >
> > > --
> > > Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> > > To make changes to your subscription:
> > > http://www.postgresql.org/mailpref/pgsql-hackers
> >
> >
> > --
> > Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>
> >
> >
> > --
> > Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> > To make changes to your subscription:
> > http://www.postgresql.org/mailpref/pgsql-hackers
> >
> >

--
Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>

On Fri, Jan 31, 2014 at 6:31 AM, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
> Hi Amit,
>
> Thanks for your reviewing. I updated the patch.
> I fixed the oids and removed the witespace.

This patch contains several whitespace-only hunks. Please revert them.

I don't like the changes to typenameTypeIdAndMod(). The code for the
missing_ok case shouldn't look completely different from the code for
the !missing_ok case. It would be cleaner to start by duplicating
typenameType into typenameTypeIdAndMod and then adjusting it as needed
to support the new argument. It might be better still to just change
parseTypeString() to call LookupTypeName() directly, and add the
necessary logic to handle missing_ok there. The advantage of that is
that you wouldn't need to modify everybody who is calling
typenameTypeIdAndMod(); there are a decent number of such callers
here, and there might be third-party code calling that as well.

I think the changes this patch makes to OpernameGetCandidates() need a
bit of further consideration. The new argument is called missing_ok,
but OpernameGetCandidates() can already return an empty list. What
that new argument does is tolerate a missing schema name. So we could
call the argument missing_schema_ok, I guess, but I'm still not sure I
like that. I don't have a better proposal right at the moment,
though.

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

On Tue, Jan 28, 2014 at 10:38 AM, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
> I revised the patch. Could you please review this?

I didn't test the patch due to the duplicate OID compilation error.
But a few things stuck out from the diffs:
* You added some unnecessary spaces at the beginning of the linein
OpernameGetCandidates.
* regclass_guts and regtype_guts can be simplified further by moving
the ereport() code into regclassin, thereby saving the "if
(missing_ok)"
* I would rephrase the documentation paragraph as:

to_regclass, to_regproc, to_regoper and to_regtype are functions
similar to the regclass, regproc, regoper and regtype casts, except
that they return InvalidOid (0) when the object is not found, instead
of raising an error.

On Wed, Jan 22, 2014 at 1:04 PM, Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:
>> I thought the consensus was that returning NULL is better than
>> InvalidOid? From an earlier message:

> Not sure. There's already at least one counter example:
>
> pg_my_temp_schema() oid OID of session's temporary schema, or 0 if none

And there are pg_relation_filenode, pg_stat_get_backend_dbid,
pg_stat_get_backend_userid which return NULL::oid. In general I don't
like magic values like 0 in SQL. For example, this query would give
unexpected results because InvalidOid has some other meaning:

select * from pg_aggregate where aggfinalfn=to_regproc('typo');

Regards,
Marti

Thanks for your a lot of comments. I revised the patch according to
comments from Robert Haas and Marti Raudsepp.

I changed to_reg* functions to return NULL if the object isn't found.
They return InvalidOid(0) when '0' is passed as parameter, of course.

I also tested this in bootstrap mode by editting postgres.bki as:

create pg_test 10000 without_oids
(
oper = regoper,
proc = regproc,
class = regclass,
type = regtype
)
open pg_test
insert (0,0,0,0)
insert ("||/", "now", "pg_class", "int4")
close pg_test

Regards,

Yugo Nagata

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> This patch contains several whitespace-only hunks. Please revert them.
>
> I don't like the changes to typenameTypeIdAndMod(). The code for the
> missing_ok case shouldn't look completely different from the code for
> the !missing_ok case. It would be cleaner to start by duplicating
> typenameType into typenameTypeIdAndMod and then adjusting it as needed
> to support the new argument. It might be better still to just change
> parseTypeString() to call LookupTypeName() directly, and add the
> necessary logic to handle missing_ok there. The advantage of that is
> that you wouldn't need to modify everybody who is calling
> typenameTypeIdAndMod(); there are a decent number of such callers
> here, and there might be third-party code calling that as well.
>
> I think the changes this patch makes to OpernameGetCandidates() need a
> bit of further consideration. The new argument is called missing_ok,
> but OpernameGetCandidates() can already return an empty list. What
> that new argument does is tolerate a missing schema name. So we could
> call the argument missing_schema_ok, I guess, but I'm still not sure I
> like that. I don't have a better proposal right at the moment,
> though.

On Thu, 6 Feb 2014 21:25:01 +0200
Marti Raudsepp <marti(at)juffo(dot)org> wrote:
> * You added some unnecessary spaces at the beginning of the linein
> OpernameGetCandidates.
> * regclass_guts and regtype_guts can be simplified further by moving
> the ereport() code into regclassin, thereby saving the "if
> (missing_ok)"
> * I would rephrase the documentation paragraph as:
>
> to_regclass, to_regproc, to_regoper and to_regtype are functions
> similar to the regclass, regproc, regoper and regtype casts, except
> that they return InvalidOid (0) when the object is not found, instead
> of raising an error.
>
> On Wed, Jan 22, 2014 at 1:04 PM, Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:
> >> I thought the consensus was that returning NULL is better than
> >> InvalidOid? From an earlier message:
>
> > Not sure. There's already at least one counter example:
> >
> > pg_my_temp_schema() oid OID of session's temporary schema, or 0 if none
>
> And there are pg_relation_filenode, pg_stat_get_backend_dbid,
> pg_stat_get_backend_userid which return NULL::oid. In general I don't
> like magic values like 0 in SQL. For example, this query would give
> unexpected results because InvalidOid has some other meaning:
>
> select * from pg_aggregate where aggfinalfn=to_regproc('typo');
>
> Regards,
> Marti
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--
Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>

On Wed, Feb 26, 2014 at 11:56 AM, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
> Thanks for your a lot of comments. I revised the patch according to
> comments from Robert Haas and Marti Raudsepp.

I have started looking into this patch and below are my
initial findings:

1. Dependency is not recorded when to_regclass is used,
however it is recorded when ::regclass is used. Below is
test to demonstrate this point. This change in behaviour is
neither discussed nor mentioned in docs along with patch.

usage of ::regclass

create sequence my_seq;
create table t1 (c1 int, c2 int default 'my_seq'::regclass);
insert into t1 values(1);
insert into t1 values(2);
select * from t1;
c1 | c2
----+-------
1 | 16390
2 | 16390
(2 rows)

drop sequence my_seq;
ERROR: cannot drop sequence my_seq because other
objects depend on it
DETAIL: default for table t1 column c2 depends on
sequence my_seq
HINT: Use DROP ... CASCADE to drop the dependent
objects too.

Check pg_depend has entry for dependency of default
value of table-column on seq.

postgres=# select * from pg_depend where refobjid = 16390;
classid | objid | objsubid | refclassid | refobjid | refobjsubid | deptype
---------+-------+----------+------------+----------+-------------+---------
1247 | 16391 | 0 | 1259 | 16390 | 0 | i
2604 | 16395 | 0 | 1259 | 16390 | 0 | n
(2 rows)

postgres=# select oid,adrelid from pg_attrdef;
oid | adrelid
-------+---------
16395 | 16392
(1 row)

usage of to_regclass

create sequence my_seq;
create table t1 (c1 int, c2 int default to_regclass('my_seq'));
insert into t1 values(1);
insert into t1 values(2);
select * from t1;
c1 | c2
----+-------
1 | 16396
2 | 16396

select * from pg_depend where refobjid = 16396;
classid | objid | objsubid | refclassid | refobjid | refobjsubid | deptype
---------+-------+----------+------------+----------+-------------+---------
1247 | 16397 | 0 | 1259 | 16396 | 0 | i
(1 row)

There is only one entry for pg_type which means the
dependent object on sequence is only its type, so it will
allow to drop sequence.

postgres=# drop sequence my_seq;
DROP SEQUENCE

2.
! if (!((Form_pg_type) GETSTRUCT(tup))->typisdefined)
! ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_OBJECT),
! errmsg("type \"%s\" is only a shell",
! TypeNameToString(typeName)),
! parser_errposition(NULL, typeName->location)));

In this case it is not exactly same as object does not exist
so I think we should not avoid error in this case
and infact OID is valid for such a case, but in else part
you have assigned it as InvalidOid which seems to be wrong.

3.
regproc_guts()
! {
! if (!missing_ok)
! ereport(ERROR,
! (errcode(ERRCODE_AMBIGUOUS_FUNCTION),
! errmsg("more than one function named \"%s\"",
! pro_name_or_oid)));
! return false;
! }

In to_regproc(), this patch is trying to supress error other
"object-name-not-found" which as far as I could read the thread
was not the original idea and even the description in docs
says only about "object-name-not-found" case.
Doc Description
+ similar to the regclass, regproc, regoper and regtype casts, except
+ that they return NULL when the object is not found, instead of raising
+ an error.

Even if you want to avoid the other error('s) like
above, then I think it's better to mention the same in docs.

I am bit worried, how is user going to distinguish between
the cases when object-not-found and more-than-one-object.

I think such a problem would not arise if we write a function
for regprocedure rather than for regproc, also manual says regprocedure
is more appropriate for most usecases.
http://www.postgresql.org/docs/devel/static/datatype-oid.html

Also I think one other advantage for doing so is that we
don't need to pass missing_ok paramter to some of the functions
like FuncnameGetCandidates(), OpernameGetCandidates().

I understand that you might be using regproc/regoper or might have
a usecase for it, but is it possible for you to use regprocedure/regoperator
instead of regproc/regoper?

4.
+ <entry><type>regclass</type></entry>
+ <entry>get the table oid</entry>

Shouldn't it be better to describe it as "get the relation oid" as
it can give oid for other objects like index as well.

5.
+ <para>
+ to_regclass, to_regproc, to_regoper and to_regtype are functions
+ similar to the regclass, regproc, regoper and regtype casts, except

In above description, it is better to use 'object identifier types'
rather than 'casts'.

6.
! * Guts of regoperin and to_regproc.
Here it should be regprocin

7.
* If not found and missing_ok is true, returns false instead of raising
an error.

Above line is more than 80 chars, it should be less than
80 char limit. This needs to be corrected whereever this line is used.

8.
! * Guts of regtypein and to_regtype.
! * If the classname is found, returns true and the OID is stored into
*typid_p.

typo in *If the classname is found*, it should be type is found.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Sat, Mar 22, 2014 at 1:17 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Wed, Feb 26, 2014 at 11:56 AM, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
>> Thanks for your a lot of comments. I revised the patch according to
>> comments from Robert Haas and Marti Raudsepp.
>
> I have started looking into this patch and below are my
> initial findings:
>
> 1. Dependency is not recorded when to_regclass is used,
> however it is recorded when ::regclass is used. Below is
> test to demonstrate this point. This change in behaviour is
> neither discussed nor mentioned in docs along with patch.

I think this is expected as per current design, because for
functions, it will create dependency on function (funcid), but
not on it's argument values. So I think for this behaviour, we might
need to mention in docs that to_regclass() and other newly added
functions will behave differently for creation of dependencies.

Anyone has any objection for this behaviour difference between
usage of ::regclass and to_regclass()?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Sun, Mar 23, 2014 at 7:57 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> Anyone has any objection for this behaviour difference between
> usage of ::regclass and to_regclass()?

No, I think that makes a lot of sense given the behavior -- if the
object is not there, to_regclass() just returns NULL. It doesn't
require the object to be present, it should not create a dependency.

This allows you to, for example, drop and recreate sequences while
tables are still using them.

Regards,
Marti

On Sat, Mar 22, 2014 at 1:17 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Wed, Feb 26, 2014 at 11:56 AM, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
>> Thanks for your a lot of comments. I revised the patch according to
>> comments from Robert Haas and Marti Raudsepp.
>
> I have started looking into this patch and below are my
> initial findings:

I have looked further into this patch and below are some more findings.
This completes my review for this patch.

1.
regclass_guts(..)
{
..
if (HeapTupleIsValid(tuple = systable_getnext(sysscan)))
*classid_p = HeapTupleGetOid(tuple);
else
return false;

/* We assume there can be only one match */

systable_endscan(sysscan);
heap_close(hdesc, AccessShareLock);

}

In case tuple is not found, false is returned without closing the scan and
relation, now if error is returned it might be okay, because it will release
lock during abort, but if error is not returned (in case of to_regclass),
it will be considered as leak. I think it might not effect anything directly,
because we are not using to_regclass() in Bootstrap mode, but still I feel
it is better to avoid such a leak in API. We can handle it similar to how it
is done in regproc_guts(). The similar improvement is required in
regtype_guts().

2.
! OpernameGetCandidates(List *names, char oprkind, bool missing_schema_ok)
{
..
! namespaceId = LookupExplicitNamespace(schemaname, missing_schema_ok);
! if (!OidIsValid(namespaceId))
! return NULL;

}

In this check it is better to check missing_schema_ok along with
invalid oid check, before returning NULL.

3.
/*
* to_regproc - converts "proname" to proc OID
*
* Diffrence from regprocin is, this does not throw an error and returns NULL
* if the proname does not exist.
* Note: this is not an I/O function.
*/

I think function header comments can be improved for all (to_*) newly
added functions. You can refer function header comments for
simple_heap_insert
which explains it's difference from heap_insert.

4. Oid's used for newly added functions became duplicate after a recent checkin.

5. This file is divided into /* SUPPORT ROUTINES*/ and USER I/O ROUTINES
isn't it better to move _guts functions into Support Routines.

6.
! parseTypeString(const char *str, Oid *typeid_p, int32 *typmod_p,
bool missing_ok)

Line length greater than 80

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Hi Amit Kapila,

Thank you for your reviewing. I updated the patch to v5.
This differences from the previous version are:

- Revise documents and comments acording with your reviews
- Fix not to avoid an error in case of "type xxxx is only a shell"
- Fix regclass_guts and regtype_guts to close the scan and relation when the
object is not found in bootstrap mode
- Fix OpernameGetCandidates to check missing_schema_ok before returning NULL
- Move _guts functions into /* SUPPORT ROUTINES */ section
- Fix oids for recent master.

With Regards,
Yugo Nagata

On Sun, 30 Mar 2014 09:26:42 +0530
Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Sat, Mar 22, 2014 at 1:17 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > On Wed, Feb 26, 2014 at 11:56 AM, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
> >> Thanks for your a lot of comments. I revised the patch according to
> >> comments from Robert Haas and Marti Raudsepp.
> >
> > I have started looking into this patch and below are my
> > initial findings:
>
> I have looked further into this patch and below are some more findings.
> This completes my review for this patch.
>
> 1.
> regclass_guts(..)
> {
> ..
> if (HeapTupleIsValid(tuple = systable_getnext(sysscan)))
> *classid_p = HeapTupleGetOid(tuple);
> else
> return false;
>
> /* We assume there can be only one match */
>
> systable_endscan(sysscan);
> heap_close(hdesc, AccessShareLock);
>
> }
>
> In case tuple is not found, false is returned without closing the scan and
> relation, now if error is returned it might be okay, because it will release
> lock during abort, but if error is not returned (in case of to_regclass),
> it will be considered as leak. I think it might not effect anything directly,
> because we are not using to_regclass() in Bootstrap mode, but still I feel
> it is better to avoid such a leak in API. We can handle it similar to how it
> is done in regproc_guts(). The similar improvement is required in
> regtype_guts().
>
> 2.
> ! OpernameGetCandidates(List *names, char oprkind, bool missing_schema_ok)
> {
> ..
> ! namespaceId = LookupExplicitNamespace(schemaname, missing_schema_ok);
> ! if (!OidIsValid(namespaceId))
> ! return NULL;
>
> }
>
> In this check it is better to check missing_schema_ok along with
> invalid oid check, before returning NULL.
>
> 3.
> /*
> * to_regproc - converts "proname" to proc OID
> *
> * Diffrence from regprocin is, this does not throw an error and returns NULL
> * if the proname does not exist.
> * Note: this is not an I/O function.
> */
>
> I think function header comments can be improved for all (to_*) newly
> added functions. You can refer function header comments for
> simple_heap_insert
> which explains it's difference from heap_insert.
>
>
> 4. Oid's used for newly added functions became duplicate after a recent checkin.
>
> 5. This file is divided into /* SUPPORT ROUTINES*/ and USER I/O ROUTINES
> isn't it better to move _guts functions into Support Routines.
>
> 6.
> ! parseTypeString(const char *str, Oid *typeid_p, int32 *typmod_p,
> bool missing_ok)
>
> Line length greater than 80
>
>
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com

--
Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>

On Mon, Mar 31, 2014 at 7:08 PM, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
> Hi Amit Kapila,
>
> Thank you for your reviewing. I updated the patch to v5.

I have checked the latest version and found few minor improvements that
are required:

1.
! if (!missing_ok)
! ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_OBJECT),
! errmsg("type \"%s\" does not exist",
! TypeNameToString(typeName)),
! parser_errposition(NULL, typeName->location)));

pfree(buf.data); seems to be missing in error cases.
Do you see any problem if we call it before calling LookupTypeName()
instead of calling at multiple places?

2.
+ raising an error. In addition, neither <function>to_regproc</function> nor
+ <function>to_regoper</function> doesn't raise an error when more than one
+ object are found.

No need to use word *doesn't* in above sentence.

3.
+ * If the type name is not found, return InvalidOid if missing_ok
+ * = true, otherwise raise an error.

I can understand above comment, but I think it is better to improve it
by reffering other such instances. I like the explanation of missing_ok
in function header of relation_openrv_extended(). Could you please check
other places and improve this comment.

4. How is user going to distinguish between the cases when object-not-found
and more-than-one-object.
Do you think such a distinction is not required for user's of this API?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Apr 2, 2014 at 1:41 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Mon, Mar 31, 2014 at 7:08 PM, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
>> Hi Amit Kapila,
>>
>> Thank you for your reviewing. I updated the patch to v5.
>
> I have checked the latest version and found few minor improvements that
> are required:
>
> 1.
> ! if (!missing_ok)
> ! ereport(ERROR,
> ! (errcode(ERRCODE_UNDEFINED_OBJECT),
> ! errmsg("type \"%s\" does not exist",
> ! TypeNameToString(typeName)),
> ! parser_errposition(NULL, typeName->location)));
>
> pfree(buf.data); seems to be missing in error cases.

Eh, surely this is being done in some memory context that an error
will reset anyway?

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

On Thu, Apr 3, 2014 at 5:43 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Apr 2, 2014 at 1:41 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> On Mon, Mar 31, 2014 at 7:08 PM, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
>>> Hi Amit Kapila,
>>>
>>> Thank you for your reviewing. I updated the patch to v5.
>>
>> I have checked the latest version and found few minor improvements that
>> are required:
>>
>> 1.
>> ! if (!missing_ok)
>> ! ereport(ERROR,
>> ! (errcode(ERRCODE_UNDEFINED_OBJECT),
>> ! errmsg("type \"%s\" does not exist",
>> ! TypeNameToString(typeName)),
>> ! parser_errposition(NULL, typeName->location)));
>>
>> pfree(buf.data); seems to be missing in error cases.
>
> Eh, surely this is being done in some memory context that an error
> will reset anyway?

Right, it will get reset in error. However still we need to free for missing_ok
case and when it is successful in getting typeid. So don't you think it is
better to just free once before calling LookupTypeName()?

The code is right in it's current form as well, it's just a minor suggestion
for improvement, so if you think current way the code written is okay, then
ignore this suggestion.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Apr 2, 2014 at 11:27 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> Right, it will get reset in error. However still we need to free for missing_ok
> case and when it is successful in getting typeid. So don't you think it is
> better to just free once before calling LookupTypeName()?
>
> The code is right in it's current form as well, it's just a minor suggestion
> for improvement, so if you think current way the code written is okay, then
> ignore this suggestion.

I see. Here's an updated patch with a bit of minor refactoring to
clean that up, and some improvements to the documentation.

I was all ready to commit this when I got cold feet. What's bothering
me is that the patch, as written, mimics the exact behavior of the
text->regproc cast, including the fact that the supplying an OID,
written as a number, will always return that OID, whether it exists or
not:

rhaas=# select to_regclass('1259'), to_regclass('pg_class');
to_regclass | to_regclass
-------------+-------------
pg_class | pg_class
(1 row)

rhaas=# select to_regclass('12590'), to_regclass('does_not_exist');
to_regclass | to_regclass
-------------+-------------
12590 |
(1 row)

I think that's unacceptably weird behavior. My suggestion is to
restructure the code so that to_regclass() only accepts a name, not an
OID, and make its charter precisely to perform a name lookup and
return an OID (as regclass) or NULL if there's no match.

Another thing I noticed is that you'll still get an error from
to_regtype() if the target type is a shell type:

rhaas=# create type i_am_a_shell;
CREATE TYPE
rhaas=# select 'i_am_a_shell'::regtype;
ERROR: type "i_am_a_shell" is only a shell
LINE 1: select 'i_am_a_shell'::regtype;
^
rhaas=# select to_regtype('i_am_a_shell');
ERROR: type "i_am_a_shell" is only a shell

Since the point here is to ignore errors that would otherwise be
caused by types not existing, ignoring errors from a type being a
shell (i.e. barely existing) seems like a possibly good idea.

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

On Fri, Apr 4, 2014 at 8:48 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I see. Here's an updated patch with a bit of minor refactoring to
> clean that up, and some improvements to the documentation.
>
> I was all ready to commit this when I got cold feet. What's bothering
> me is that the patch, as written, mimics the exact behavior of the
> text->regproc cast, including the fact that the supplying an OID,
> written as a number, will always return that OID, whether it exists or
> not:
>
> rhaas=# select to_regclass('1259'), to_regclass('pg_class');
> to_regclass | to_regclass
> -------------+-------------
> pg_class | pg_class
> (1 row)
>
> rhaas=# select to_regclass('12590'), to_regclass('does_not_exist');
> to_regclass | to_regclass
> -------------+-------------
> 12590 |
> (1 row)
>
> I think that's unacceptably weird behavior.

Agreed. Actually I had also noticed this behaviour, but ignored it thinking
that we should just consider behavior change for to_ function incase name
is passed. However after you pointed, it looks pretty wrong for OID cases.

The reason of this behavior is that in out functions (regclassout), we return
the OID as it is incase it doesn't exist. One way to fix this is incase of
OID input parameters, we check if the passed OID exists in to_* functions
and return NULL if it doesn't exist. I think by this way we can retain
the similarity of input parameters between types and to_* functions and
making to_* functions return NULL incase OID doesn't exist makes it
similar to case when user passed name.

> My suggestion is to
> restructure the code so that to_regclass() only accepts a name, not an
> OID, and make its charter precisely to perform a name lookup and
> return an OID (as regclass) or NULL if there's no match.

How will we restrict user from passing some number in string form?
Do you mean that incase user has passed OID, to_* functions should
return NULL or if found that it is OID then return error incase of to_*
functions?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Sat, Apr 5, 2014 at 1:10 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> The reason of this behavior is that in out functions (regclassout), we return
> the OID as it is incase it doesn't exist. One way to fix this is incase of
> OID input parameters, we check if the passed OID exists in to_* functions
> and return NULL if it doesn't exist. I think by this way we can retain
> the similarity of input parameters between types and to_* functions and
> making to_* functions return NULL incase OID doesn't exist makes it
> similar to case when user passed name.

We could do that, but that's not my preferred fix.

>> My suggestion is to
>> restructure the code so that to_regclass() only accepts a name, not an
>> OID, and make its charter precisely to perform a name lookup and
>> return an OID (as regclass) or NULL if there's no match.
>
> How will we restrict user from passing some number in string form?
> Do you mean that incase user has passed OID, to_* functions should
> return NULL or if found that it is OID then return error incase of to_*
> functions?

Each of the _guts functions first handles the case where the input is
exactly "-"; then it checks for an all-numeric value, which is
interpreted an OID; then it has a lengthy chunk of logic to handle the
case where we're in bootstrap mode; and if none of those cases handle
the situation, then it ends by doing the lookup in the "normal" way,
in each case marked with a comment that says "Normal case". I think
that we should do only the last part - what in each case follows the
"normal case" comment - for the to_reg* functions.

In other words, let's revert the whole refactoring of this file to
create reg*_guts functions, and instead just copy the relevant logic
for the name lookups into the new functions. For to_regproc(), for
example, it would look like this (untested):

names = stringToQualifiedNameList(pro_name_or_oid);
clist = FuncnameGetCandidates(names, -1, NIL, false, false, false);
if (clist == NULL || clist->next != NULL)
result = InvalidOid;
else
result = clist->oid;

With that change, this patch will actually get a whole lot smaller,
change less already-existing code, and deliver cleaner behavior.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> In other words, let's revert the whole refactoring of this file to
> create reg*_guts functions, and instead just copy the relevant logic
> for the name lookups into the new functions.

The main discomfort I'd had with this patch was the amount of refactoring
it did; that made it hard to verify and I wasn't feeling like it made for
much net improvement in cleanliness. If we can do it without that, and
have only as much duplicate code as you suggest, then +1.

regards, tom lane

On 2014-04-04 11:18:10 -0400, Robert Haas wrote:
> On Wed, Apr 2, 2014 at 11:27 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > Right, it will get reset in error. However still we need to free for missing_ok
> > case and when it is successful in getting typeid. So don't you think it is
> > better to just free once before calling LookupTypeName()?
> >
> > The code is right in it's current form as well, it's just a minor suggestion
> > for improvement, so if you think current way the code written is okay, then
> > ignore this suggestion.
>
> I see. Here's an updated patch with a bit of minor refactoring to
> clean that up, and some improvements to the documentation.
>
> I was all ready to commit this when I got cold feet. What's bothering
> me is that the patch, as written, mimics the exact behavior of the
> text->regproc cast, including the fact that the supplying an OID,
> written as a number, will always return that OID, whether it exists or
> not:
>
> rhaas=# select to_regclass('1259'), to_regclass('pg_class');
> to_regclass | to_regclass
> -------------+-------------
> pg_class | pg_class
> (1 row)
>
> rhaas=# select to_regclass('12590'), to_regclass('does_not_exist');
> to_regclass | to_regclass
> -------------+-------------
> 12590 |
> (1 row)
>
> I think that's unacceptably weird behavior. My suggestion is to
> restructure the code so that to_regclass() only accepts a name, not an
> OID, and make its charter precisely to perform a name lookup and
> return an OID (as regclass) or NULL if there's no match.

There's actually another good reason to not copy regclass's behaviour:

postgres=# CREATE TABLE "123"();
CREATE TABLE
postgres=# SELECT '123'::regclass;
regclass
----------
123
(1 row)

I don't think that's fixable for ::regclass, but we shouldn't copy it.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> There's actually another good reason to not copy regclass's behaviour:

> postgres=# CREATE TABLE "123"();
> CREATE TABLE
> postgres=# SELECT '123'::regclass;
> regclass
> ----------
> 123
> (1 row)

> I don't think that's fixable for ::regclass, but we shouldn't copy it.

I think that's not proving what you thought; the case is correctly handled
if you quote:

regression=# create table "123"(z int);
CREATE TABLE
regression=# select '123'::regclass;
regclass
----------
123
(1 row)

regression=# select '"123"'::regclass;
regclass
----------
"123"
(1 row)

But I agree that we don't want these functions accepting numeric OIDs,
even though ::regclass must.

regards, tom lane

On 2014-04-07 12:59:36 -0400, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > There's actually another good reason to not copy regclass's behaviour:
>
> > postgres=# CREATE TABLE "123"();
> > CREATE TABLE
> > postgres=# SELECT '123'::regclass;
> > regclass
> > ----------
> > 123
> > (1 row)
>
> > I don't think that's fixable for ::regclass, but we shouldn't copy it.
>
> I think that's not proving what you thought; the case is correctly handled
> if you quote:

I know, but it's a pretty easy to make mistake. Many if not most of the
usages of regclass I have seen were wrong in that way.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, 7 Apr 2014 12:00:49 -0400
Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> In other words, let's revert the whole refactoring of this file to
> create reg*_guts functions, and instead just copy the relevant logic
> for the name lookups into the new functions. For to_regproc(), for
> example, it would look like this (untested):
>
> names = stringToQualifiedNameList(pro_name_or_oid);
> clist = FuncnameGetCandidates(names, -1, NIL, false, false, false);
> if (clist == NULL || clist->next != NULL)
> result = InvalidOid;
> else
> result = clist->oid;
>
> With that change, this patch will actually get a whole lot smaller,
> change less already-existing code, and deliver cleaner behavior.

Here is an updated patch. I rewrote regproc.c not to use _guts functions,
and fixed to_reg* not accept a numeric OID. I also updated the documents
and some comments. Is this better than the previous one?

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

--
Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>

On Tue, Apr 8, 2014 at 3:01 AM, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
> On Mon, 7 Apr 2014 12:00:49 -0400
> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> In other words, let's revert the whole refactoring of this file to
>> create reg*_guts functions, and instead just copy the relevant logic
>> for the name lookups into the new functions. For to_regproc(), for
>> example, it would look like this (untested):
>>
>> names = stringToQualifiedNameList(pro_name_or_oid);
>> clist = FuncnameGetCandidates(names, -1, NIL, false, false, false);
>> if (clist == NULL || clist->next != NULL)
>> result = InvalidOid;
>> else
>> result = clist->oid;
>>
>> With that change, this patch will actually get a whole lot smaller,
>> change less already-existing code, and deliver cleaner behavior.
>
> Here is an updated patch. I rewrote regproc.c not to use _guts functions,
> and fixed to_reg* not accept a numeric OID. I also updated the documents
> and some comments. Is this better than the previous one?

Looks good, committed with a bit of further cleanup.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Looks good, committed with a bit of further cleanup.

I had not actually paid attention to the non-regclass parts of this, and
now that I look, I've got to say that it seems borderline insane to have
chosen to implement regproc/regoper rather than regprocedure/regoperator.
The types implemented here are incapable of dealing with overloaded names,
which --- particularly in the operator case --- makes them close to
useless. I don't think this code was ready to commit.

regards, tom lane

On Tue, Apr 8, 2014 at 10:50 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Looks good, committed with a bit of further cleanup.
>
> I had not actually paid attention to the non-regclass parts of this, and
> now that I look, I've got to say that it seems borderline insane to have
> chosen to implement regproc/regoper rather than regprocedure/regoperator.
> The types implemented here are incapable of dealing with overloaded names,
> which --- particularly in the operator case --- makes them close to
> useless. I don't think this code was ready to commit.

Well, I noticed that, too, but I didn't think it was my job to tell
the patch author what functions he should have wanted. A follow-on
patch to add to_regprocedure and to_regoperator wouldn't be much work,
if you want that.

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

On Tue, Apr 8, 2014 at 11:01 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Apr 8, 2014 at 10:50 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> Looks good, committed with a bit of further cleanup.
>>
>> I had not actually paid attention to the non-regclass parts of this, and
>> now that I look, I've got to say that it seems borderline insane to have
>> chosen to implement regproc/regoper rather than regprocedure/regoperator.
>> The types implemented here are incapable of dealing with overloaded names,
>> which --- particularly in the operator case --- makes them close to
>> useless. I don't think this code was ready to commit.
>
> Well, I noticed that, too, but I didn't think it was my job to tell
> the patch author what functions he should have wanted. A follow-on
> patch to add to_regprocedure and to_regoperator wouldn't be much work,
> if you want that.

And here is a patch for that.

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

>> Well, I noticed that, too, but I didn't think it was my job to tell
>> the patch author what functions he should have wanted. A follow-on
>> patch to add to_regprocedure and to_regoperator wouldn't be much work,
>> if you want that.
>
> And here is a patch for that.

Looks good to me except duplicate oids. Included is a patch to fix that.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp

On Wed, Apr 16, 2014 at 3:27 AM, Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:
>>> Well, I noticed that, too, but I didn't think it was my job to tell
>>> the patch author what functions he should have wanted. A follow-on
>>> patch to add to_regprocedure and to_regoperator wouldn't be much work,
>>> if you want that.
>>
>> And here is a patch for that.
>
> Looks good to me except duplicate oids. Included is a patch to fix that.

Committed.

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