Re: patch: General purpose utility functions used by the JSON data type

On Fri, Aug 13, 2010 at 12:55 PM, Joseph Adams
<joeyadams3.14159@gmail.com> wrote:
> Indeed, a built-in JSON data type will certainly not need it.
> However, users may want to return enums from procedures written in C,
> and this function provides a way to do it.

Yeah, but I can't see accepting it on speculation.  We'll add it if
and when it's clear that it will be generally useful.

>> Incidentally, if we were going to accept this function, I think we'd
>> need to add some kind of a check to throw an error if any of the
>> labels can't be mapped onto an Oid.  Otherwise, an error in this area
>> might lead to difficult-to-find misbehavior.
>
> I agree.  Perhaps an ereport(ERROR, ...) would be better, since it
> would not be hard for a user to cause an enum mapping error (even in a
> production database) by updating a stored procedure in C but not
> updating the corresponding enum (or vice versa, if enum labels are
> renamed).

Right...

> Fair enough.  Perhaps the comment about FN_EXTRA (which explains
> fn_extra in more detail) could be reworded (to talk about using
> fcinfo->flinfo->fn_extra manually) and included in the documentation
> at xfunc-c.html .  fn_extra currently only gets a passing mention in
> the discussion about set-returning functions.

Feel to propose a patch to that comment.

>>> pg_substring, pg_encoding_substring
>>>  * Useful-ometer: ()-------o
>>>  * Rationale: The JSONPath code uses it / will use it for extracting
>>> substrings, which is probably not a very useful feature (but who am I
>>> to say that).  This function could probably benefit the
>>> text_substring() function in varlena.c , but it would take a bit of
>>> work to ensure it continues to comply with standards.
>>
>> I'm more positive about this idea.  If you can make this generally
>> useful, I'd encourage you to do that.  On a random coding style note,
>> I see that you have two copies of the following code, which can fairly
>> obviously be written in a single line rather than five, assuming it's
>> actually safe.
>>
>> +               if (sub_end + len > e)
>> +               {
>> +                       Assert(false);          /* Clipped multibyte
>> character */
>> +                       break;
>> +               }
>
> If I simply say Assert(sub_end + len <= e), the function will yield a
> range hanging off the edge of the input string (out of bounds).  The
> five lines include a safeguard against that when assertion checking is
> off.  This could happen if the input string has a clipped multibyte
> character at the end.  Perhaps I should just drop the assertions and
> make it so if there's a clipped character at the end, it'll be
> ignored, no big deal.

I think maybe what you want is ereport(ERROR).  It should never be
possible for user action to trip an Assert(); Assert() is only to
catch coding mistakes.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company