pgsql: Move strtoint() to common

Move strtoint() to common

Several places used similar code to convert a string to an int, so take
the function that we already had and make it globally available.

Reviewed-by: Michael Paquier <michael(at)paquier(dot)xyz>

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/17bb62501787c56e0518e61db13a523d47afd724

Modified Files
--------------
src/backend/nodes/read.c | 12 +++++-------
src/backend/parser/scan.l | 9 ++++-----
src/backend/utils/adt/datetime.c | 18 +-----------------
src/common/string.c | 15 +++++++++++++++
src/include/common/string.h | 1 +
src/interfaces/ecpg/pgtypeslib/.gitignore | 1 +
src/interfaces/ecpg/pgtypeslib/Makefile | 6 +++++-
src/interfaces/ecpg/pgtypeslib/interval.c | 16 ++--------------
src/interfaces/ecpg/preproc/pgc.l | 10 +++++-----
9 files changed, 39 insertions(+), 49 deletions(-)

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Move strtoint() to common

Buildfarm seems to think this isn't quite baked for Windows.

regards, tom lane

On 14 March 2018 at 08:10, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>> Move strtoint() to common
>
> Buildfarm seems to think this isn't quite baked for Windows.

Yeah, "restrict" seems to be C99, and the Microsoft compilers don't
quite know about that yet. The attached compiles fine for me on a
windows machine.

Changing "restrict" to "__restrict" also works, so it might,
longer-term, be worth some configure test and a PG_RESTICT macro so we
can allow this, assuming there are performance gains to be had.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 3/13/18 21:10, David Rowley wrote:
> On 14 March 2018 at 08:10, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>>> Move strtoint() to common
>>
>> Buildfarm seems to think this isn't quite baked for Windows.
>
> Yeah, "restrict" seems to be C99, and the Microsoft compilers don't
> quite know about that yet. The attached compiles fine for me on a
> windows machine.

We have a configure test for it and we already use it elsewhere. Is it
not working?

I think the problem is rather that we somehow need to tell it to link
src/common/string.c into pgtypeslib.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
> I think the problem is rather that we somehow need to tell it to link
> src/common/string.c into pgtypeslib.

Yeah, that's what I supposed.

Looking at pgtypeslib, there's already infrastructure for collecting
stuff from src/port/, and I see you added some for src/common/ in
its Makefile, but evidently not in the MSVC scripts. It looks like
the way the MSVC build works now is dependent on @pgportfiles.

You could invent parallel infrastructure for src/common/, but I wonder
whether the path of least resistance might not be to put strtoint()
into src/port/ instead.

regards, tom lane

On Wed, Mar 14, 2018 at 11:23:35AM -0400, Tom Lane wrote:
> Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
> > I think the problem is rather that we somehow need to tell it to link
> > src/common/string.c into pgtypeslib.
>
> Yeah, that's what I supposed.
>
> Looking at pgtypeslib, there's already infrastructure for collecting
> stuff from src/port/, and I see you added some for src/common/ in
> its Makefile, but evidently not in the MSVC scripts. It looks like
> the way the MSVC build works now is dependent on @pgportfiles.
>
> You could invent parallel infrastructure for src/common/, but I wonder
> whether the path of least resistance might not be to put strtoint()
> into src/port/ instead.

This line from the buildfarm failures is indicating that the handling of
restrict is incorrect:
src/common/string.c(50): error C2146: syntax error : missing ')' before
identifier 'str'
[C:\buildfarm\buildenv\HEAD\pgsql.build\libpgtypes.vcxproj]

So I concur with David that we ought to do something for that. One way
to do things simply is to remove the restrict keyword as suggested
upthread. Another one, which David has not considered, is that there is
a pg_restrict macro defined in pg_config.h. So you could just use that.

Attached is a patch which fixes the compilation failure on Windows for
me. That should put the buildfarm back to green.
--
Michael

Michael Paquier wrote:

> Attached is a patch which fixes the compilation failure on Windows for
> me. That should put the buildfarm back to green.

Pushed, thanks -- let's see how that goes.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> Michael Paquier wrote:
>> Attached is a patch which fixes the compilation failure on Windows for
>> me. That should put the buildfarm back to green.

> Pushed, thanks -- let's see how that goes.

build now works, ecpg tests fail.

regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> > Michael Paquier wrote:
> >> Attached is a patch which fixes the compilation failure on Windows for
> >> me. That should put the buildfarm back to green.
>
> > Pushed, thanks -- let's see how that goes.
>
> build now works, ecpg tests fail.

I stared at the code for a while, didn't notice anything amiss. I'm
mystified. Peter?

I think the guilty bit is the one below, but
1) I don't see how the new code fails to work exactly like the old code
2) I don't understand why it would only fail on Windows.

I thought it may be a port difference in strtol, but I don't see what
it'd be.

Also: it seems strtol per spec returns LONG_MAX/LONG_MIN on
overflow/underflow, and our strtoint doesn't do (an equivalent of) that.
But I don't see how that would affect the failing ecpg test.

diff --git a/src/interfaces/ecpg/preproc/pgc.l b/src/interfaces/ecpg/preproc/pgc.l
index ba1798c77e..405dee73b0 100644
*** a/src/interfaces/ecpg/preproc/pgc.l
--- b/src/interfaces/ecpg/preproc/pgc.l
***************
*** 723,744 **** cppline {space}*#([^i][A-Za-z]*|{if}|{ifdef}|{ifndef}|{import})((\/\*[^*/]*\*+
return Op;
}
<SQL>{param} {
base_yylval.ival = atol(yytext+1);
return PARAM;
}
<C,SQL>{integer} {
! long val;
char* endptr;

errno = 0;
! val = strtol((char *)yytext, &endptr,10);
! if (*endptr != '\0' || errno == ERANGE ||
! /* check for overflow of int */
! val != (int) val)
{
errno = 0;
base_yylval.str = mm_strdup(yytext);
return FCONST;
}
base_yylval.ival = val;
return ICONST;
--- 725,744 ----
return Op;
}
<SQL>{param} {
base_yylval.ival = atol(yytext+1);
return PARAM;
}
<C,SQL>{integer} {
! int val;
char* endptr;

errno = 0;
! val = strtoint(yytext, &endptr, 10);
! if (*endptr != '\0' || errno == ERANGE)
{
errno = 0;
base_yylval.str = mm_strdup(yytext);
return FCONST;
}
base_yylval.ival = val;
return ICONST;

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

ah, but bowerbird is OK on ecpg, this is only failing on thrips, whelk,
woodlouse. It sounds related to 32 vs. 64 bits ...

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> Tom Lane wrote:
>> build now works, ecpg tests fail.

> I stared at the code for a while, didn't notice anything amiss. I'm
> mystified. Peter?

I'm wondering if the Windows environment is somehow supplying a
"strtoint" that isn't actually ABI-compatible with ours.

A different theory is that the pg_restrict markers are bollixing things.
No idea how, but they don't look like they're really doing anything for us
optimization-wise, so it doesn't seem unreasonable to remove them and see
if that makes a difference.

regards, tom lane

Oh ... duh. We've been assuming that the strtoint change broke it,
but that's wrong. The test case that is failing is new as of yesterday,
and the correct answer is that it's never worked on Windows. See

https://git.postgresql.org/gitweb/?p=postgresql.git&a=commitdiff&h=3b7ab4380440d7b14ee390fabf39f6d87d7491e2

I think what's wrong is that src/tools/msvc/ecpg_regression.proj
needs to be taught that tests under ecpg/test/compat-oracle need
to be run with "-C ORACLE". Neither that directory nor that
switch existed before yesterday. There's already stuff in there
that knows about "-C INFORMIX", but beyond seeing the switch it
looks like line noise to me, so I'm not volunteering to fix it.

regards, tom lane

On Thu, Mar 15, 2018 at 06:57:27PM -0400, Tom Lane wrote:
> I think what's wrong is that src/tools/msvc/ecpg_regression.proj
> needs to be taught that tests under ecpg/test/compat-oracle need
> to be run with "-C ORACLE". Neither that directory nor that
> switch existed before yesterday. There's already stuff in there
> that knows about "-C INFORMIX", but beyond seeing the switch it
> looks like line noise to me, so I'm not volunteering to fix it.

I can confirm that commit 3b7ab43 is at fault, that I can see the
failure, and that the patch attached fixes the failure.
--
Michael

Michael Paquier <michael(at)paquier(dot)xyz> writes:
> On Thu, Mar 15, 2018 at 06:57:27PM -0400, Tom Lane wrote:
>> I think what's wrong is that src/tools/msvc/ecpg_regression.proj
>> needs to be taught that tests under ecpg/test/compat-oracle need
>> to be run with "-C ORACLE". Neither that directory nor that
>> switch existed before yesterday. There's already stuff in there
>> that knows about "-C INFORMIX", but beyond seeing the switch it
>> looks like line noise to me, so I'm not volunteering to fix it.

> I can confirm that commit 3b7ab43 is at fault, that I can see the
> failure, and that the patch attached fixes the failure.

Seems sane AFAICT, so pushed. Thanks!

regards, tom lane

Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> ah, but bowerbird is OK on ecpg, this is only failing on thrips, whelk,
> woodlouse. It sounds related to 32 vs. 64 bits ...

BTW, the reason why bowerbird was green had nothing to do with 32
or 64 bits, but rather that it isn't running the ecpg tests:

'invocation_args' => [
'--skip-steps',
'ecpg-check',

I think Andrew put that in ages ago when we didn't have any MSVC
ecpg test support at all. Might be time to take it out.

regards, tom lane

On Thu, Mar 15, 2018 at 10:37:06PM -0400, Tom Lane wrote:
> Seems sane AFAICT, so pushed. Thanks!

Thanks, Tom.
--
Michael

On Fri, Mar 16, 2018 at 1:12 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> > ah, but bowerbird is OK on ecpg, this is only failing on thrips, whelk,
> > woodlouse. It sounds related to 32 vs. 64 bits ...
>
> BTW, the reason why bowerbird was green had nothing to do with 32
> or 64 bits, but rather that it isn't running the ecpg tests:
>
> 'invocation_args' => [
> '--skip-steps',
> 'ecpg-check',
>
> I think Andrew put that in ages ago when we didn't have any MSVC
> ecpg test support at all. Might be time to take it out.
>
>

I'll try. I put it in ages ago because I was getting random hangs I could
never manage to diagnose with ECPG checks.

cheers

andrew

Am 15.03.2018 um 23:57 schrieb Tom Lane:
> Oh ... duh. We've been assuming that the strtoint change broke it,
> but that's wrong. The test case that is failing is new as of yesterday,
> and the correct answer is that it's never worked on Windows. See
> ...

Correct, thanks for figuring this out Tom. The messages were confusing,
it sure looked like a strtoint problem to me. And admittedly I don't
know much about the Windows build.

Also I was not able to spend enough time looking into this as my flight
home from SCaLE took almost exactly 80 hours, among which 24 were
literally on airplanes and another 40 in St. John's, Canada. Yeah, I
hadn't heard of this place before either.

Michael