Re: [HACKERS] Description of create_singleton_array()

[Neha Khatri]

On Tue, May 2, 2017 at 5:47 AM, Tom Lane  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

Re: [HACKERS] Description of create_singleton_array()

[Tom Lane]

Neha Khatri  writes:
> Is it intentional to have the existing $SUBJECT.
> The commit 33f43725
> 
> 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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Description of create_singleton_array()

[Neha Khatri]

Is it intentional to have the existing $SUBJECT.

The commit 33f43725

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


correct-desc-create-singleton-array.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers