Обсуждение: 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@sraoss.co.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@sraoss.co.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@sraoss.co.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@gf.microolap.com
On 2013-12-04 20:25:53 -0500, Tom Lane wrote: > Tatsuo Ishii <ishii@sraoss.co.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@2ndquadrant.com>
On 2013-12-04 20:25:53 -0500, Tom Lane wrote:
> Tatsuo Ishii <ishii@sraoss.co.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@postgresql.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@2ndquadrant.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@2ndquadrant.com>
On 2013-12-05 11:54:20 +0100, Pavel Stehule wrote:> 2013/12/5 Andres Freund <andres@2ndquadrant.com>Uh. How is that going to help applications that upgraded, without having
> 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
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
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@sraoss.co.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@gf.microolap.com
Pavel Golub <pavel@microolap.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@microolap.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@gmx.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@sss.pgh.pa.us> wrote: > Pavel Golub <pavel@microolap.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@gmail.com> writes: > On Thu, Dec 5, 2013 at 9:41 AM, Tom Lane <tgl@sss.pgh.pa.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@gmail.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@gmx.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@sss.pgh.pa.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@postgresql.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@postgresql.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 diff --git a/src/backend/utils/adt/regproc.c b/src/backend/utils/adt/regproc.c index c24a2c1..d3532d7 100644 --- a/src/backend/utils/adt/regproc.c +++ b/src/backend/utils/adt/regproc.c @@ -45,6 +45,8 @@ static char *format_operator_internal(Oid operator_oid, bool force_qualify);static char *format_procedure_internal(Oidprocedure_oid, bool force_qualify);static void parseNameAndArgTypes(const char *string, boolallowNone, List **names, int *nargs, Oid *argtypes); +static Datum regclass_gut(char *class_name_or_oid, bool raiseError); +static List *stringToQualifiedNameList_gut(const char *string, bool raiseError);/***************************************************************************** @@ -804,21 +806,55 @@ Datumregclassin(PG_FUNCTION_ARGS){ char *class_name_or_oid = PG_GETARG_CSTRING(0); + Oid result; + + result = regclass_gut(class_name_or_oid, true); + PG_RETURN_OID(result); +} + +Datum +toregclass(PG_FUNCTION_ARGS) +{ + char *class_name_or_oid = PG_GETARG_CSTRING(0); + Oid result; + + result = regclass_gut(class_name_or_oid, false); + PG_RETURN_OID(result); +} + +/* + * Gut of regclassin and toregclass. + * If raiseError is false, returns InvalidOid upon error. + */ +static Datum regclass_gut(char *class_name_or_oid, bool raiseError) +{ Oid result = InvalidOid; List *names; /* '-' ? */ if (strcmp(class_name_or_oid, "-") == 0) - PG_RETURN_OID(InvalidOid); + return result; /* Numeric OID? */ if (class_name_or_oid[0] >= '0' && class_name_or_oid[0] <= '9' && strspn(class_name_or_oid, "0123456789") == strlen(class_name_or_oid)) { - result = DatumGetObjectId(DirectFunctionCall1(oidin, - CStringGetDatum(class_name_or_oid))); - PG_RETURN_OID(result); + PG_TRY(); + { + result = DatumGetObjectId(DirectFunctionCall1(oidin, + CStringGetDatum(class_name_or_oid))); + } + PG_CATCH(); + { + if (raiseError) + PG_RE_THROW(); + else + return InvalidOid; + } + PG_END_TRY(); + + return result; } /* Else it's a name, possibly schema-qualified */ @@ -848,28 +884,36 @@ regclassin(PG_FUNCTION_ARGS) if (HeapTupleIsValid(tuple = systable_getnext(sysscan))) result = HeapTupleGetOid(tuple); else - ereport(ERROR, - (errcode(ERRCODE_UNDEFINED_TABLE), - errmsg("relation \"%s\" does not exist", class_name_or_oid))); + if (raiseError) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_TABLE), + errmsg("relation \"%s\" does not exist", class_name_or_oid))); + else + return InvalidOid; /* We assume there can be only one match */ systable_endscan(sysscan); heap_close(hdesc, AccessShareLock); - PG_RETURN_OID(result); + return result; } /* * Normal case: parse the name into components and see if it matches any * pg_classentries in the current search path. */ - names = stringToQualifiedNameList(class_name_or_oid); + names = stringToQualifiedNameList_gut(class_name_or_oid, false); + if (names == NIL) + return InvalidOid; /* We might not even have permissions on this relation; don't lock it. */ - result = RangeVarGetRelid(makeRangeVarFromNameList(names), NoLock, false); + if (raiseError) + result = RangeVarGetRelid(makeRangeVarFromNameList(names), NoLock, false); + else + result = RangeVarGetRelid(makeRangeVarFromNameList(names), NoLock, true); - PG_RETURN_OID(result); + return result;}/* @@ -1352,6 +1396,12 @@ text_regclass(PG_FUNCTION_ARGS)List *stringToQualifiedNameList(const char *string){ + return stringToQualifiedNameList_gut(string, true); +} + +static List * +stringToQualifiedNameList_gut(const char *string, bool raiseError) +{ char *rawname; List *result = NIL; List *namelist; @@ -1361,14 +1411,30 @@ stringToQualifiedNameList(const char *string) rawname = pstrdup(string); if (!SplitIdentifierString(rawname,'.', &namelist)) - ereport(ERROR, - (errcode(ERRCODE_INVALID_NAME), - errmsg("invalid name syntax"))); + { + if (raiseError) + ereport(ERROR, + (errcode(ERRCODE_INVALID_NAME), + errmsg("invalid name syntax"))); + else + { + pfree(rawname); + return result; + } + } if (namelist == NIL) - ereport(ERROR, - (errcode(ERRCODE_INVALID_NAME), - errmsg("invalid name syntax"))); + { + if (raiseError) + ereport(ERROR, + (errcode(ERRCODE_INVALID_NAME), + errmsg("invalid name syntax"))); + else + { + pfree(rawname); + return result; + } + } foreach(l, namelist) { diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h index 0117500..3f00b05 100644 --- a/src/include/catalog/pg_proc.h +++ b/src/include/catalog/pg_proc.h @@ -3304,6 +3304,7 @@ DATA(insert OID = 2218 ( regclassin PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 2DESCR("I/O");DATA(insertOID = 2219 ( regclassout PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 2275 "2205" _null_ _null__null_ _null_ regclassout _null_ _null_ _null_ ));DESCR("I/O"); +DATA(insert OID = 3179 ( toregclass PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 2205 "2275" _null_ _null_ _null_ _null_toregclass _null_ _null_ _null_ ));DATA(insert OID = 2220 ( regtypein PGNSP PGUID 12 1 0 0 0 f f f f tf s 1 0 2206 "2275" _null_ _null_ _null_ _null_ regtypein _null_ _null_ _null_ ));DESCR("I/O");DATA(insert OID = 2221 ( regtypeout PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 2275 "2206" _null_ _null_ _null_ _null_ regtypeout _null_ _null__null_ )); diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h index 1bfd145..1b57a7b 100644 --- a/src/include/utils/builtins.h +++ b/src/include/utils/builtins.h @@ -600,6 +600,7 @@ extern Datum regclassin(PG_FUNCTION_ARGS);extern Datum regclassout(PG_FUNCTION_ARGS);extern Datum regclassrecv(PG_FUNCTION_ARGS);externDatum regclasssend(PG_FUNCTION_ARGS); +extern Datum toregclass(PG_FUNCTION_ARGS);extern Datum regtypein(PG_FUNCTION_ARGS);extern Datum regtypeout(PG_FUNCTION_ARGS);externDatum regtyperecv(PG_FUNCTION_ARGS);
Tatsuo Ishii <ishii@postgresql.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@postgresql.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 diff --git a/src/backend/utils/adt/regproc.c b/src/backend/utils/adt/regproc.c index c24a2c1..0406c30 100644 --- a/src/backend/utils/adt/regproc.c +++ b/src/backend/utils/adt/regproc.c @@ -45,6 +45,7 @@ static char *format_operator_internal(Oid operator_oid, bool force_qualify);static char *format_procedure_internal(Oidprocedure_oid, bool force_qualify);static void parseNameAndArgTypes(const char *string, boolallowNone, List **names, int *nargs, Oid *argtypes); +static Datum regclass_guts(char *class_name_or_oid, bool raiseError);/***************************************************************************** @@ -804,21 +805,50 @@ Datumregclassin(PG_FUNCTION_ARGS){ char *class_name_or_oid = PG_GETARG_CSTRING(0); + Oid result; + + result = regclass_guts(class_name_or_oid, true); + PG_RETURN_OID(result); +} + +/* + * toregclass - converts "classname" to class OID + * + * Diffrence from rgclassin is, this does not throw an error if the classname + * does not exist. + * Note: this is not an I/O function. + */ +Datum +toregclass(PG_FUNCTION_ARGS) +{ + char *class_name_or_oid = PG_GETARG_CSTRING(0); + Oid result; + + result = regclass_guts(class_name_or_oid, false); + PG_RETURN_OID(result); +} + +/* + * Guts of regclassin and toregclass. + * If raiseError is false, returns InvalidOid upon error. + */ +static Datum regclass_guts(char *class_name_or_oid, bool raiseError) +{ Oid result = InvalidOid; List *names; /* '-' ? */ if (strcmp(class_name_or_oid, "-") == 0) - PG_RETURN_OID(InvalidOid); + return result; /* Numeric OID? */ if (class_name_or_oid[0] >= '0' && class_name_or_oid[0] <= '9' && strspn(class_name_or_oid, "0123456789") == strlen(class_name_or_oid)) { - result = DatumGetObjectId(DirectFunctionCall1(oidin, + result = DatumGetObjectId(DirectFunctionCall1(oidin, CStringGetDatum(class_name_or_oid))); - PG_RETURN_OID(result); + return result; } /* Else it's a name, possibly schema-qualified */ @@ -848,16 +878,19 @@ regclassin(PG_FUNCTION_ARGS) if (HeapTupleIsValid(tuple = systable_getnext(sysscan))) result = HeapTupleGetOid(tuple); else - ereport(ERROR, - (errcode(ERRCODE_UNDEFINED_TABLE), - errmsg("relation \"%s\" does not exist", class_name_or_oid))); + if (raiseError) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_TABLE), + errmsg("relation \"%s\" does not exist", class_name_or_oid))); + else + return InvalidOid; /* We assume there can be only one match */ systable_endscan(sysscan); heap_close(hdesc, AccessShareLock); - PG_RETURN_OID(result); + return result; } /* @@ -865,11 +898,16 @@ regclassin(PG_FUNCTION_ARGS) * pg_class entries in the current search path. */ names = stringToQualifiedNameList(class_name_or_oid); + if (names == NIL) + return InvalidOid; /* We might not even have permissions on this relation; don't lock it. */ - result = RangeVarGetRelid(makeRangeVarFromNameList(names), NoLock, false); + if (raiseError) + result = RangeVarGetRelid(makeRangeVarFromNameList(names), NoLock, false); + else + result = RangeVarGetRelid(makeRangeVarFromNameList(names), NoLock, true); - PG_RETURN_OID(result); + return result;}/* diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h index 0117500..472ccad 100644 --- a/src/include/catalog/pg_proc.h +++ b/src/include/catalog/pg_proc.h @@ -3304,13 +3304,14 @@ DATA(insert OID = 2218 ( regclassin PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 2DESCR("I/O");DATA(insertOID = 2219 ( regclassout PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 2275 "2205" _null_ _null__null_ _null_ regclassout _null_ _null_ _null_ ));DESCR("I/O"); +DATA(insert OID = 3179 ( toregclass PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 2205 "2275" _null_ _null_ _null_ _null_toregclass _null_ _null_ _null_ )); +DESCR("convert classname to regclass");DATA(insert OID = 2220 ( regtypein PGNSP PGUID 12 1 0 0 0 f f f f t fs 1 0 2206 "2275" _null_ _null_ _null_ _null_ regtypein _null_ _null_ _null_ ));DESCR("I/O");DATA(insert OID = 2221 ( regtypeout PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 2275 "2206" _null_ _null_ _null_ _null_ regtypeout _null_ _null__null_ ));DESCR("I/O");DATA(insert OID = 1079 ( regclass PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 2205"25" _null_ _null_ _null_ _null_ text_regclass _null_ _null_ _null_ ));DESCR("convert text to regclass"); -DATA(insert OID = 2246 ( fmgr_internal_validator PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 2278 "26" _null_ _null_ _null__null_ fmgr_internal_validator _null_ _null_ _null_ ));DESCR("(internal)");DATA(insert OID = 2247 ( fmgr_c_validator PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 2278 "26" _null_ _null_ _null_ _null_ fmgr_c_validator _null__null_ _null_ )); diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h index 1bfd145..1b57a7b 100644 --- a/src/include/utils/builtins.h +++ b/src/include/utils/builtins.h @@ -600,6 +600,7 @@ extern Datum regclassin(PG_FUNCTION_ARGS);extern Datum regclassout(PG_FUNCTION_ARGS);extern Datum regclassrecv(PG_FUNCTION_ARGS);externDatum regclasssend(PG_FUNCTION_ARGS); +extern Datum toregclass(PG_FUNCTION_ARGS);extern Datum regtypein(PG_FUNCTION_ARGS);extern Datum regtypeout(PG_FUNCTION_ARGS);externDatum regtyperecv(PG_FUNCTION_ARGS);
>> 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@postgresql.org>
> On 12/31/2013 02:38 AM, Tatsuo Ishii wrote:I have no problem with adding "to_". Objection?
>> 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 like to_reg* too
Regards
Pavel
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@postgresql.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.stehule@gmail.com> wrote: > 2013/12/31 Tatsuo Ishii <ishii@postgresql.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@postgresql.org) > > To make changes to your subscription: > > http://www.postgresql.org/mailpref/pgsql-hackers > > -- Yugo Nagata <nagata@sraoss.co.jp>
Вложения
Hi, On Tue, Jan 14, 2014 at 4:28 PM, Yugo Nagata <nagata@sraoss.co.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@sraoss.co.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@sraoss.co.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.stehule@gmail.com> wrote:
> 2013/12/31 Tatsuo Ishii <ishii@postgresql.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@postgresql.org)
> > To make changes to your subscription:
> > http://www.postgresql.org/mailpref/pgsql-hackers
> >
Yugo Nagata <nagata@sraoss.co.jp>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.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 | aggpg_catalog | sum | double precision | double precision | aggpg_catalog | sum | bigint | integer | aggpg_catalog | sum | interval | interval | aggpg_catalog | sum | money | money | aggpg_catalog | sum | numeric | numeric | aggpg_catalog | sum | real | real | aggpg_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@sraoss.co.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@gmail.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@sraoss.co.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@gmail.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@postgresql.org> wrote: > > On Tue, Jan 14, 2014 at 9:28 AM, Yugo Nagata <nagata@sraoss.co.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@gmail.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@sraoss.co.jp>
On Wed, Jan 22, 2014 at 1:44 PM, Yugo Nagata <nagata@sraoss.co.jp> wrote: > On Wed, 22 Jan 2014 20:04:12 +0900 (JST) > Tatsuo Ishii <ishii@postgresql.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@postgresql.org> wrote:
> I, as a user would be happier if we also have to_regprocedure() andI doubt the value of the use case above. Hasn't psql already done an
> 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)');
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@juffo.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@sranhm.sra.co.jp>: mail for srasce.sra.co.jp loops back to myself > <t-ishii@sra.co.jp>: mail for srasce.sra.co.jp loops back to myself > > ---------- Forwarded message ---------- > From: Marti Raudsepp <marti@juffo.org> > Date: Thu, Jan 23, 2014 at 3:39 AM > Subject: Re: [HACKERS] Proposal: variant of regclass > To: Yugo Nagata <nagata@sraoss.co.jp> > Cc: Tatsuo Ishii <ishii@postgresql.org>, pgsql-hackers > <pgsql-hackers@postgresql.org>, Vik Fearing <vik.fearing@dalibo.com>, > Robert Haas <robertmhaas@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>, > Pavel Golub <pavel@gf.microolap.com>, Pavel Golub > <pavel@microolap.com>, Andres Freund <andres@2ndquadrant.com>, Pavel > Stěhule <pavel.stehule@gmail.com> > > > On Wed, Jan 22, 2014 at 1:44 PM, Yugo Nagata <nagata@sraoss.co.jp> wrote: > > On Wed, 22 Jan 2014 20:04:12 +0900 (JST) > > Tatsuo Ishii <ishii@postgresql.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@sraoss.co.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@sraoss.co.jp> wrote: > On Thu, 23 Jan 2014 13:19:37 +0200 > Marti Raudsepp <marti@juffo.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@sranhm.sra.co.jp>: mail for srasce.sra.co.jp loops back to myself > > <t-ishii@sra.co.jp>: mail for srasce.sra.co.jp loops back to myself > > > > ---------- Forwarded message ---------- > > From: Marti Raudsepp <marti@juffo.org> > > Date: Thu, Jan 23, 2014 at 3:39 AM > > Subject: Re: [HACKERS] Proposal: variant of regclass > > To: Yugo Nagata <nagata@sraoss.co.jp> > > Cc: Tatsuo Ishii <ishii@postgresql.org>, pgsql-hackers > > <pgsql-hackers@postgresql.org>, Vik Fearing <vik.fearing@dalibo.com>, > > Robert Haas <robertmhaas@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>, > > Pavel Golub <pavel@gf.microolap.com>, Pavel Golub > > <pavel@microolap.com>, Andres Freund <andres@2ndquadrant.com>, Pavel > > Stěhule <pavel.stehule@gmail.com> > > > > > > On Wed, Jan 22, 2014 at 1:44 PM, Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > On Wed, 22 Jan 2014 20:04:12 +0900 (JST) > > > Tatsuo Ishii <ishii@postgresql.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@sraoss.co.jp> > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers -- Yugo Nagata <nagata@sraoss.co.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@sraoss.co.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@sraoss.co.jp> wrote:
> On Thu, 23 Jan 2014 13:19:37 +0200
> Marti Raudsepp <marti@juffo.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@sranhm.sra.co.jp>: mail for srasce.sra.co.jp loops back to myself
> > <t-ishii@sra.co.jp>: mail for srasce.sra.co.jp loops back to myself
> >
> > ---------- Forwarded message ----------
> > From: Marti Raudsepp <marti@juffo.org>
> > Date: Thu, Jan 23, 2014 at 3:39 AM
> > Subject: Re: [HACKERS] Proposal: variant of regclass
> > To: Yugo Nagata <nagata@sraoss.co.jp>
> > Cc: Tatsuo Ishii <ishii@postgresql.org>, pgsql-hackers
> > <pgsql-hackers@postgresql.org>, Vik Fearing <vik.fearing@dalibo.com>,
> > Robert Haas <robertmhaas@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
> > Pavel Golub <pavel@gf.microolap.com>, Pavel Golub
> > <pavel@microolap.com>, Andres Freund <andres@2ndquadrant.com>, Pavel
> > Stěhule <pavel.stehule@gmail.com>
> >
> >
> > On Wed, Jan 22, 2014 at 1:44 PM, Yugo Nagata <nagata@sraoss.co.jp> wrote:
> > > On Wed, 22 Jan 2014 20:04:12 +0900 (JST)
> > > Tatsuo Ishii <ishii@postgresql.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@sraoss.co.jp>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
Yugo Nagata <nagata@sraoss.co.jp>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.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.khandekar@enterprisedb.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@sraoss.co.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@sraoss.co.jp> wrote: > > > > > On Thu, 23 Jan 2014 13:19:37 +0200 > > > Marti Raudsepp <marti@juffo.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@sranhm.sra.co.jp>: mail for srasce.sra.co.jp loops back to > > myself > > > > <t-ishii@sra.co.jp>: mail for srasce.sra.co.jp loops back to myself > > > > > > > > ---------- Forwarded message ---------- > > > > From: Marti Raudsepp <marti@juffo.org> > > > > Date: Thu, Jan 23, 2014 at 3:39 AM > > > > Subject: Re: [HACKERS] Proposal: variant of regclass > > > > To: Yugo Nagata <nagata@sraoss.co.jp> > > > > Cc: Tatsuo Ishii <ishii@postgresql.org>, pgsql-hackers > > > > <pgsql-hackers@postgresql.org>, Vik Fearing <vik.fearing@dalibo.com>, > > > > Robert Haas <robertmhaas@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>, > > > > Pavel Golub <pavel@gf.microolap.com>, Pavel Golub > > > > <pavel@microolap.com>, Andres Freund <andres@2ndquadrant.com>, Pavel > > > > Stěhule <pavel.stehule@gmail.com> > > > > > > > > > > > > On Wed, Jan 22, 2014 at 1:44 PM, Yugo Nagata <nagata@sraoss.co.jp> > > wrote: > > > > > On Wed, 22 Jan 2014 20:04:12 +0900 (JST) > > > > > Tatsuo Ishii <ishii@postgresql.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@sraoss.co.jp> > > > > > > > > > -- > > > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > > > To make changes to your subscription: > > > http://www.postgresql.org/mailpref/pgsql-hackers > > > > > > -- > > Yugo Nagata <nagata@sraoss.co.jp> > > > > > > -- > > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > > To make changes to your subscription: > > http://www.postgresql.org/mailpref/pgsql-hackers > > > > -- Yugo Nagata <nagata@sraoss.co.jp>
Вложения
On Fri, Jan 31, 2014 at 6:31 AM, Yugo Nagata <nagata@sraoss.co.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@sraoss.co.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@postgresql.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@gmail.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@juffo.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@postgresql.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@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers -- Yugo Nagata <nagata@sraoss.co.jp>
Вложения
On Wed, Feb 26, 2014 at 11:56 AM, Yugo Nagata <nagata@sraoss.co.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.kapila16@gmail.com> wrote: > On Wed, Feb 26, 2014 at 11:56 AM, Yugo Nagata <nagata@sraoss.co.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.kapila16@gmail.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.kapila16@gmail.com> wrote: > On Wed, Feb 26, 2014 at 11:56 AM, Yugo Nagata <nagata@sraoss.co.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 SupportRoutines. 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.kapila16@gmail.com> wrote: > On Sat, Mar 22, 2014 at 1:17 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Wed, Feb 26, 2014 at 11:56 AM, Yugo Nagata <nagata@sraoss.co.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@sraoss.co.jp>
Вложения
On Mon, Mar 31, 2014 at 7:08 PM, Yugo Nagata <nagata@sraoss.co.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 sucha 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.kapila16@gmail.com> wrote: > On Mon, Mar 31, 2014 at 7:08 PM, Yugo Nagata <nagata@sraoss.co.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@gmail.com> wrote: > On Wed, Apr 2, 2014 at 1:41 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> On Mon, Mar 31, 2014 at 7:08 PM, Yugo Nagata <nagata@sraoss.co.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.kapila16@gmail.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@gmail.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.kapila16@gmail.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@gmail.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.kapila16@gmail.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@2ndquadrant.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@2ndquadrant.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@gmail.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@sraoss.co.jp>
Вложения
On Tue, Apr 8, 2014 at 3:01 AM, Yugo Nagata <nagata@sraoss.co.jp> wrote: > On Mon, 7 Apr 2014 12:00:49 -0400 > Robert Haas <robertmhaas@gmail.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@gmail.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@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.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@gmail.com> wrote: > On Tue, Apr 8, 2014 at 10:50 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Robert Haas <robertmhaas@gmail.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 diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 0809a6d..db8691a 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -15294,10 +15294,18 @@ SELECT pg_type_is_visible('myschema.widget'::regtype); </indexterm> <indexterm> + <primary>to_regprocedure</primary> + </indexterm> + + <indexterm> <primary>to_regoper</primary> </indexterm> <indexterm> + <primary>to_regoperator</primary> + </indexterm> + + <indexterm> <primary>to_regtype</primary> </indexterm> @@ -15482,11 +15490,21 @@ SELECT pg_type_is_visible('myschema.widget'::regtype); <entry>get the oid of the named function</entry> </row> <row> + <entry><literal><function>to_regprocedure(<parameter>func_name</parameter>)</function></literal></entry> + <entry><type>regprocedure</type></entry> + <entry>get the oid of the named function</entry> + </row> + <row> <entry><literal><function>to_regoper(<parameter>operator_name</parameter>)</function></literal></entry> <entry><type>regoper</type></entry> <entry>get the oid of the named operator</entry> </row> <row> + <entry><literal><function>to_regoperator(<parameter>operator_name</parameter>)</function></literal></entry> + <entry><type>regoperator</type></entry> + <entry>get the oid of the named operator</entry> + </row> + <row> <entry><literal><function>to_regtype(<parameter>type_name</parameter>)</function></literal></entry> <entry><type>regtype</type></entry> <entry>get the oid of the named type</entry> @@ -15658,10 +15676,12 @@ SELECT collation for ('foo' COLLATE "de_DE"); <para> The <function>to_regclass</function>, <function>to_regproc</function>, - <function>to_regoper</function> and <function>to_regtype</function> - translate relation, function, operator, and type names to objects of - type <type>regclass</>, <type>regproc</>, <type>regoper</> and - <type>regtype</>, respectively. These functions differ from a cast from + <function>to_regprocedure</function>, <function>to_regoper</function>, + <function>to_regoperator</function>, and <function>to_regtype</function> + functions translate relation, function, operator, and type names to objects + of type <type>regclass</>, <type>regproc</>, <type>regprocedure</type>, + <type>regoper</>, <type>regoperator</type>, and <type>regtype</>, + respectively. These functions differ from a cast from text in that they don't accept a numeric OID, and that theyreturn null rather than throwing an error if the name is not found (or, for <function>to_regproc</function> and <function>to_regoper</function>,if diff --git a/src/backend/utils/adt/regproc.c b/src/backend/utils/adt/regproc.c index ed2bdbf..6210f45 100644 --- a/src/backend/utils/adt/regproc.c +++ b/src/backend/utils/adt/regproc.c @@ -323,6 +323,38 @@ regprocedurein(PG_FUNCTION_ARGS)}/* + * to_regprocedure - converts "proname(args)" to proc OID + * + * If the name is not found, we return NULL. + */ +Datum +to_regprocedure(PG_FUNCTION_ARGS) +{ + char *pro_name = PG_GETARG_CSTRING(0); + List *names; + int nargs; + Oid argtypes[FUNC_MAX_ARGS]; + FuncCandidateList clist; + + /* + * Parse the name and arguments, look up potential matches in the current + * namespace search list, and scan to see which one exactly matches the + * given argument types. (There will not be more than one match.) + */ + parseNameAndArgTypes(pro_name, false, &names, &nargs, argtypes); + + clist = FuncnameGetCandidates(names, nargs, NIL, false, false, true); + + for (; clist; clist = clist->next) + { + if (memcmp(clist->args, argtypes, nargs * sizeof(Oid)) == 0) + PG_RETURN_OID(clist->oid); + } + + PG_RETURN_NULL(); +} + +/* * format_procedure - converts proc OID to "pro_name(args)" * * This exports the useful functionality of regprocedureoutfor use @@ -722,6 +754,45 @@ regoperatorin(PG_FUNCTION_ARGS)}/* + * to_regoperator - converts "oprname(args)" to operator OID + * + * If the name is not found, we return NULL. + */ +Datum +to_regoperator(PG_FUNCTION_ARGS) +{ + char *opr_name_or_oid = PG_GETARG_CSTRING(0); + Oid result; + List *names; + int nargs; + Oid argtypes[FUNC_MAX_ARGS]; + + /* + * Parse the name and arguments, look up potential matches in the current + * namespace search list, and scan to see which one exactly matches the + * given argument types. (There will not be more than one match.) + */ + parseNameAndArgTypes(opr_name_or_oid, true, &names, &nargs, argtypes); + if (nargs == 1) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_PARAMETER), + errmsg("missing argument"), + errhint("Use NONE to denote the missing argument of a unary operator."))); + if (nargs != 2) + ereport(ERROR, + (errcode(ERRCODE_TOO_MANY_ARGUMENTS), + errmsg("too many arguments"), + errhint("Provide two argument types for operator."))); + + result = OpernameGetOprid(names, argtypes[0], argtypes[1]); + + if (!OidIsValid(result)) + PG_RETURN_NULL(); + + PG_RETURN_OID(result); +} + +/* * format_operator - converts operator OID to "opr_name(args)" * * This exports the useful functionality of regoperatoroutfor use diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h index 643408d..c77c036 100644 --- a/src/include/catalog/pg_proc.h +++ b/src/include/catalog/pg_proc.h @@ -175,6 +175,8 @@ DATA(insert OID = 45 ( regprocout PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0DESCR("I/O");DATA(insertOID = 3494 ( to_regproc PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 24 "2275" _null_ _null__null_ _null_ to_regproc _null_ _null_ _null_ ));DESCR("convert proname to regproc"); +DATA(insert OID = 3479 ( to_regprocedure PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 2202 "2275" _null_ _null_ _null_ _null_to_regprocedure _null_ _null_ _null_ )); +DESCR("convert proname to regprocedure");DATA(insert OID = 46 ( textin PGNSP PGUID 12 1 0 0 0 f f f f tf i 1 0 25 "2275" _null_ _null_ _null_ _null_ textin _null_ _null_ _null_ ));DESCR("I/O");DATA(insert OID = 47 ( textout PGNSP PGUID 12 1 0 0 0 f f f f t f i 1 0 2275 "25" _null_ _null_ _null_ _null_ textout _null_ _null_ _null_)); @@ -3351,6 +3353,8 @@ DATA(insert OID = 2215 ( regoperout PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 2DESCR("I/O");DATA(insertOID = 3492 ( to_regoper PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 2203 "2275" _null_ _null__null_ _null_ to_regoper _null_ _null_ _null_ ));DESCR("convert operator name to regoper"); +DATA(insert OID = 3476 ( to_regoperator PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 2204 "2275" _null_ _null_ _null_ _null_to_regoperator _null_ _null_ _null_ )); +DESCR("convert operator name to regoperator");DATA(insert OID = 2216 ( regoperatorin PGNSP PGUID 12 1 0 0 0 f ff f t f s 1 0 2204 "2275" _null_ _null_ _null_ _null_ regoperatorin _null_ _null_ _null_ ));DESCR("I/O");DATA(insert OID= 2217 ( regoperatorout PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 2275 "2204" _null_ _null_ _null_ _null_ regoperatorout_null_ _null_ _null_ )); diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h index 11ff4fd..33b6dca 100644 --- a/src/include/utils/builtins.h +++ b/src/include/utils/builtins.h @@ -602,6 +602,7 @@ extern char *regexp_fixed_prefix(text *text_re, bool case_insensitive,extern Datum regprocin(PG_FUNCTION_ARGS);externDatum regprocout(PG_FUNCTION_ARGS);extern Datum to_regproc(PG_FUNCTION_ARGS); +extern Datum to_regprocedure(PG_FUNCTION_ARGS);extern Datum regprocrecv(PG_FUNCTION_ARGS);extern Datum regprocsend(PG_FUNCTION_ARGS);externDatum regprocedurein(PG_FUNCTION_ARGS); @@ -613,6 +614,7 @@ extern Datum regoperout(PG_FUNCTION_ARGS);extern Datum regoperrecv(PG_FUNCTION_ARGS);extern Datum regopersend(PG_FUNCTION_ARGS);externDatum to_regoper(PG_FUNCTION_ARGS); +extern Datum to_regoperator(PG_FUNCTION_ARGS);extern Datum regoperatorin(PG_FUNCTION_ARGS);extern Datum regoperatorout(PG_FUNCTION_ARGS);externDatum regoperatorrecv(PG_FUNCTION_ARGS); diff --git a/src/test/regress/expected/regproc.out b/src/test/regress/expected/regproc.out index 56ab245..3342129 100644 --- a/src/test/regress/expected/regproc.out +++ b/src/test/regress/expected/regproc.out @@ -9,12 +9,24 @@ SELECT regoper('||/'); ||/(1 row) +SELECT regoperator('+(int4,int4)'); + regoperator +-------------------- + +(integer,integer) +(1 row) +SELECT regproc('now'); regproc --------- now(1 row) +SELECT regprocedure('abs(numeric)'); + regprocedure +-------------- + abs(numeric) +(1 row) +SELECT regclass('pg_class'); regclass ---------- @@ -33,12 +45,24 @@ SELECT to_regoper('||/'); ||/(1 row) +SELECT to_regoperator('+(int4,int4)'); + to_regoperator +-------------------- + +(integer,integer) +(1 row) +SELECT to_regproc('now'); to_regproc ------------ now(1 row) +SELECT to_regprocedure('abs(numeric)'); + to_regprocedure +----------------- + abs(numeric) +(1 row) +SELECT to_regclass('pg_class'); to_regclass ------------- @@ -58,12 +82,24 @@ SELECT regoper('pg_catalog.||/'); ||/(1 row) +SELECT regoperator('pg_catalog.+(int4,int4)'); + regoperator +-------------------- + +(integer,integer) +(1 row) +SELECT regproc('pg_catalog.now'); regproc --------- now(1 row) +SELECT regprocedure('pg_catalog.abs(numeric)'); + regprocedure +-------------- + abs(numeric) +(1 row) +SELECT regclass('pg_catalog.pg_class'); regclass ---------- @@ -88,6 +124,12 @@ SELECT to_regproc('pg_catalog.now'); now(1 row) +SELECT to_regprocedure('pg_catalog.abs(numeric)'); + to_regprocedure +----------------- + abs(numeric) +(1 row) +SELECT to_regclass('pg_catalog.pg_class'); to_regclass ------------- @@ -106,10 +148,18 @@ SELECT regoper('||//');ERROR: operator does not exist: ||//LINE 3: SELECT regoper('||//'); ^ +SELECT regoperator('++(int4,int4)'); +ERROR: operator does not exist: ++(int4,int4) +LINE 1: SELECT regoperator('++(int4,int4)'); + ^SELECT regproc('know');ERROR: function "know" does not existLINE 1: SELECT regproc('know'); ^ +SELECT regprocedure('absinthe(numeric)'); +ERROR: function "absinthe(numeric)" does not exist +LINE 1: SELECT regprocedure('absinthe(numeric)'); + ^SELECT regclass('pg_classes');ERROR: relation "pg_classes" does not existLINE 1: SELECT regclass('pg_classes'); @@ -123,10 +173,18 @@ SELECT regoper('ng_catalog.||/');ERROR: schema "ng_catalog" does not existLINE 1: SELECT regoper('ng_catalog.||/'); ^ +SELECT regoperator('ng_catalog.+(int4,int4)'); +ERROR: operator does not exist: ng_catalog.+(int4,int4) +LINE 1: SELECT regoperator('ng_catalog.+(int4,int4)'); + ^SELECT regproc('ng_catalog.now');ERROR: schema "ng_catalog" does not existLINE 1: SELECT regproc('ng_catalog.now'); ^ +SELECT regprocedure('ng_catalog.abs(numeric)'); +ERROR: schema "ng_catalog" does not exist +LINE 1: SELECT regprocedure('ng_catalog.abs(numeric)'); + ^SELECT regclass('ng_catalog.pg_class');ERROR: schema "ng_catalog" does not existLINE 1: SELECTregclass('ng_catalog.pg_class'); @@ -143,12 +201,24 @@ SELECT to_regoper('||//'); (1 row) +SELECT to_regoperator('++(int4,int4)'); + to_regoperator +---------------- + +(1 row) +SELECT to_regproc('know'); to_regproc ------------ (1 row) +SELECT to_regprocedure('absinthe(numeric)'); + to_regprocedure +----------------- + +(1 row) +SELECT to_regclass('pg_classes'); to_regclass ------------- @@ -168,12 +238,24 @@ SELECT to_regoper('ng_catalog.||/'); (1 row) +SELECT to_regoperator('ng_catalog.+(int4,int4)'); + to_regoperator +---------------- + +(1 row) +SELECT to_regproc('ng_catalog.now'); to_regproc ------------ (1 row) +SELECT to_regprocedure('ng_catalog.abs(numeric)'); + to_regprocedure +----------------- + +(1 row) +SELECT to_regclass('ng_catalog.pg_class'); to_regclass ------------- diff --git a/src/test/regress/sql/regproc.sql b/src/test/regress/sql/regproc.sql index 1334cfb..cc90838 100644 --- a/src/test/regress/sql/regproc.sql +++ b/src/test/regress/sql/regproc.sql @@ -7,24 +7,31 @@-- without schemanameSELECT regoper('||/'); +SELECT regoperator('+(int4,int4)');SELECT regproc('now'); +SELECT regprocedure('abs(numeric)');SELECT regclass('pg_class');SELECT regtype('int4');SELECT to_regoper('||/'); +SELECT to_regoperator('+(int4,int4)');SELECT to_regproc('now'); +SELECT to_regprocedure('abs(numeric)');SELECT to_regclass('pg_class');SELECT to_regtype('int4');-- with schemanameSELECTregoper('pg_catalog.||/'); +SELECT regoperator('pg_catalog.+(int4,int4)');SELECT regproc('pg_catalog.now'); +SELECT regprocedure('pg_catalog.abs(numeric)');SELECT regclass('pg_catalog.pg_class');SELECT regtype('pg_catalog.int4');SELECTto_regoper('pg_catalog.||/');SELECT to_regproc('pg_catalog.now'); +SELECT to_regprocedure('pg_catalog.abs(numeric)');SELECT to_regclass('pg_catalog.pg_class');SELECT to_regtype('pg_catalog.int4'); @@ -33,14 +40,18 @@ SELECT to_regtype('pg_catalog.int4');-- without schemanameSELECT regoper('||//'); +SELECT regoperator('++(int4,int4)');SELECT regproc('know'); +SELECT regprocedure('absinthe(numeric)');SELECT regclass('pg_classes');SELECT regtype('int3');-- with schemanameSELECT regoper('ng_catalog.||/'); +SELECT regoperator('ng_catalog.+(int4,int4)');SELECT regproc('ng_catalog.now'); +SELECT regprocedure('ng_catalog.abs(numeric)');SELECT regclass('ng_catalog.pg_class');SELECT regtype('ng_catalog.int4'); @@ -49,13 +60,17 @@ SELECT regtype('ng_catalog.int4');-- without schemanameSELECT to_regoper('||//'); +SELECT to_regoperator('++(int4,int4)');SELECT to_regproc('know'); +SELECT to_regprocedure('absinthe(numeric)');SELECT to_regclass('pg_classes');SELECT to_regtype('int3');-- with schemanameSELECTto_regoper('ng_catalog.||/'); +SELECT to_regoperator('ng_catalog.+(int4,int4)');SELECT to_regproc('ng_catalog.now'); +SELECT to_regprocedure('ng_catalog.abs(numeric)');SELECT to_regclass('ng_catalog.pg_class');SELECT to_regtype('ng_catalog.int4');
On Wed, Apr 16, 2014 at 3:27 AM, Tatsuo Ishii <ishii@postgresql.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