64-bit pgbench V2
in [PgSql]
|
From: Tom Lane on 5 Jul 2010 20:17 Greg Smith <greg(a)2ndquadrant.com> writes: > The main tricky part was figuring how to convert the \setshell > implementation. That uses strtol to parse the number that should have > been returned by the shell call. It turns out there are a stack of ways > to do something similar but return 64 bits instead: 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. (BTW, I don't actually believe that the proposed code works at all, since in general strtoll or other variants aren't going to be macros, but plain functions.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Robert Haas on 5 Jul 2010 20:32 On Mon, Jul 5, 2010 at 8:17 PM, Tom Lane <tgl(a)sss.pgh.pa.us> wrote: > Greg Smith <greg(a)2ndquadrant.com> writes: >> The main tricky part was figuring how to convert the \setshell >> implementation. �That uses strtol to parse the number that should have >> been returned by the shell call. �It turns out there are a stack of ways >> to do something similar but return 64 bits instead: > > 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. It doesn't seem very palatable to have multiple handwritten integer parsers floating around the code base either. Maybe we should try to standardize something and ship it in src/port, or somesuch. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Greg Smith on 6 Jul 2010 11:01 Robert Haas wrote: > It doesn't seem very palatable to have multiple handwritten integer > parsers floating around the code base either. Maybe we should try to > standardize something and ship it in src/port, or somesuch I was considering at one point making two trips through strtol, each allowed to gobble 10 characters, then combining the two--just to cut down a little bit on the roll your own parser aspects here. I hadn't really considered how the main server does this job though. If there's something reasonable to expose by refactoring some code that's already there, I could take a stab at that. I'm not exactly sure where the integer parsing code in the server that would be appropriate is to break out is at though. -- Greg Smith 2ndQuadrant US Baltimore, MD PostgreSQL Training, Services and Support greg(a)2ndQuadrant.com www.2ndQuadrant.us -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Robert Haas on 6 Jul 2010 11:03 On Tue, Jul 6, 2010 at 11:01 AM, Greg Smith <greg(a)2ndquadrant.com> wrote: > Robert Haas wrote: >> >> It doesn't seem very palatable to have multiple handwritten integer >> parsers floating around the code base either. �Maybe we should try to >> standardize something and ship it in src/port, or somesuch > > I was considering at one point making two trips through strtol, each allowed > to gobble 10 characters, then combining the two--just to cut down a little > bit on the roll your own parser aspects here. �I hadn't really considered > how the main server does this job though. �If there's something reasonable > to expose by refactoring some code that's already there, I could take a stab > at that. �I'm not exactly sure where the integer parsing code in the server > that would be appropriate is to break out is at though. Take a look at int8in. It's got some backend-specific stuff in it ATM but maybe it would be reasonable to try to fact that out somehow. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Greg Smith on 12 Jul 2010 15:56 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(a)2ndQuadrant.com www.2ndQuadrant.us
|
Pages: 1 Prev: [HACKERS] 64-bit pgbench V2 Next: get_whatever_oid, part 2 |