On 1/3/16 10:23 PM, Pavel Stehule wrote:

2016-01-03 22:49 GMT+01:00 Jim Nasby <jim.na...@bluetreble.com

    On 1/3/16 2:37 PM, Pavel Stehule wrote:

        +               /* num_nulls(VARIADIC NULL) is defined as NULL */
        +               if (PG_ARGISNULL(0))
        +                       return false;

    Could you add to the comment explaining why that's the desired behavior?

This case should be different than num_nulls(VARIADIC ARRAY[..]) - this
situation is really equivalent of missing data and NULL is correct
answer. It should not be too clean in num_nulls, but when it is cleaner
for num_notnulls. And more, it is consistent with other variadic
functions in Postgres: see concat_internal and text_format.

Makes sense, now that you explain it. Which is why I'm thinking it'd be good to add that explanation to the comment... ;)


    Erm... is that really the way to verify that what you have is an
    array? ISTM there should be a macro for that somewhere...

really, it is. It is used more time. Although I am not against some
macro, I don't think so it is necessary. The macro should not be too
shorter than this text.

Well, if there's other stuff doing that... would be nice to refactor that though.

    For brevity and example sake it'd probably be better to just use the
    normal iterator, unless there's a serious speed difference?

The iterator does some memory allocations and some access to type cache.
Almost all work of iterator is useless for this case. This code is
developed by Marko, but I agree with this design. Using the iterator is
big gun for this case. I didn't any performance checks, but it should be
measurable  for any varlena arrays.

Makes sense then.
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

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

Reply via email to