Thread: [COMMITTERS] pgsql: Add pg_strnlen() a portable implementation of strlen.
Add pg_strnlen() a portable implementation of strlen. As the OS version is likely going to be more optimized, fall back to it if available, as detected by configure. Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/8a241792f968ed5be6cf4d41e32c0d264f6c0c65 Modified Files -------------- configure | 2 +- configure.in | 2 +- src/common/string.c | 20 ++++++++++++++++++++ src/include/common/string.h | 15 +++++++++++++++ src/include/pg_config.h.in | 3 +++ src/include/pg_config.h.win32 | 3 +++ src/port/snprintf.c | 12 ++---------- 7 files changed, 45 insertions(+), 12 deletions(-) -- Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
Re: [COMMITTERS] pgsql: Add pg_strnlen() a portable implementationof strlen.
From
Andres Freund
Date:
Hi Andrew, On 2017-10-09 22:22:04 +0000, Andres Freund wrote: > Add pg_strnlen() a portable implementation of strlen. > > As the OS version is likely going to be more optimized, fall back to > it if available, as detected by configure. I'm a bit confused, frogmouth https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=frogmouth&dt=2017-10-09%2022%3A30%3A41 shows that it compiled the new code, but the configure output doesn't show it ran through the new configure test. Additionally, without the the config define, this should result in the replacement being used. Which doesn't seem to be the case either. Kinda sounds like this used some halfway outdated build or such? Greetings, Andres Freund -- Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
Re: [COMMITTERS] pgsql: Add pg_strnlen() a portable implementation ofstrlen.
From
Andrew Dunstan
Date:
On 10/09/2017 07:15 PM, Andres Freund wrote: > Hi Andrew, > > On 2017-10-09 22:22:04 +0000, Andres Freund wrote: >> Add pg_strnlen() a portable implementation of strlen. >> >> As the OS version is likely going to be more optimized, fall back to >> it if available, as detected by configure. > I'm a bit confused, frogmouth > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=frogmouth&dt=2017-10-09%2022%3A30%3A41 > shows that it compiled the new code, but the configure output doesn't > show it ran through the new configure test. Additionally, without the > the config define, this should result in the replacement being > used. Which doesn't seem to be the case either. > > Kinda sounds like this used some halfway outdated build or such? > frogmouth is using some code not yet released that makes the config cache persistent. I just identified and fixed a stupid bug in the code that obsoletes the cache, and I have removed frogmouth's cache file and set it running again, so we'll see if that fixes things. cheers andrew -- Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
Andrew Dunstan <andrew@dunslane.net> writes:
> frogmouth is using some code not yet released that makes the config
> cache persistent. I just identified and fixed a stupid bug in the code
> that obsoletes the cache, and I have removed frogmouth's cache file and
> set it running again, so we'll see if that fixes things.
I doubt it. I think the problem with this patch is that Andres has
made libpgport dependent on libpgcommon, which is backwards, or at
least circular. The module layering is supposed to go the other way.
regards, tom lane
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
Re: [COMMITTERS] pgsql: Add pg_strnlen() a portable implementationof strlen.
From
Andres Freund
Date:
On 2017-10-10 00:25:52 -0400, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > > frogmouth is using some code not yet released that makes the config > > cache persistent. I just identified and fixed a stupid bug in the code > > that obsoletes the cache, and I have removed frogmouth's cache file and > > set it running again, so we'll see if that fixes things. Interesting. Although an outdated cache file doesn't explain why configure didn't even do the relevant check, no? This kinda sounds more like configure as a whole is outdated, rather than it's cache file. > I doubt it. It'll quite possibly fix it, but probably not for good reasons. Presumably after a proper configure run the fallback won't be used anymore. > I think the problem with this patch is that Andres has > made libpgport dependent on libpgcommon, which is backwards, or at > least circular. The module layering is supposed to go the other way. Yes, quite possibly. At least one platform without strnlen [1], as well as my local machine when intentionally marking strnlen as not available, ran successfully. But that's likely just a difference in how/when symbols are resolved. I think the current split between common and port isn't particularly meaningful. But as long as we have it, this probably belongs more in port than in common. Greetings, Andres Freund [1] https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=gharial&dt=2017-10-10%2000%3A31%3A38&stg=config -- Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
Andres Freund <andres@anarazel.de> writes:
> I think the current split between common and port isn't particularly
> meaningful. But as long as we have it, this probably belongs more in
> port than in common.
Well, port is supposed to be for stuff that we expect to find in libc
on common platforms, but is missing some places. strnlen seems to
fall in that category well enough.
regards, tom lane
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
Re: [COMMITTERS] pgsql: Add pg_strnlen() a portable implementationof strlen.
From
Andres Freund
Date:
On 2017-10-09 23:33:36 -0400, Andrew Dunstan wrote: > > > On 10/09/2017 07:15 PM, Andres Freund wrote: > > Hi Andrew, > > > > On 2017-10-09 22:22:04 +0000, Andres Freund wrote: > >> Add pg_strnlen() a portable implementation of strlen. > >> > >> As the OS version is likely going to be more optimized, fall back to > >> it if available, as detected by configure. > > I'm a bit confused, frogmouth > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=frogmouth&dt=2017-10-09%2022%3A30%3A41 > > shows that it compiled the new code, but the configure output doesn't > > show it ran through the new configure test. Additionally, without the > > the config define, this should result in the replacement being > > used. Which doesn't seem to be the case either. > > > > Kinda sounds like this used some halfway outdated build or such? > > > > > > frogmouth is using some code not yet released that makes the config > cache persistent. I just identified and fixed a stupid bug in the code > that obsoletes the cache, and I have removed frogmouth's cache file and > set it running again, so we'll see if that fixes things. As far as I can tell it's still somehow using a configure from before the last commits: https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=frogmouth&dt=2017-10-10%2018%3A35%3A06&stg=configure Note that it's not running the new test. (there's definitely fixes to be made to where strnlen's replacement is located, but regardless, this needs to be fixed too) - Andres -- Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
Andres Freund <andres@anarazel.de> writes:
> (there's definitely fixes to be made to where strnlen's replacement is
> located, but regardless, this needs to be fixed too)
Given that strnlen is standardized by POSIX, and has been for nigh a
decade, I think it'd be all right for us to treat it as a straight
port replacement function, a la strlcpy() for instance. That is,
forget the pg_ prefix and just create a strnlen() function if the
platform has not got it.
regards, tom lane
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
Re: [COMMITTERS] pgsql: Add pg_strnlen() a portable implementationof strlen.
From
Andres Freund
Date:
On 2017-10-10 16:37:04 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > (there's definitely fixes to be made to where strnlen's replacement is > > located, but regardless, this needs to be fixed too) > > Given that strnlen is standardized by POSIX, and has been for nigh a > decade, I think it'd be all right for us to treat it as a straight > port replacement function, a la strlcpy() for instance. That is, > forget the pg_ prefix and just create a strnlen() function if the > platform has not got it. Yea, I'm ok with that. But can't do that before the configure issue on these boxes is fixed, otherwise we'll likely get symbol conflicts... Greetings, Andres Freund -- Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
Andres Freund <andres@anarazel.de> writes:
> As far as I can tell it's still somehow using a configure from before
> the last commits:
No, it's pilot error. The AC_CHECK_FUNCS call you added strnlen to
is only executed if
AS_IF([test "$enable_thread_safety" = yes -a "$PORTNAME" != "win32"],
A better place would be configure.in:1403, probably.
regards, tom lane
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
Re: [COMMITTERS] pgsql: Add pg_strnlen() a portable implementationof strlen.
From
Andres Freund
Date:
On 2017-10-10 16:51:39 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > As far as I can tell it's still somehow using a configure from before > > the last commits: > > No, it's pilot error. The AC_CHECK_FUNCS call you added strnlen to > is only executed if > AS_IF([test "$enable_thread_safety" = yes -a "$PORTNAME" != "win32"], Dear god, I'm daft. Dear god, m4 is a horrible lanuagage. Thanks. -- Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
Re: [COMMITTERS] pgsql: Add pg_strnlen() a portable implementation ofstrlen.
From
Andrew Dunstan
Date:
On 10/10/2017 03:48 PM, Andres Freund wrote: > On 2017-10-09 23:33:36 -0400, Andrew Dunstan wrote: >> >> On 10/09/2017 07:15 PM, Andres Freund wrote: >>> Hi Andrew, >>> >>> On 2017-10-09 22:22:04 +0000, Andres Freund wrote: >>>> Add pg_strnlen() a portable implementation of strlen. >>>> >>>> As the OS version is likely going to be more optimized, fall back to >>>> it if available, as detected by configure. >>> I'm a bit confused, frogmouth >>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=frogmouth&dt=2017-10-09%2022%3A30%3A41 >>> shows that it compiled the new code, but the configure output doesn't >>> show it ran through the new configure test. Additionally, without the >>> the config define, this should result in the replacement being >>> used. Which doesn't seem to be the case either. >>> >>> Kinda sounds like this used some halfway outdated build or such? >>> >> >> >> frogmouth is using some code not yet released that makes the config >> cache persistent. I just identified and fixed a stupid bug in the code >> that obsoletes the cache, and I have removed frogmouth's cache file and >> set it running again, so we'll see if that fixes things. > As far as I can tell it's still somehow using a configure from before > the last commits: > https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=frogmouth&dt=2017-10-10%2018%3A35%3A06&stg=configure > > Note that it's not running the new test. > > (there's definitely fixes to be made to where strnlen's replacement is > located, but regardless, this needs to be fixed too) > This test is governed by the test at line 946 of configure.in. You need to move it somewhere else. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
Re: [COMMITTERS] pgsql: Add pg_strnlen() a portable implementationof strlen.
From
Andres Freund
Date:
On 2017-10-10 13:53:15 -0700, Andres Freund wrote: > On 2017-10-10 16:51:39 -0400, Tom Lane wrote: > > Andres Freund <andres@anarazel.de> writes: > > > As far as I can tell it's still somehow using a configure from before > > > the last commits: > > > > No, it's pilot error. The AC_CHECK_FUNCS call you added strnlen to > > is only executed if > > AS_IF([test "$enable_thread_safety" = yes -a "$PORTNAME" != "win32"], > > Dear god, I'm daft. Dear god, m4 is a horrible lanuagage. Here's a fix. Not quite sure whether we really need the HAVE_DECL_STRNLEN test, added it for symmetry. Afaict windows "always" had strnlen, so no need to meddle with the windows build. Will eat lunch and push, even if there's some futher adjustments, I'd like to get Andrew's animals green again. Greetings, Andres Freund -- Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
Attachment
Re: [COMMITTERS] pgsql: Add pg_strnlen() a portable implementationof strlen.
From
Andres Freund
Date:
On 2017-10-10 17:34:17 -0400, Andrew Dunstan wrote: > This test is governed by the test at line 946 of configure.in. You need > to move it somewhere else. Yea, sorry for the noise :(. Greetings, Andres Freund -- Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
Andres Freund <andres@anarazel.de> writes:
> Here's a fix. Not quite sure whether we really need the
> HAVE_DECL_STRNLEN test, added it for symmetry.
LGTM. I think the DECL test is a good idea; the system definition
might be a macro or otherwise weird, in which case we'd cause problems
if we write an extern definition anyway.
regards, tom lane
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
Re: [COMMITTERS] pgsql: Add pg_strnlen() a portable implementationof strlen.
From
Andres Freund
Date:
On 2017-10-10 18:10:15 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > Here's a fix. Not quite sure whether we really need the > > HAVE_DECL_STRNLEN test, added it for symmetry. > > LGTM. Thanks for checking. > I think the DECL test is a good idea; the system definition > might be a macro or otherwise weird, in which case we'd cause problems > if we write an extern definition anyway. I was thinking about protecting it with HAVE_STRNLEN rather than not protecting it at all. But this works, so whatever ;) Greetings, Andres Freund -- Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers