Thread: get_cast_func syscache utility function

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

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

Andrew Dunstan <andrew@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

On 11/04/2014 01:45 PM, Tom Lane wrote:
> Andrew Dunstan <andrew@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;
CoercionPathTypectype;
 
                    ctype = find_coercion_pathway(JSONOID, typoid,
COERCION_EXPLICIT,&castfunc);
 
                    if (ctype == COERCION_PATH_FUNC && OidIsValid(castfunc))                    {
*tcategory = JSONTYPE_CAST;                        *outfuncoid = castfunc;                    }                }
 



cheers

andrew

Andrew Dunstan <andrew@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

On 11/05/2014 10:10 AM, Tom Lane wrote:
> Andrew Dunstan <andrew@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