Neha Khatri <nehakhatri5@gmail.com> writes:
> Is it intentional to have the existing $SUBJECT.
> The commit 33f43725
>
<https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;f=src/backend/utils/adt/varlena.c;h=33f43725fb5e133454a3e1d0a33bd7cbc75c3c59>
> updated
> the function text_to_array() such that it does not directly invoke
> create_singleton_array(). But $SUBJECT was not updated.
Yeah, that was pretty sloppy.
> If it is not intentional then is it fine to update the description like
> attached.
Well, now that we've been burnt once by the specific call site moving,
I think we should learn from experience and not have this say where
it's called from. That's a lousy substitute for defining the API
expectations explicitly, anyway.
Your proposed patch tries to improve that, but the result isn't
necessarily a "1-D array" --- it's a one-element array, with possibly
a higher number of dimensions than 1. (Not really sure why we thought
flexibility in the number of dimensions was useful, but there it is.)
Actually, the thing that's more important to specify is that the function
insists on using the caller's fcinfo->flinfo->fn_extra. The usage in
text_to_array[_internal] is on the hairy edge of being broken: if that
function were using fn_extra for some other purpose in other code paths,
you could get a core dump or worse from the conflict, because it's
possible for fldsep to vary from empty to non-empty within a single
sequence of calls. That's especially nasty because that would be far from
a mainstream usage, so such a bug could go undetected for a long time.
I wonder if we wouldn't be better off to get rid of this function entirely.
It seems like it's not providing any real increment of simplicity over a
direct call to construct_md_array, since text_to_array could perfectly
well hard-wire the array element storage properties, as we do in very many
other places. And it's a bug waiting to happen, looks like.
I pushed an update to the header comment, but now I'm thinking maybe we
should just get rid of it.
regards, tom lane
