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
