Re: 64-bit pgbench V2

Greg Smith <greg@2ndquadrant.com>

From: Greg Smith <greg@2ndquadrant.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2010-07-12T19:56:51Z
Lists: pgsql-hackers

Attachments

Tom Lane wrote:
> Please choose a way that doesn't introduce new portability assumptions.
> The backend gets along fine without strtoll, and I don't see why pgbench
> should have to require it.
>   

Funny you should mention this...it turns out there is some code already 
there, I just didn't notice it before because it's only the unsigned 
64-bit strtoul used, not the signed one I was looking for, and it's only 
called in one place I didn't previously check.  
src/interfaces/ecpg/ecpglib/data.c does this:

*((unsigned long long int *) (var + offset * act_tuple)) = 
strtoull(pval, &scan_length, 10);

The appropriate autoconf magic was in the code all along for both 
versions, so my bad not noticing it until now.  It even transparently 
remaps the BSD-ism of calling it strtoq.

I suspect that this alone isn't sufficient to make the code I'm trying 
to wedge into pgbench to always work on the platforms I consider must 
haves, because of the weird names like _strtoi64 that Windows uses:  
http://msdn.microsoft.com/en-us/library/h80404d3(v=VS.80).aspx  In fact, 
I wouldn't be surprised to discover the ECPG code above doesn't do the 
right thing if compiled with a 64-bit MSVC version.  Don't expect that's 
a popular combination to explicitly test in a way that hits the code 
path where this line is at.

The untested (I need to setup for building Windows to really confirm 
this works) next patch attempt I've attached does what I think is the 
right general sort of thing here.  It extends the autoconf remapping 
that was already being done to include the second variation on how the 
function needed can be named in a MSVC build.  This might improve the 
ECPG compatibility issue I theorize could be there on that platform.  
Given the autoconf stuff and use of the unsigned version was already a 
dependency, I'd rather improve that code (so it's more obvious when it 
is broken) than do the refactoring work suggested to re-use the server's 
internal 64-bit parsing method instead.  I could split this into two 
patches instead--"add 64-bit strtoull/strtoll support for MSVC" on the 
presumption it's actually broken now (possibly wrong on my part) and 
"make pgbench use 64-bit values"--but it's not so complicated as one.

I expect there is almost zero overlap between "needs pgbench setshell to 
return >32 bit return values" and "not on a platform with a working 
64-bit strtoull variation".  What I did to hedge against that was add a 
little check to pgbench that lets you confirm whether setshell lines are 
limited to 32 bits or not, depending on whether the appropriate function 
was found.  It tries to fall back to the existing strtol in that case, 
and I've put a note when that happens (and matching documentation to 
look for it) into the debug output of the program.

I'll continue with testing work here, but what's attached is now the 
first form I think this could potentially be committed in once it's 
known to be free of obvious bugs (testing at this database scale takes 
forever).  I can revisit not using the library function instead if Tom 
or someone else really opposes this new approach.  Given most of the 
autoconf bits are already there and the limited number of platforms 
where this is a problem, I think there's little gain for doing that work 
though.

Style/functional suggestions appreciated.

-- 
Greg Smith  2ndQuadrant US  Baltimore, MD
PostgreSQL Training, Services and Support
greg@2ndQuadrant.com   www.2ndQuadrant.us