Обсуждение: 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@paquier.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@gmx.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@sss.pgh.pa.us> wrote: > Peter Eisentraut <peter_e@gmx.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@sss.pgh.pa.us> wrote: >> Peter Eisentraut <peter_e@gmx.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.eisentraut@2ndquadrant.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.eisentraut@2ndquadrant.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@alvh.no-ip.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@alvh.no-ip.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@alvh.no-ip.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@paquier.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@alvh.no-ip.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@sss.pgh.pa.us> wrote:
Alvaro Herrera <alvherre@alvh.no-ip.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