pgsql: Move strtoint() to common

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

pgsql: Move strtoint() to common

Peter Eisentraut-2
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 <[hidden email]>

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(-)

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Move strtoint() to common

Tom Lane-2
Peter Eisentraut <[hidden email]> writes:
> Move strtoint() to common

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

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Move strtoint() to common

David Rowley-3
On 14 March 2018 at 08:10, Tom Lane <[hidden email]> wrote:
> Peter Eisentraut <[hidden email]> 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

remove_restrict_keyword.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Move strtoint() to common

Peter Eisentraut-6
On 3/13/18 21:10, David Rowley wrote:
> On 14 March 2018 at 08:10, Tom Lane <[hidden email]> wrote:
>> Peter Eisentraut <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Move strtoint() to common

Tom Lane-2
Peter Eisentraut <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Move strtoint() to common

Michael Paquier-2
On Wed, Mar 14, 2018 at 11:23:35AM -0400, Tom Lane wrote:

> Peter Eisentraut <[hidden email]> 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

fix-msvc-compile.patch (978 bytes) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Move strtoint() to common

Álvaro Herrera
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

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Move strtoint() to common

Tom Lane-2
Alvaro Herrera <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Move strtoint() to common

Álvaro Herrera
Tom Lane wrote:
> Alvaro Herrera <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Move strtoint() to common

Álvaro Herrera
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

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Move strtoint() to common

Tom Lane-2
In reply to this post by Álvaro Herrera
Alvaro Herrera <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Move strtoint() to common

Tom Lane-2
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

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Move strtoint() to common

Michael Paquier-2
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

ecpg-test-fix.patch (763 bytes) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Move strtoint() to common

Tom Lane-2
Michael Paquier <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Move strtoint() to common

Tom Lane-2
In reply to this post by Álvaro Herrera
Alvaro Herrera <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Move strtoint() to common

Michael Paquier-2
In reply to this post by Tom Lane-2
On Thu, Mar 15, 2018 at 10:37:06PM -0400, Tom Lane wrote:
> Seems sane AFAICT, so pushed.  Thanks!

Thanks, Tom.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Move strtoint() to common

Andrew Dunstan
In reply to this post by Tom Lane-2


On Fri, Mar 16, 2018 at 1:12 PM, Tom Lane <[hidden email]> wrote:
Alvaro Herrera <[hidden email]> 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
 
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Move strtoint() to common

Michael Meskes-3
In reply to this post by Tom Lane-2
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


Previous Thread Next Thread