Re: Fwd: [HACKERS] Proposal: variant of regclass

2014-04-08 Thread Yugo Nagata
On Mon, 7 Apr 2014 12:00:49 -0400
Robert Haas robertmh...@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 nag...@sraoss.co.jp


to_regclass.patch.v7
Description: Binary data

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


Re: Fwd: [HACKERS] Proposal: variant of regclass

2014-04-08 Thread Robert Haas
On Tue, Apr 8, 2014 at 3:01 AM, Yugo Nagata nag...@sraoss.co.jp wrote:
 On Mon, 7 Apr 2014 12:00:49 -0400
 Robert Haas robertmh...@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


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


Re: Fwd: [HACKERS] Proposal: variant of regclass

2014-04-08 Thread Tom Lane
Robert Haas robertmh...@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


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


Re: Fwd: [HACKERS] Proposal: variant of regclass

2014-04-08 Thread Robert Haas
On Tue, Apr 8, 2014 at 10:50 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@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


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


Re: Fwd: [HACKERS] Proposal: variant of regclass

2014-04-08 Thread Robert Haas
On Tue, Apr 8, 2014 at 11:01 AM, Robert Haas robertmh...@gmail.com wrote:
 On Tue, Apr 8, 2014 at 10:50 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@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
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 33e093e..e742181 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -15288,10 +15288,18 @@ SELECT pg_type_is_visible('myschema.widget'::regtype);
/indexterm
 
indexterm
+primaryto_regprocedure/primary
+   /indexterm
+
+   indexterm
 primaryto_regoper/primary
/indexterm
 
indexterm
+primaryto_regoperator/primary
+   /indexterm
+
+   indexterm
 primaryto_regtype/primary
/indexterm
 
@@ -15476,11 +15484,21 @@ SELECT pg_type_is_visible('myschema.widget'::regtype);
entryget the oid of the named function/entry
   /row
   row
+   entryliteralfunctionto_regprocedure(parameterfunc_name/parameter)/function/literal/entry
+   entrytyperegprocedure/type/entry
+   entryget the oid of the named function/entry
+  /row
+  row
entryliteralfunctionto_regoper(parameteroperator_name/parameter)/function/literal/entry
entrytyperegoper/type/entry
entryget the oid of the named operator/entry
   /row
   row
+   entryliteralfunctionto_regoperator(parameteroperator_name/parameter)/function/literal/entry
+   entrytyperegoperator/type/entry
+   entryget the oid of the named operator/entry
+  /row
+  row
entryliteralfunctionto_regtype(parametertype_name/parameter)/function/literal/entry
entrytyperegtype/type/entry
entryget the oid of the named type/entry
@@ -15652,10 +15670,12 @@ SELECT collation for ('foo' COLLATE de_DE);
 
   para
The functionto_regclass/function, functionto_regproc/function,
-   functionto_regoper/function and functionto_regtype/function
-   translate relation, function, operator, and type names to objects of
-   type typeregclass/, typeregproc/, typeregoper/ and
-   typeregtype/, respectively.  These functions differ from a cast from
+   functionto_regprocedure/function, functionto_regoper/function,
+   functionto_regoperator/function, and functionto_regtype/function
+   functions translate relation, function, operator, and type names to objects
+   of type typeregclass/, typeregproc/, typeregprocedure/type,
+   typeregoper/, typeregoperator/type, and typeregtype/,
+   respectively.  These functions differ from a cast from
text in that they don't accept a numeric OID, and that they return null
rather than throwing an error if the name is not found (or, for
functionto_regproc/function and functionto_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 regprocedureout for 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 = 

Re: Fwd: [HACKERS] Proposal: variant of regclass

2014-04-07 Thread Robert Haas
On Sat, Apr 5, 2014 at 1:10 AM, Amit Kapila amit.kapil...@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


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


Re: Fwd: [HACKERS] Proposal: variant of regclass

2014-04-07 Thread Tom Lane
Robert Haas robertmh...@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


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


Re: Fwd: [HACKERS] Proposal: variant of regclass

2014-04-07 Thread Andres Freund
On 2014-04-04 11:18:10 -0400, Robert Haas wrote:
 On Wed, Apr 2, 2014 at 11:27 PM, Amit Kapila amit.kapil...@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


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


Re: Fwd: [HACKERS] Proposal: variant of regclass

2014-04-07 Thread Tom Lane
Andres Freund and...@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


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


Re: Fwd: [HACKERS] Proposal: variant of regclass

2014-04-07 Thread Andres Freund
On 2014-04-07 12:59:36 -0400, Tom Lane wrote:
 Andres Freund and...@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


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


Re: Fwd: [HACKERS] Proposal: variant of regclass

2014-04-04 Thread Amit Kapila
On Fri, Apr 4, 2014 at 8:48 PM, Robert Haas robertmh...@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


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


Re: Fwd: [HACKERS] Proposal: variant of regclass

2014-04-02 Thread Robert Haas
On Wed, Apr 2, 2014 at 1:41 AM, Amit Kapila amit.kapil...@gmail.com wrote:
 On Mon, Mar 31, 2014 at 7:08 PM, Yugo Nagata nag...@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


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


Re: Fwd: [HACKERS] Proposal: variant of regclass

2014-04-02 Thread Amit Kapila
On Thu, Apr 3, 2014 at 5:43 AM, Robert Haas robertmh...@gmail.com wrote:
 On Wed, Apr 2, 2014 at 1:41 AM, Amit Kapila amit.kapil...@gmail.com wrote:
 On Mon, Mar 31, 2014 at 7:08 PM, Yugo Nagata nag...@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


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


Re: Fwd: [HACKERS] Proposal: variant of regclass

2014-04-01 Thread Amit Kapila
On Mon, Mar 31, 2014 at 7:08 PM, Yugo Nagata nag...@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 functionto_regproc/function nor
+functionto_regoper/function doesn't raise an error when more than one
+object are found.

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


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

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

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

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


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


Re: Fwd: [HACKERS] Proposal: variant of regclass

2014-03-29 Thread Amit Kapila
On Sat, Mar 22, 2014 at 1:17 PM, Amit Kapila amit.kapil...@gmail.com wrote:
 On Wed, Feb 26, 2014 at 11:56 AM, Yugo Nagata nag...@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


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


Re: Fwd: [HACKERS] Proposal: variant of regclass

2014-03-23 Thread Marti Raudsepp
On Sun, Mar 23, 2014 at 7:57 AM, Amit Kapila amit.kapil...@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


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


Re: Fwd: [HACKERS] Proposal: variant of regclass

2014-03-22 Thread Amit Kapila
On Wed, Feb 26, 2014 at 11:56 AM, Yugo Nagata nag...@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.
+entrytyperegclass/type/entry
+entryget 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 

Re: Fwd: [HACKERS] Proposal: variant of regclass

2014-03-22 Thread Amit Kapila
On Sat, Mar 22, 2014 at 1:17 PM, Amit Kapila amit.kapil...@gmail.com wrote:
 On Wed, Feb 26, 2014 at 11:56 AM, Yugo Nagata nag...@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


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


Re: Fwd: [HACKERS] Proposal: variant of regclass

2014-02-06 Thread Marti Raudsepp
On Tue, Jan 28, 2014 at 10:38 AM, Yugo Nagata nag...@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 is...@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


Re: Fwd: [HACKERS] Proposal: variant of regclass

2014-01-31 Thread Amit Khandekar
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 nag...@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 nag...@sraoss.co.jp wrote:

  On Thu, 23 Jan 2014 13:19:37 +0200
  Marti Raudsepp ma...@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.
  
   yug...@sranhm.sra.co.jp: mail for srasce.sra.co.jp loops back to
 myself
   t-is...@sra.co.jp: mail for srasce.sra.co.jp loops back to myself
  
   -- Forwarded message --
   From: Marti Raudsepp ma...@juffo.org
   Date: Thu, Jan 23, 2014 at 3:39 AM
   Subject: Re: [HACKERS] Proposal: variant of regclass
   To: Yugo Nagata nag...@sraoss.co.jp
   Cc: Tatsuo Ishii is...@postgresql.org, pgsql-hackers
   pgsql-hackers@postgresql.org, Vik Fearing vik.fear...@dalibo.com,
   Robert Haas robertmh...@gmail.com, Tom Lane t...@sss.pgh.pa.us,
   Pavel Golub pa...@gf.microolap.com, Pavel Golub
   pa...@microolap.com, Andres Freund and...@2ndquadrant.com, Pavel
   Stěhule pavel.steh...@gmail.com
  
  
   On Wed, Jan 22, 2014 at 1:44 PM, Yugo Nagata nag...@sraoss.co.jp
 wrote:
On Wed, 22 Jan 2014 20:04:12 +0900 (JST)
Tatsuo Ishii is...@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 nag...@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 nag...@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




Re: Fwd: [HACKERS] Proposal: variant of regclass

2014-01-31 Thread Robert Haas
On Fri, Jan 31, 2014 at 6:31 AM, Yugo Nagata nag...@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


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


Re: Fwd: [HACKERS] Proposal: variant of regclass

2014-01-23 Thread Yugo Nagata
On Thu, 23 Jan 2014 13:19:37 +0200
Marti Raudsepp ma...@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.
 
 yug...@sranhm.sra.co.jp: mail for srasce.sra.co.jp loops back to myself
 t-is...@sra.co.jp: mail for srasce.sra.co.jp loops back to myself
 
 -- Forwarded message --
 From: Marti Raudsepp ma...@juffo.org
 Date: Thu, Jan 23, 2014 at 3:39 AM
 Subject: Re: [HACKERS] Proposal: variant of regclass
 To: Yugo Nagata nag...@sraoss.co.jp
 Cc: Tatsuo Ishii is...@postgresql.org, pgsql-hackers
 pgsql-hackers@postgresql.org, Vik Fearing vik.fear...@dalibo.com,
 Robert Haas robertmh...@gmail.com, Tom Lane t...@sss.pgh.pa.us,
 Pavel Golub pa...@gf.microolap.com, Pavel Golub
 pa...@microolap.com, Andres Freund and...@2ndquadrant.com, Pavel
 Stěhule pavel.steh...@gmail.com
 
 
 On Wed, Jan 22, 2014 at 1:44 PM, Yugo Nagata nag...@sraoss.co.jp wrote:
  On Wed, 22 Jan 2014 20:04:12 +0900 (JST)
  Tatsuo Ishii is...@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 nag...@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