Re: [HACKERS] get_cast_func syscache utility function

2014-11-05 Thread Andrew Dunstan 


On 11/04/2014 01:45 PM, Tom Lane wrote:

Andrew Dunstan and...@dunslane.net writes:

here's a patch for a utility function to look up the cast function for
a from/to pair of types, as recently suggested by Alvaro. Although it
only contains one use (in json.c), the upcoming jsonb generators would
also use it twice. I'd like to get this committed fairly quickly so I
can prepare an updated patch for the jsonb generators.

I'm not exactly convinced that this is an appropriate utility function,
because casts for which a COERCION_METHOD_FUNCTION entry exists in pg_cast
are by no means the whole universe of casts.  I'm concerned that creating
this function will encourage patch authors to blow off other
possibilities when they should not.  In the particular context at hand,
it seems like you might be better advised to think about how to refactor
json_categorize_type to be more helpful to your new use-case.

A concrete example of what I'm worried about is that json_categorize_type
already ignores the possibility of binary-compatible (WITHOUT FUNCTION)
casts to json.  Since it's eliminated the domain case earlier, that's
perhaps not too horridly broken; but it'd be very easy for new uses of
this get_cast_func() function to overlook such considerations.

In short, I'd rather see this addressed through functions with slightly
higher-level APIs that are capable of covering more cases.  In most cases
it'd be best if callers were using find_coercion_pathway() rather than
taking shortcuts.




Well, then, do we really need a wrapper at all? Should we just be doing 
something like this?


if (typoid = FirstNormalObjectId)
{
Oid castfunc;
CoercionPathType ctype;

ctype = find_coercion_pathway(JSONOID, typoid,
  COERCION_EXPLICIT, castfunc);

if (ctype == COERCION_PATH_FUNC  OidIsValid(castfunc))
{
*tcategory = JSONTYPE_CAST;
*outfuncoid = castfunc;
}
}



cheers

andrew


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

Re: [HACKERS] get_cast_func syscache utility function

2014-11-05 Thread Tom Lane 
Andrew Dunstan and...@dunslane.net writes:
 On 11/04/2014 01:45 PM, Tom Lane wrote:
 In short, I'd rather see this addressed through functions with slightly
 higher-level APIs that are capable of covering more cases.  In most cases
 it'd be best if callers were using find_coercion_pathway() rather than
 taking shortcuts.

 Well, then, do we really need a wrapper at all? Should we just be doing 
 something like this?

  if (typoid = FirstNormalObjectId)
  {
  Oid castfunc;
  CoercionPathType ctype;

  ctype = find_coercion_pathway(JSONOID, typoid,
COERCION_EXPLICIT, 
 castfunc);

  if (ctype == COERCION_PATH_FUNC  OidIsValid(castfunc))
  {
  *tcategory = JSONTYPE_CAST;
  *outfuncoid = castfunc;
  }
  }

Well, of course, the question that immediately raises is why isn't this
code handling the other possible CoercionPathTypes ;-).  But at least
it's pretty obvious from the code that you are ignoring such cases,
so yes I think this is better than what's there now.

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: [HACKERS] get_cast_func syscache utility function

2014-11-05 Thread Andrew Dunstan 


On 11/05/2014 10:10 AM, Tom Lane wrote:

Andrew Dunstan and...@dunslane.net writes:

On 11/04/2014 01:45 PM, Tom Lane wrote:

In short, I'd rather see this addressed through functions with slightly
higher-level APIs that are capable of covering more cases.  In most cases
it'd be best if callers were using find_coercion_pathway() rather than
taking shortcuts.

Well, then, do we really need a wrapper at all? Should we just be doing
something like this?
  if (typoid = FirstNormalObjectId)
  {
  Oid castfunc;
  CoercionPathType ctype;
  ctype = find_coercion_pathway(JSONOID, typoid,
COERCION_EXPLICIT, 
castfunc);
  if (ctype == COERCION_PATH_FUNC  OidIsValid(castfunc))
  {
  *tcategory = JSONTYPE_CAST;
  *outfuncoid = castfunc;
  }
  }

Well, of course, the question that immediately raises is why isn't this
code handling the other possible CoercionPathTypes ;-).  But at least
it's pretty obvious from the code that you are ignoring such cases,
so yes I think this is better than what's there now.


Also, it's equivalent to what's there now, I think. I wasn't intending 
to change the behaviour - if someone wants to do that they can submit a 
separate patch.



cheers

andrew



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

[HACKERS] get_cast_func syscache utility function

2014-11-04 Thread Andrew Dunstan 


here's a patch for a utility function to look up the cast function for a 
from/to pair of types, as recently suggested by Alvaro. Although it only 
contains one use (in json.c), the upcoming jsonb generators would also 
use it twice. I'd like to get this committed fairly quickly so I can 
prepare an updated patch for the jsonb generators.


cheers

andrew


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

Re: [HACKERS] get_cast_func syscache utility function

2014-11-04 Thread Pavel Stehule 
missing patch

Regards

Pavel

2014-11-04 18:57 GMT+01:00 Andrew Dunstan and...@dunslane.net:


 here's a patch for a utility function to look up the cast function for a
 from/to pair of types, as recently suggested by Alvaro. Although it only
 contains one use (in json.c), the upcoming jsonb generators would also use
 it twice. I'd like to get this committed fairly quickly so I can prepare an
 updated patch for the jsonb generators.

 cheers

 andrew


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

Re: [HACKERS] get_cast_func syscache utility function

2014-11-04 Thread Andrew Dunstan 


On 11/04/2014 12:57 PM, Andrew Dunstan wrote:


here's a patch for a utility function to look up the cast function for 
a from/to pair of types, as recently suggested by Alvaro. Although it 
only contains one use (in json.c), the upcoming jsonb generators would 
also use it twice. I'd like to get this committed fairly quickly so I 
can prepare an updated patch for the jsonb generators.





This time with patch.

cheers

andrew
diff --git a/src/backend/utils/adt/json.c b/src/backend/utils/adt/json.c
index d2bf640..fe9cf83 100644
--- a/src/backend/utils/adt/json.c
+++ b/src/backend/utils/adt/json.c
@@ -1299,22 +1299,12 @@ json_categorize_type(Oid typoid,
 /* but let's look for a cast to json, if it's not built-in */
 if (typoid = FirstNormalObjectId)
 {
-	HeapTuple	tuple;
+	Oid castfunc = get_cast_func(typoid, JSONOID);
 
-	tuple = SearchSysCache2(CASTSOURCETARGET,
-			ObjectIdGetDatum(typoid),
-			ObjectIdGetDatum(JSONOID));
-	if (HeapTupleIsValid(tuple))
+	if (OidIsValid(castfunc))
 	{
-		Form_pg_cast castForm = (Form_pg_cast) GETSTRUCT(tuple);
-
-		if (castForm-castmethod == COERCION_METHOD_FUNCTION)
-		{
-			*tcategory = JSONTYPE_CAST;
-			*outfuncoid = castForm-castfunc;
-		}
-
-		ReleaseSysCache(tuple);
+		*tcategory = JSONTYPE_CAST;
+		*outfuncoid = castfunc;
 	}
 }
 			}
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index 552e498..0da08fe 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -21,6 +21,7 @@
 #include bootstrap/bootstrap.h
 #include catalog/pg_amop.h
 #include catalog/pg_amproc.h
+#include catalog/pg_cast.h
 #include catalog/pg_collation.h
 #include catalog/pg_constraint.h
 #include catalog/pg_namespace.h
@@ -2889,3 +2890,36 @@ get_range_subtype(Oid rangeOid)
 	else
 		return InvalidOid;
 }
+
+/*
+ *
+ * get_cast_func(fromtype, totype) - funcoid
+ *
+ * find a cast function if any from fromtype to totype.
+ *
+ * return the Oid of the fucntion found or InvalidOid otherwise.
+ */
+
+Oid
+get_cast_func(Oid from_type, Oid to_type)
+{
+	HeapTuple	tuple;
+	Oid castfunc = InvalidOid;
+
+	tuple = SearchSysCache2(CASTSOURCETARGET,
+			ObjectIdGetDatum(from_type),
+			ObjectIdGetDatum(to_type));
+	if (HeapTupleIsValid(tuple))
+	{
+		Form_pg_cast castForm = (Form_pg_cast) GETSTRUCT(tuple);
+
+		if (castForm-castmethod == COERCION_METHOD_FUNCTION)
+		{
+			castfunc = castForm-castfunc;
+		}
+
+		ReleaseSysCache(tuple);
+	}
+
+	return castfunc;
+}
diff --git a/src/include/utils/lsyscache.h b/src/include/utils/lsyscache.h
index 07d24d4..0291b96 100644
--- a/src/include/utils/lsyscache.h
+++ b/src/include/utils/lsyscache.h
@@ -151,6 +151,7 @@ extern void free_attstatsslot(Oid atttype,
   float4 *numbers, int nnumbers);
 extern char *get_namespace_name(Oid nspid);
 extern Oid	get_range_subtype(Oid rangeOid);
+extern Oid  get_cast_func(Oid from_type, Oid to_type);
 
 #define type_is_array(typid)  (get_element_type(typid) != InvalidOid)
 /* type_is_array_domain accepts both plain arrays and domains over arrays */

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

Re: [HACKERS] get_cast_func syscache utility function

2014-11-04 Thread Tom Lane 
Andrew Dunstan and...@dunslane.net writes:
 here's a patch for a utility function to look up the cast function for 
 a from/to pair of types, as recently suggested by Alvaro. Although it 
 only contains one use (in json.c), the upcoming jsonb generators would 
 also use it twice. I'd like to get this committed fairly quickly so I 
 can prepare an updated patch for the jsonb generators.

I'm not exactly convinced that this is an appropriate utility function,
because casts for which a COERCION_METHOD_FUNCTION entry exists in pg_cast
are by no means the whole universe of casts.  I'm concerned that creating
this function will encourage patch authors to blow off other
possibilities when they should not.  In the particular context at hand,
it seems like you might be better advised to think about how to refactor
json_categorize_type to be more helpful to your new use-case.

A concrete example of what I'm worried about is that json_categorize_type
already ignores the possibility of binary-compatible (WITHOUT FUNCTION)
casts to json.  Since it's eliminated the domain case earlier, that's
perhaps not too horridly broken; but it'd be very easy for new uses of
this get_cast_func() function to overlook such considerations.

In short, I'd rather see this addressed through functions with slightly
higher-level APIs that are capable of covering more cases.  In most cases
it'd be best if callers were using find_coercion_pathway() rather than
taking shortcuts.

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