On 10/27/2014 05:57 PM, Alvaro Herrera wrote:
> Andrew Dunstan wrote:
>
> This bit:
>
>> +/*
>> + * Determine how we want to render values of a given type in datum_to_jsonb.
>> + *
>> + * Given the datatype OID, return its JsonbTypeCategory, as well as the type's
>> + * output function OID. If the returned category is JSONBTYPE_CAST, we
>> + * return the OID of the type->JSON cast function instead.
>> + */
>> +static void
>> +jsonb_categorize_type(Oid typoid,
>> + JsonbTypeCategory * tcategory,
>> + Oid *outfuncoid)
>> +{
> seems like it can return without having set the category and func OID,
> if there's no available cast. Callers don't seem to check for this
> condition; is this a bug? If not, why not? Maybe some extra comments
> are warranted.
Umm, no. The outfuncoid is set by the call to getTypeOutputInfo() and
the category is set by every branch of the switch. We override the
funcoid in the case where there's a cast to json or jsonb.
I'll add a comment to that effect.
>
> Right now, for the "general case" there, there are two syscache lookups
> rather than one. The fix is simple: just do the getTypeOutputInfo call
> inside each case inside the switch instead of once at the beginning, so
> that the general case can omit it; then there is just one syscache
> access in all the cases. json.c suffers from the same problem.
We only do more than one if it's not a builtin type, or an array or
composite. So 99% of the time this won't even be called.
> Anyway this whole business of searching through the CASTSOURCETARGET
> syscache seems like it could be refactored. If I'm counting correctly,
> that block now appears four times (three in this patch, once in json.c).
> Can't we add a new function to (say) lsyscache and remove that?
Twice, not three times in this patch, unless I'm going crazier than I
thought.
I can add a function to lsyscache along the lines of
Oid get_cast_func(Oid from_type, Oid to_type)
if you think it's worth it.
cheers
andrew
