doc: array_length produces null instead of 0

Hi David,

Per discussion here:

/messages/by-id/163636931138.8076.5140809232053731248@wrigleys.postgresql.org

We can now easily document the array_length behavior of returning null instead of zero for an empty array/dimension.

I added an example to the json_array_length function to demonstrate that it does return 0 as one would expect, but contrary to the SQL array behavior.

I did not bother to add examples to the other half dozen or so "_length" functions that all produce 0 as expected. Just the surprising case and the adjacent one.

Good catch.

+        <literal>array_length(array[], 1)</literal>
+        <returnvalue>NULL</returnvalue>

One tiny nitpick I have is that this example will not work if used
literally, as is:

```
=# select array_length(array[], 1);
ERROR: cannot determine type of empty array
LINE 1: select array_length(array[], 1);
```

Maybe it's worth using `array_length(array[] :: int[], 1)` instead.

--
Best regards,
Aleksander Alekseev

On Tue, Jun 21, 2022 at 6:33 AM Aleksander Alekseev <
aleksander@timescale.com> wrote:

Hi David,

Per discussion here:

/messages/by-id/163636931138.8076.5140809232053731248@wrigleys.postgresql.org

We can now easily document the array_length behavior of returning null

instead of zero for an empty array/dimension.

I added an example to the json_array_length function to demonstrate that

it does return 0 as one would expect, but contrary to the SQL array
behavior.

I did not bother to add examples to the other half dozen or so "_length"

functions that all produce 0 as expected. Just the surprising case and the
adjacent one.

Good catch.

+        <literal>array_length(array[], 1)</literal>
+        <returnvalue>NULL</returnvalue>

One tiny nitpick I have is that this example will not work if used
literally, as is:

```
=# select array_length(array[], 1);
ERROR: cannot determine type of empty array
LINE 1: select array_length(array[], 1);
```

Maybe it's worth using `array_length(array[] :: int[], 1)` instead.

I think subconsciously the cast looked ugly to me so I probably skipped
adding it. I do agree the example should be executable though, and most of
the existing examples use integer[] (not the abbreviated form, int) so I'll
plan to go with that.

Thanks for the review!

David J.

On Tue, Jun 21, 2022 at 09:02:41AM -0700, David G. Johnston wrote:

On Tue, Jun 21, 2022 at 6:33 AM Aleksander Alekseev <aleksander@timescale.com>
Maybe it's worth using `array_length(array[] :: int[], 1)` instead.

I think subconsciously the cast looked ugly to me so I probably skipped adding
it.  I do agree the example should be executable though, and most of the
existing examples use integer[] (not the abbreviated form, int) so I'll plan to
go with that.

Patch applied through PG 13, with adjustments suggested above. Our doc
formatting for pre-PG 13 was too different for me to risk backpatching
further back.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Indecision is a decision. Inaction is an action. Mark Batterson

Hi,

While compiling the PostgreSQL I have found that *memset_s function
requires a define "*__STDC_WANT_LIB_EXT1__*" *

*explicit_bzero.c:* In function ‘*explicit_bzero*’:

*explicit_bzero.c:23:9:* *warning: *implicit declaration of function ‘
*memset_s*’; did you mean ‘*memset*’? [*-Wimplicit-function-declaration*]

(void) *memset_s*(buf, len, 0, len);

*^~~~~~~~*

Attached is the patch to define that in the case of Solaris.

--
Ibrar Ahmed

Ibrar Ahmed <ibrar.ahmad@gmail.com> writes:

While compiling the PostgreSQL I have found that *memset_s function
requires a define "*__STDC_WANT_LIB_EXT1__*" *
*explicit_bzero.c:* In function ‘*explicit_bzero*’:
*explicit_bzero.c:23:9:* *warning: *implicit declaration of function ‘
*memset_s*’; did you mean ‘*memset*’? [*-Wimplicit-function-declaration*]

Hmm.

Attached is the patch to define that in the case of Solaris.

If you don't have any test you want to make before adding the
#define, I don't think this is idiomatic use of autoconf.
Personally I'd have just added "-D__STDC_WANT_LIB_EXT1__" into
the CPPFLAGS for Solaris, perhaps in src/template/solaris,
or maybe just adjust the stanza immediately above this one:

if test "$PORTNAME" = "solaris"; then
CPPFLAGS="$CPPFLAGS -D_POSIX_PTHREAD_SEMANTICS"
fi

regards, tom lane

On Sat, Jul 9, 2022 at 6:46 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Ibrar Ahmed <ibrar.ahmad@gmail.com> writes:

While compiling the PostgreSQL I have found that *memset_s function
requires a define "*__STDC_WANT_LIB_EXT1__*" *
*explicit_bzero.c:* In function ‘*explicit_bzero*’:
*explicit_bzero.c:23:9:* *warning: *implicit declaration of function ‘
*memset_s*’; did you mean ‘*memset*’? [*-Wimplicit-function-declaration*]

Hmm.

Attached is the patch to define that in the case of Solaris.

If you don't have any test you want to make before adding the
#define, I don't think this is idiomatic use of autoconf.
Personally I'd have just added "-D__STDC_WANT_LIB_EXT1__" into
the CPPFLAGS for Solaris, perhaps in src/template/solaris,
or maybe just adjust the stanza immediately above this one:

if test "$PORTNAME" = "solaris"; then
CPPFLAGS="$CPPFLAGS -D_POSIX_PTHREAD_SEMANTICS"
fi

regards, tom lane

Thanks for looking at that, yes you are right, the attached patch do that
now

if test "$PORTNAME" = "solaris"; then

CPPFLAGS="$CPPFLAGS -D_POSIX_PTHREAD_SEMANTICS"

+ CPPFLAGS="$CPPFLAGS -D__STDC_WANT_LIB_EXT1__"

fi

--
Ibrar Ahmed

On Sat, Jul 9, 2022 at 2:02 PM Ibrar Ahmed <ibrar.ahmad@gmail.com> wrote:

Thanks for looking at that, yes you are right, the attached patch do that now

if test "$PORTNAME" = "solaris"; then

CPPFLAGS="$CPPFLAGS -D_POSIX_PTHREAD_SEMANTICS"

+ CPPFLAGS="$CPPFLAGS -D__STDC_WANT_LIB_EXT1__"

fi

Hmm. K.3.3.1 of [1]https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf says you can show or hide all that _s stuff by
defining that macro to 0 or 1 before you include <string.h>, but it's
implementation-defined whether they are exposed by default, and the
template file is one way to deal with that
implementation-definedness... it's not quite in the autoconf spirit
though, it's kinda manual. Another approach would be to define it
unconditionally at the top of explicit_bzero.c before including "c.h",
on all platforms. The man page on my system tells me I should do that
anyway, even though you don't need to on my system.

Why is your Solaris system trying to compile that file in the first
place? A quick check of the Solaris and Illumos build farm animals
and some online man pages tells me they have explicit_bzero().
Ahhh... looks like it came a few years ago in some Solaris 11.4
update[2]https://blogs.oracle.com/solaris/post/expanding-the-library, and Illumos (which forked around 10) probably added it
independently (why do Solaris man pages not have a history section to
tell us these things?!). I guess you must be running an old version.
OK then.

[1]: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf
[2]: https://blogs.oracle.com/solaris/post/expanding-the-library

On Sat, Jul 9, 2022 at 10:28 AM Thomas Munro <thomas.munro@gmail.com> wrote:

On Sat, Jul 9, 2022 at 2:02 PM Ibrar Ahmed <ibrar.ahmad@gmail.com> wrote:

Thanks for looking at that, yes you are right, the attached patch do

that now

if test "$PORTNAME" = "solaris"; then

CPPFLAGS="$CPPFLAGS -D_POSIX_PTHREAD_SEMANTICS"

+ CPPFLAGS="$CPPFLAGS -D__STDC_WANT_LIB_EXT1__"

fi

Hmm. K.3.3.1 of [1] says you can show or hide all that _s stuff by
defining that macro to 0 or 1 before you include <string.h>, but it's
implementation-defined whether they are exposed by default, and the
template file is one way to deal with that
implementation-definedness... it's not quite in the autoconf spirit
though, it's kinda manual. Another approach would be to define it
unconditionally at the top of explicit_bzero.c before including "c.h",
on all platforms. The man page on my system tells me I should do that
anyway, even though you don't need to on my system.

Why is your Solaris system trying to compile that file in the first
place? A quick check of the Solaris and Illumos build farm animals
and some online man pages tells me they have explicit_bzero().
Ahhh... looks like it came a few years ago in some Solaris 11.4
update[2], and Illumos (which forked around 10) probably added it
independently (why do Solaris man pages not have a history section to
tell us these things?!). I guess you must be running an old version.
OK then.

[1] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf
[2] https://blogs.oracle.com/solaris/post/expanding-the-library

I am using "SunOS solaris-vagrant 5.11 11.4.0.15.0 i86pc i386 i86pc", I gave
another thought and Tom is right src/template/solaris is a better place to
add that.

--
Ibrar Ahmed

On Sun, Jul 10, 2022 at 5:47 AM Ibrar Ahmed <ibrar.ahmad@gmail.com> wrote:

I am using "SunOS solaris-vagrant 5.11 11.4.0.15.0 i86pc i386 i86pc",

Hah. So your vagrant image must be from a fairly narrow range of time
when Solaris 11.4 came out with memset_s but didn't yet have
explicit_bzero. That arrived in SRU12 in 2019, which came out before
we started using the function. Real Solaris systems would have
absorbed that via "pkg update", explaining why no one ever noticed
this problem.

I gave
another thought and Tom is right src/template/solaris is a better place to add that.

Something bothers me about adding yet more clutter to every compile
line for the rest of time to solve a problem that exists only for
unpatched systems, and also that it's not even really a Solaris thing,
it's a C11 thing. But I'm not going to object. At least it's
recorded in the archives that it's an obvious candidate to be removed
again in a few years... I was mostly interested in understanding WHY
we suddenly need this...

Thomas Munro <thomas.munro@gmail.com> writes:

Something bothers me about adding yet more clutter to every compile
line for the rest of time to solve a problem that exists only for
unpatched systems, and also that it's not even really a Solaris thing,
it's a C11 thing.

I tend to agree with this standpoint: if it's only a warning, and
it only appears in a small range of not-up-to-date Solaris builds,
then a reasonable approach is "update your system if you don't want
to see the warning".

A positive argument for doing nothing is that there's room to worry
whether -D__STDC_WANT_LIB_EXT1__ might have any side-effects we
*don't* want.

regards, tom lane

On Sun, Jul 10, 2022 at 9:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Thomas Munro <thomas.munro@gmail.com> writes:

Something bothers me about adding yet more clutter to every compile
line for the rest of time to solve a problem that exists only for
unpatched systems, and also that it's not even really a Solaris thing,
it's a C11 thing.

I tend to agree with this standpoint: if it's only a warning, and
it only appears in a small range of not-up-to-date Solaris builds,
then a reasonable approach is "update your system if you don't want
to see the warning".

A positive argument for doing nothing is that there's room to worry
whether -D__STDC_WANT_LIB_EXT1__ might have any side-effects we
*don't* want.

This is still listed in the CF as needing review, so I went and marked
it rejected.

--
John Naylor
EDB: http://www.enterprisedb.com

On Tue, Sep 6, 2022 at 9:24 AM John Naylor <john.naylor@enterprisedb.com>
wrote:

On Sun, Jul 10, 2022 at 9:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Thomas Munro <thomas.munro@gmail.com> writes:

Something bothers me about adding yet more clutter to every compile
line for the rest of time to solve a problem that exists only for
unpatched systems, and also that it's not even really a Solaris thing,
it's a C11 thing.

I tend to agree with this standpoint: if it's only a warning, and
it only appears in a small range of not-up-to-date Solaris builds,
then a reasonable approach is "update your system if you don't want
to see the warning".

A positive argument for doing nothing is that there's room to worry
whether -D__STDC_WANT_LIB_EXT1__ might have any side-effects we
*don't* want.

This is still listed in the CF as needing review, so I went and marked
it rejected.

+1, Thanks

--
John Naylor
EDB: http://www.enterprisedb.com

--
Ibrar Ahmed