Horton

Thread

Commits

  1. Remove create_singleton_array(), hard-coding the case in its sole caller.

  2. Improve function header comment for create_singleton_array().

  3. Add three-parameter forms of array_to_string and string_to_array, to allow

  1. Description of create_singleton_array()

    Neha Khatri <nehakhatri5@gmail.com> — 2017-05-01T13:51:02Z

    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.

If it is not intentional then is it fine to update the description like
attached.

Regards,
Neha

  2. Re: Description of create_singleton_array()

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-05-01T19:47:10Z

    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

  3. Re: Description of create_singleton_array()

    Neha Khatri <nehakhatri5@gmail.com> — 2017-05-01T23:26:14Z

    On Tue, May 2, 2017 at 5:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

>
>
> 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.
>

Agreed.

>
> 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.)
>

Ah, yes. The function in itself has the capability to allow m-D array where
'm' could be more than 1. I kind of missed this fact because the only
caller of the function, attempts to create a 1-D array.

>
> 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.


Yesterday, while prying into the definition of this function it did occur
to me that whether there is an additional benefit of this function over
construct_md_array. Yes, it looked like that construct_md_array could be
used in lieu of create_singleton_array.

Regards,
Neha