Thread: Proposal: More portable way to support 64bit platforms
Proposal: More portable way to support 64bit platforms
Short description:
Current PostgreSQL implementation has some portability issues to
support 64bit platforms: pointer calculations using long is not
portable, for example on Windows x64 platform. We propose to use
intptr_t instead of long, which appears in in C99.
Details: intptr_t is defined in <stdint.h>. configure script already
has HAVE_STDINT_H but never uses it. This needs to be enabled.
Please note that Windows/VC++ defines intptr_t in <crtdefs.h>.
Included is a conceptual patch to use intptr_t. Comments are welcome.
Some notes for the patches:
access/common/heaptuple.c:
Casting using (long) is removed. It is no more necessary if we
introduce intptr_t.
include/c.h:
Many Alignment macros which use "long" are rewritten to use intrptr_t.
The patches is against PostgreSQL 8.4beta2. Regression test
passed. Windows x64 is ok even with shared_buffers = 3000MB.
Tested platforms are as follows:
Windows Server 2008 SP1 x64 + Visual Studio 2005
RHEL 4 x86_64 + gcc 3.4.6
FreeBSD 7.1 i386 + gcc 4.2.1
TODO:
Some problems may occur on older platforms, which do not have
stdint.h. In this case we need to add something like below to
include/port/*.h.
/* LP64, IPL64, ILP32, LP32 */
typedef long intptr_t;
typedef unsigned long uintptr_t;
/* LLP64 */
typedef long long intptr_t;
typedef unsigned long long uintptr_t;
Thanks,
--
Tsutomu Yamada // tsutomu@sraoss.co.jp
SRA OSS, Inc. Japan
diff -cwbr postgresql-8.4beta2-orig/src/backend/access/common/heaptuple.c
postgresql-8.4beta2-winx64/src/backend/access/common/heaptuple.c
*** postgresql-8.4beta2-orig/src/backend/access/common/heaptuple.c 2009-03-30 13:08:43.000000000 +0900
--- postgresql-8.4beta2-winx64/src/backend/access/common/heaptuple.c 2009-06-19 16:26:01.000000000 +0900
***************
*** 192,198 ****
if (att[i]->attbyval)
{
/* pass-by-value */
! data = (char *) att_align_nominal((long) data, att[i]->attalign);
store_att_byval(data, values[i], att[i]->attlen);
data_length = att[i]->attlen;
}
--- 192,198 ----
if (att[i]->attbyval)
{
/* pass-by-value */
! data = (char *) att_align_nominal(data, att[i]->attalign);
store_att_byval(data, values[i], att[i]->attlen);
data_length = att[i]->attlen;
}
***************
*** 226,232 ****
else
{
/* full 4-byte header varlena */
! data = (char *) att_align_nominal((long) data,
att[i]->attalign);
data_length = VARSIZE(val);
memcpy(data, val, data_length);
--- 226,232 ----
else
{
/* full 4-byte header varlena */
! data = (char *) att_align_nominal(data,
att[i]->attalign);
data_length = VARSIZE(val);
memcpy(data, val, data_length);
***************
*** 243,249 ****
else
{
/* fixed-length pass-by-reference */
! data = (char *) att_align_nominal((long) data, att[i]->attalign);
Assert(att[i]->attlen > 0);
data_length = att[i]->attlen;
memcpy(data, DatumGetPointer(values[i]), data_length);
--- 243,249 ----
else
{
/* fixed-length pass-by-reference */
! data = (char *) att_align_nominal(data, att[i]->attalign);
Assert(att[i]->attlen > 0);
data_length = att[i]->attlen;
memcpy(data, DatumGetPointer(values[i]), data_length);
diff -cwbr postgresql-8.4beta2-orig/src/backend/access/hash/hashfunc.c
postgresql-8.4beta2-winx64/src/backend/access/hash/hashfunc.c
*** postgresql-8.4beta2-orig/src/backend/access/hash/hashfunc.c 2009-02-10 06:18:28.000000000 +0900
--- postgresql-8.4beta2-winx64/src/backend/access/hash/hashfunc.c 2009-06-18 22:37:46.000000000 +0900
***************
*** 319,325 ****
a = b = c = 0x9e3779b9 + len + 3923095;
/* If the source pointer is word-aligned, we use word-wide fetches */
! if (((long) k & UINT32_ALIGN_MASK) == 0)
{
/* Code path for aligned source data */
register const uint32 *ka = (const uint32 *) k;
--- 319,325 ----
a = b = c = 0x9e3779b9 + len + 3923095;
/* If the source pointer is word-aligned, we use word-wide fetches */
! if (((intptr_t) k & UINT32_ALIGN_MASK) == 0)
{
/* Code path for aligned source data */
register const uint32 *ka = (const uint32 *) k;
diff -cwbr postgresql-8.4beta2-orig/src/include/access/tupmacs.h
postgresql-8.4beta2-winx64/src/include/access/tupmacs.h
*** postgresql-8.4beta2-orig/src/include/access/tupmacs.h 2009-01-02 02:23:56.000000000 +0900
--- postgresql-8.4beta2-winx64/src/include/access/tupmacs.h 2009-06-19 16:20:02.000000000 +0900
***************
*** 142,148 ****
#define att_align_nominal(cur_offset, attalign) \
( \
((attalign) == 'i') ? INTALIGN(cur_offset) : \
! (((attalign) == 'c') ? (long) (cur_offset) : \
(((attalign) == 'd') ? DOUBLEALIGN(cur_offset) : \
( \
AssertMacro((attalign) == 's'), \
--- 142,148 ----
#define att_align_nominal(cur_offset, attalign) \
( \
((attalign) == 'i') ? INTALIGN(cur_offset) : \
! (((attalign) == 'c') ? (intptr_t) (cur_offset) : \
(((attalign) == 'd') ? DOUBLEALIGN(cur_offset) : \
( \
AssertMacro((attalign) == 's'), \
diff -cwbr postgresql-8.4beta2-orig/src/include/c.h postgresql-8.4beta2-winx64/src/include/c.h
*** postgresql-8.4beta2-orig/src/include/c.h 2009-03-27 07:26:07.000000000 +0900
--- postgresql-8.4beta2-winx64/src/include/c.h 2009-06-19 15:43:35.000000000 +0900
***************
*** 82,87 ****
--- 82,91 ----
#include <SupportDefs.h>
#endif
+ #ifdef HAVE_STDINT_H
+ #include <stdint.h>
+ #endif
+
#if defined(WIN32) || defined(__CYGWIN__)
/* We have to redefine some system functions after they are included above. */
#include "pg_config_os.h"
***************
*** 492,498 ****
* True iff pointer is properly aligned to point to the given type.
*/
#define PointerIsAligned(pointer, type) \
! (((long)(pointer) % (sizeof (type))) == 0)
#define OidIsValid(objectId) ((bool) ((objectId) != InvalidOid))
--- 496,502 ----
* True iff pointer is properly aligned to point to the given type.
*/
#define PointerIsAligned(pointer, type) \
! (((intptr_t)(pointer) % (sizeof (type))) == 0)
#define OidIsValid(objectId) ((bool) ((objectId) != InvalidOid))
***************
*** 538,544 ****
*/
#define TYPEALIGN(ALIGNVAL,LEN) \
! (((long) (LEN) + ((ALIGNVAL) - 1)) & ~((long) ((ALIGNVAL) - 1)))
#define SHORTALIGN(LEN) TYPEALIGN(ALIGNOF_SHORT, (LEN))
#define INTALIGN(LEN) TYPEALIGN(ALIGNOF_INT, (LEN))
--- 542,548 ----
*/
#define TYPEALIGN(ALIGNVAL,LEN) \
! (((intptr_t) (LEN) + ((ALIGNVAL) - 1)) & ~((intptr_t) ((ALIGNVAL) - 1)))
#define SHORTALIGN(LEN) TYPEALIGN(ALIGNOF_SHORT, (LEN))
#define INTALIGN(LEN) TYPEALIGN(ALIGNOF_INT, (LEN))
***************
*** 549,555 ****
#define BUFFERALIGN(LEN) TYPEALIGN(ALIGNOF_BUFFER, (LEN))
#define TYPEALIGN_DOWN(ALIGNVAL,LEN) \
! (((long) (LEN)) & ~((long) ((ALIGNVAL) - 1)))
#define SHORTALIGN_DOWN(LEN) TYPEALIGN_DOWN(ALIGNOF_SHORT, (LEN))
#define INTALIGN_DOWN(LEN) TYPEALIGN_DOWN(ALIGNOF_INT, (LEN))
--- 553,559 ----
#define BUFFERALIGN(LEN) TYPEALIGN(ALIGNOF_BUFFER, (LEN))
#define TYPEALIGN_DOWN(ALIGNVAL,LEN) \
! (((intptr_t) (LEN)) & ~((intptr_t) ((ALIGNVAL) - 1)))
#define SHORTALIGN_DOWN(LEN) TYPEALIGN_DOWN(ALIGNOF_SHORT, (LEN))
#define INTALIGN_DOWN(LEN) TYPEALIGN_DOWN(ALIGNOF_INT, (LEN))
***************
*** 630,636 ****
int _val = (val); \
Size _len = (len); \
\
! if ((((long) _vstart) & LONG_ALIGN_MASK) == 0 && \
(_len & LONG_ALIGN_MASK) == 0 && \
_val == 0 && \
_len <= MEMSET_LOOP_LIMIT && \
--- 634,640 ----
int _val = (val); \
Size _len = (len); \
\
! if ((((intptr_t) _vstart) & LONG_ALIGN_MASK) == 0 && \
(_len & LONG_ALIGN_MASK) == 0 && \
_val == 0 && \
_len <= MEMSET_LOOP_LIMIT && \
diff -cwbr postgresql-8.4beta2-orig/src/include/pg_config.h.win32
postgresql-8.4beta2-winx64/src/include/pg_config.h.win32
*** postgresql-8.4beta2-orig/src/include/pg_config.h.win32 2009-05-15 11:18:27.000000000 +0900
--- postgresql-8.4beta2-winx64/src/include/pg_config.h.win32 2009-06-18 19:38:25.000000000 +0900
***************
*** 350,356 ****
/* #undef HAVE_SRANDOM */
/* Define to 1 if you have the <stdint.h> header file. */
! #define HAVE_STDINT_H 1
/* Define to 1 if you have the <stdlib.h> header file. */
#define HAVE_STDLIB_H 1
--- 350,356 ----
/* #undef HAVE_SRANDOM */
/* Define to 1 if you have the <stdint.h> header file. */
! /* #undef HAVE_STDINT_H */
/* Define to 1 if you have the <stdlib.h> header file. */
#define HAVE_STDLIB_H 1
diff -cwbr postgresql-8.4beta2-orig/src/interfaces/ecpg/ecpglib/data.c
postgresql-8.4beta2-winx64/src/interfaces/ecpg/ecpglib/data.c
*** postgresql-8.4beta2-orig/src/interfaces/ecpg/ecpglib/data.c 2009-01-15 20:52:55.000000000 +0900
--- postgresql-8.4beta2-winx64/src/interfaces/ecpg/ecpglib/data.c 2009-06-18 19:29:20.000000000 +0900
***************
*** 153,163 ****
if (pval)
{
if (varcharsize == 0 || varcharsize * offset >= size)
! memcpy((char *) ((long) var + offset * act_tuple),
pval, size);
else
{
! memcpy((char *) ((long) var + offset * act_tuple),
pval, varcharsize * offset);
if (varcharsize * offset < size)
--- 153,163 ----
if (pval)
{
if (varcharsize == 0 || varcharsize * offset >= size)
! memcpy((char *) ((intptr_t) var + offset * act_tuple),
pval, size);
else
{
! memcpy((char *) ((intptr_t) var + offset * act_tuple),
pval, varcharsize * offset);
if (varcharsize * offset < size)
***************
*** 392,401 ****
if (pval)
{
if (varcharsize == 0 || varcharsize > size)
! strncpy((char *) ((long) var + offset * act_tuple), pval, size + 1);
else
{
! strncpy((char *) ((long) var + offset * act_tuple), pval, varcharsize);
if (varcharsize < size)
{
--- 392,401 ----
if (pval)
{
if (varcharsize == 0 || varcharsize > size)
! strncpy((char *) ((intptr_t) var + offset * act_tuple), pval, size + 1);
else
{
! strncpy((char *) ((intptr_t) var + offset * act_tuple), pval, varcharsize);
if (varcharsize < size)
{
***************
*** 434,440 ****
if (pval)
{
struct ECPGgeneric_varchar *variable =
! (struct ECPGgeneric_varchar *) ((long) var + offset * act_tuple);
variable->len = size;
if (varcharsize == 0)
--- 434,440 ----
if (pval)
{
struct ECPGgeneric_varchar *variable =
! (struct ECPGgeneric_varchar *) ((intptr_t) var + offset * act_tuple);
variable->len = size;
if (varcharsize == 0)
diff -cwbr postgresql-8.4beta2-orig/src/pl/plperl/plperl.c postgresql-8.4beta2-winx64/src/pl/plperl/plperl.c
*** postgresql-8.4beta2-orig/src/pl/plperl/plperl.c 2009-02-20 19:39:19.000000000 +0900
--- postgresql-8.4beta2-winx64/src/pl/plperl/plperl.c 2009-06-18 19:25:06.000000000 +0900
***************
*** 2192,2198 ****
************************************************************/
qdesc = (plperl_query_desc *) malloc(sizeof(plperl_query_desc));
MemSet(qdesc, 0, sizeof(plperl_query_desc));
! snprintf(qdesc->qname, sizeof(qdesc->qname), "%lx", (long) qdesc);
qdesc->nargs = argc;
qdesc->argtypes = (Oid *) malloc(argc * sizeof(Oid));
qdesc->arginfuncs = (FmgrInfo *) malloc(argc * sizeof(FmgrInfo));
--- 2192,2198 ----
************************************************************/
qdesc = (plperl_query_desc *) malloc(sizeof(plperl_query_desc));
MemSet(qdesc, 0, sizeof(plperl_query_desc));
! snprintf(qdesc->qname, sizeof(qdesc->qname), "%p", qdesc);
qdesc->nargs = argc;
qdesc->argtypes = (Oid *) malloc(argc * sizeof(Oid));
qdesc->arginfuncs = (FmgrInfo *) malloc(argc * sizeof(FmgrInfo));
diff -cwbr postgresql-8.4beta2-orig/src/port/open.c postgresql-8.4beta2-winx64/src/port/open.c
*** postgresql-8.4beta2-orig/src/port/open.c 2009-01-02 02:24:04.000000000 +0900
--- postgresql-8.4beta2-winx64/src/port/open.c 2009-06-18 19:22:08.000000000 +0900
***************
*** 123,129 ****
}
/* _open_osfhandle will, on error, set errno accordingly */
! if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0)
CloseHandle(h); /* will not affect errno */
else if (fileFlags & (O_TEXT | O_BINARY) &&
_setmode(fd, fileFlags & (O_TEXT | O_BINARY)) < 0)
--- 123,129 ----
}
/* _open_osfhandle will, on error, set errno accordingly */
! if ((fd = _open_osfhandle((intptr_t) h, fileFlags & O_APPEND)) < 0)
CloseHandle(h); /* will not affect errno */
else if (fileFlags & (O_TEXT | O_BINARY) &&
_setmode(fd, fileFlags & (O_TEXT | O_BINARY)) < 0)
On Friday 26 June 2009 12:07:24 Tsutomu Yamada wrote: > Proposal: More portable way to support 64bit platforms > > Short description: > > Current PostgreSQL implementation has some portability issues to > support 64bit platforms: pointer calculations using long is not > portable, for example on Windows x64 platform. We propose to use > intptr_t instead of long, which appears in in C99. This makes sense. You can also review the archives for previous iterations of this discussion (search for "intptr_t"). You might want to add your patch to the next commit fest.
Peter Eisentraut <peter_e@gmx.net> wrote:> On Friday 26 June 2009 12:07:24 Tsutomu Yamada wrote:> > Proposal: More portableway to support 64bit platforms> >> > Short description:> >> > Current PostgreSQL implementation has some portabilityissues to> > support 64bit platforms: pointer calculations using long is not> > portable, for example on Windowsx64 platform. We propose to use> > intptr_t instead of long, which appears in in C99.> > This makes sense. You canalso review the archives for previous iterations of > this discussion (search for "intptr_t"). Yes, I have read through the discusion but it seems somewhat faded out. This is because no platform other than Windows has 64bit pointer issues IMO. I think using intptr_t is cleaner and will bring more portability. Moreover it will solve Windows 64bit pointer issues, I believe. > You might want to add your patch to the next commit fest. Yes, I would like to submit patches for the next commit fest. -- Tsutomu Yamada SRA OSS, Inc. Japan
Tsutomu Yamada <tsutomu@sraoss.co.jp> writes:
> Yes, I have read through the discusion but it seems somewhat faded
> out. This is because no platform other than Windows has 64bit
> pointer issues IMO. I think using intptr_t is cleaner and will bring
> more portability. Moreover it will solve Windows 64bit pointer issues,
> I believe.
The problem with this is that it's barely the tip of the iceberg.
One point I recall is that there are lots of places where "%lu" is
assumed to be the correct format to print Datums with. If it were
actually possible to support Win64 with only a couple of dozen lines
of changes, we would have done it long since.
regards, tom lane
On Monday 29 June 2009 17:20:09 Tom Lane wrote: > The problem with this is that it's barely the tip of the iceberg. > One point I recall is that there are lots of places where "%lu" is > assumed to be the correct format to print Datums with. Hmm. I tried this out. typedef Datum to be long long int on a 32-bit platform and compile. You get lots of warnings, but none about a format problem. But if you explicitly insert a call like elog(INFO "datum is %lu", somedatum), then you see a warning. So this problem might not be very widespread. > If it were > actually possible to support Win64 with only a couple of dozen lines > of changes, we would have done it long since. Possibly, or everyone was too confused and didn't know where to start. I think this proposed change is a step in the right direction, and it doesn't make things worse for anyone else.
Peter Eisentraut <peter_e@gmx.net> writes:
> On Monday 29 June 2009 17:20:09 Tom Lane wrote:
>> If it were
>> actually possible to support Win64 with only a couple of dozen lines
>> of changes, we would have done it long since.
> Possibly, or everyone was too confused and didn't know where to start.
Well, the previous proposal involved a massively invasive patch IIRC,
so I'm suspicious of this one being so short...
regards, tom lane
On Friday 26 June 2009 12:07:24 Tsutomu Yamada wrote: > Included is a conceptual patch to use intptr_t. Comments are welcome. After closer inspection, not having a win64 box available, I have my doubts whether this patch actually does anything. Foremost, it doesn't touch the definition of the Datum type, which ought to be at the core of a change like this. Now I see that you call this a "conceptual patch". Perhaps we should wait until you have developed it into a complete patch?
Peter,
* Peter Eisentraut (peter_e@gmx.net) wrote:
> After closer inspection, not having a win64 box available, I have my doubts
> whether this patch actually does anything. Foremost, it doesn't touch the
> definition of the Datum type, which ought to be at the core of a change like
> this.
Do you need access to a Win64 box? I can provide you access to a
Win64 system, which Dave Page and Magnus already have access to, if it
would be useful..
Thanks,
Stephen
On Fri, Jul 24, 2009 at 10:35 PM, Stephen Frost<sfrost@snowman.net> wrote: > Peter, > > * Peter Eisentraut (peter_e@gmx.net) wrote: >> After closer inspection, not having a win64 box available, I have my doubts >> whether this patch actually does anything. Foremost, it doesn't touch the >> definition of the Datum type, which ought to be at the core of a change like >> this. > > Do you need access to a Win64 box? I can provide you access to a > Win64 system, which Dave Page and Magnus already have access to, if it > would be useful.. I haven't got round to installing a build env on there yet btw. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave,
* Dave Page (dpage@pgadmin.org) wrote:
> On Fri, Jul 24, 2009 at 10:35 PM, Stephen Frost<sfrost@snowman.net> wrote:
> > Do you need access to a Win64 box? I can provide you access to a
> > Win64 system, which Dave Page and Magnus already have access to, if it
> > would be useful..
>
> I haven't got round to installing a build env on there yet btw.
Anything we can do to help..? If you can tell us what you'd like
installed, I can probably have someone install it, provided it's not
horribly complicated. :)
Thanks,
Stephen
On Fri, Jul 24, 2009 at 10:53 PM, Stephen Frost<sfrost@snowman.net> wrote: > Dave, > > * Dave Page (dpage@pgadmin.org) wrote: >> On Fri, Jul 24, 2009 at 10:35 PM, Stephen Frost<sfrost@snowman.net> wrote: >> > Do you need access to a Win64 box? I can provide you access to a >> > Win64 system, which Dave Page and Magnus already have access to, if it >> > would be useful.. >> >> I haven't got round to installing a build env on there yet btw. > > Anything we can do to help..? If you can tell us what you'd like > installed, I can probably have someone install it, provided it's not > horribly complicated. :) Well, if you have a spare few minutes, VC++ 2005 Express, and the platform SDK would be useful. Thanks. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
On Sat, Jul 25, 2009 at 02:24, Dave Page<dpage@pgadmin.org> wrote: > On Fri, Jul 24, 2009 at 10:53 PM, Stephen Frost<sfrost@snowman.net> wrote: >> Dave, >> >> * Dave Page (dpage@pgadmin.org) wrote: >>> On Fri, Jul 24, 2009 at 10:35 PM, Stephen Frost<sfrost@snowman.net> wrote: >>> > Do you need access to a Win64 box? I can provide you access to a >>> > Win64 system, which Dave Page and Magnus already have access to, if it >>> > would be useful.. >>> >>> I haven't got round to installing a build env on there yet btw. >> >> Anything we can do to help..? If you can tell us what you'd like >> installed, I can probably have someone install it, provided it's not >> horribly complicated. :) > > Well, if you have a spare few minutes, VC++ 2005 Express, and the > platform SDK would be useful. IIRC, there is no 64-bit support in VC++2005 Express. There is a 64-bit compiler in the SDK though, that can probably be made to work with it. I think the official support for this (SDK compiler integrated with VC++ Express) only arrived in 2008. I don't know how much work it would be though, so it would seem it's worth a try :-) But the integration of 64-bit is one of the reasons they claim for buying the full version. -- Magnus HaganderSelf: http://www.hagander.net/Work: http://www.redpill-linpro.com/
On Sat, Jul 25, 2009 at 9:18 AM, Magnus Hagander<magnus@hagander.net> wrote: > IIRC, there is no 64-bit support in VC++2005 Express. There is a > 64-bit compiler in the SDK though, that can probably be made to work > with it. I think the official support for this (SDK compiler > integrated with VC++ Express) only arrived in 2008. I don't know how > much work it would be though, so it would seem it's worth a try :-) > But the integration of 64-bit is one of the reasons they claim for > buying the full version. That rings a bell actually. No problem - we can just compile on the command line, -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
On Sat, Jul 25, 2009 at 10:35, Dave Page<dpage@pgadmin.org> wrote: > On Sat, Jul 25, 2009 at 9:18 AM, Magnus Hagander<magnus@hagander.net> wrote: > >> IIRC, there is no 64-bit support in VC++2005 Express. There is a >> 64-bit compiler in the SDK though, that can probably be made to work >> with it. I think the official support for this (SDK compiler >> integrated with VC++ Express) only arrived in 2008. I don't know how >> much work it would be though, so it would seem it's worth a try :-) >> But the integration of 64-bit is one of the reasons they claim for >> buying the full version. > > That rings a bell actually. No problem - we can just compile on the > command line, You still need vcbuild, which I don't believe ships with the platform sdk. -- Magnus HaganderSelf: http://www.hagander.net/Work: http://www.redpill-linpro.com/
On Fri, Jul 24, 2009 at 4:24 PM, Peter Eisentraut<peter_e@gmx.net> wrote: > On Friday 26 June 2009 12:07:24 Tsutomu Yamada wrote: >> Included is a conceptual patch to use intptr_t. Comments are welcome. > > After closer inspection, not having a win64 box available, I have my doubts > whether this patch actually does anything. Foremost, it doesn't touch the > definition of the Datum type, which ought to be at the core of a change like > this. > > Now I see that you call this a "conceptual patch". Perhaps we should wait > until you have developed it into a complete patch? Is there any reason to consider this patch any further during this CommitFest? It seems that this is a long way from being ready to go. ...Robert
Robert Haas <robertmhaas@gmail.com> wrote:> On Fri, Jul 24, 2009 at 4:24 PM, Peter Eisentraut<peter_e@gmx.net> wrote:> >On Friday 26 June 2009 12:07:24 Tsutomu Yamada wrote:> >> Included is a conceptual patch to use intptr_t. Comments are welcome.>>> > After closer inspection, not having a win64 box available, I have my doubts> > whether this patch actuallydoes anything. Foremost, it doesn't touch the> > definition of the Datum type, which ought to be at the core of achange like> > this.> >> > Now I see that you call this a "conceptual patch". Perhaps we should wait> > until you have developedit into a complete patch?> > Is there any reason to consider this patch any further during this> CommitFest? Itseems that this is a long way from being ready to go. I'm sorry for delaying response. This patch is needed as a base of the fix for Windows x64 in the future. There are still a lot of corrections necessary for Win x64. (typedef Datum, shared buffer, "%lu" messages, headers, build scripts, ...) We are trying these now, and want to offer the result to the next Commit Fest. Because we are glad if this pointer patch is confirmed at the early stage, we submitted patch to this Commit Fest. Thanks. -- Tsutomu Yamada SRA OSS, Inc. Japan
On Tuesday 04 August 2009 14:03:34 Tsutomu Yamada wrote: > Robert Haas <robertmhaas@gmail.com> wrote: > > On Fri, Jul 24, 2009 at 4:24 PM, Peter Eisentraut<peter_e@gmx.net> wrote: > > > On Friday 26 June 2009 12:07:24 Tsutomu Yamada wrote: > > >> Included is a conceptual patch to use intptr_t. Comments are welcome. > > > > > > After closer inspection, not having a win64 box available, I have my > > > doubts whether this patch actually does anything. Foremost, it doesn't > > > touch the definition of the Datum type, which ought to be at the core > > > of a change like this. > > > > > > Now I see that you call this a "conceptual patch". Perhaps we should > > > wait until you have developed it into a complete patch? > > > > Is there any reason to consider this patch any further during this > > CommitFest? It seems that this is a long way from being ready to go. > > I'm sorry for delaying response. > > This patch is needed as a base of the fix for Windows x64 in the future. > > There are still a lot of corrections necessary for Win x64. > (typedef Datum, shared buffer, "%lu" messages, headers, build scripts, ...) > We are trying these now, and want to offer the result to the next Commit > Fest. > > Because we are glad if this pointer patch is confirmed at the early stage, > we submitted patch to this Commit Fest. Well, there is nothing outright wrong with this patch, but without any measurable effect, it is too early to commit it. At least I would like to see the Datum typedef to be changed to use intptr_t and the fallout from that cleaned up.
On Tue, Aug 4, 2009 at 16:10, Peter Eisentraut<peter_e@gmx.net> wrote: > On Tuesday 04 August 2009 14:03:34 Tsutomu Yamada wrote: >> Robert Haas <robertmhaas@gmail.com> wrote: >> > On Fri, Jul 24, 2009 at 4:24 PM, Peter Eisentraut<peter_e@gmx.net> wrote: >> > > On Friday 26 June 2009 12:07:24 Tsutomu Yamada wrote: >> > >> Included is a conceptual patch to use intptr_t. Comments are welcome. >> > > >> > > After closer inspection, not having a win64 box available, I have my >> > > doubts whether this patch actually does anything. Foremost, it doesn't >> > > touch the definition of the Datum type, which ought to be at the core >> > > of a change like this. >> > > >> > > Now I see that you call this a "conceptual patch". Perhaps we should >> > > wait until you have developed it into a complete patch? >> > >> > Is there any reason to consider this patch any further during this >> > CommitFest? It seems that this is a long way from being ready to go. >> >> I'm sorry for delaying response. >> >> This patch is needed as a base of the fix for Windows x64 in the future. >> >> There are still a lot of corrections necessary for Win x64. >> (typedef Datum, shared buffer, "%lu" messages, headers, build scripts, ...) >> We are trying these now, and want to offer the result to the next Commit >> Fest. >> >> Because we are glad if this pointer patch is confirmed at the early stage, >> we submitted patch to this Commit Fest. > > Well, there is nothing outright wrong with this patch, but without any > measurable effect, it is too early to commit it. At least I would like to see > the Datum typedef to be changed to use intptr_t and the fallout from that > cleaned up. +1. I think it's good that it was posted for a quick review of the general idea, but I agree that it's too early to commit it until we can see some actual benefit. And I expect the Datum changes to be much larger than this, so we can just review/apply them as one when the time comes. -- Magnus HaganderSelf: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
> On Tue, Aug 4, 2009 at 16:10, Peter Eisentraut<peter_e@gmx.net> wrote:
>> Well, there is nothing outright wrong with this patch, but without any
>> measurable effect, it is too early to commit it. �At least I would like to see
>> the Datum typedef to be changed to use intptr_t and the fallout from that
>> cleaned up.
> +1.
> I think it's good that it was posted for a quick review of the general
> idea, but I agree that it's too early to commit it until we can see
> some actual benefit. And I expect the Datum changes to be much larger
> than this, so we can just review/apply them as one when the time
> comes.
The other thing that I would say is a non-negotiable minimum requirement
is that the patch include the necessary configure pushups so it does not
break machines without uintptr_t. I think we could just do a
conditional
typedef unsigned long uintptr_t;
and proceed from there; then machines without the typedef are no worse
off than before.
regards, tom lane
On Tuesday 04 August 2009 17:56:41 Tom Lane wrote: > The other thing that I would say is a non-negotiable minimum requirement > is that the patch include the necessary configure pushups so it does not > break machines without uintptr_t. There is AC_TYPE_UINTPTR_T, so that should be easy.
Peter Eisentraut <peter_e@gmx.net> writes:
> On Tuesday 04 August 2009 17:56:41 Tom Lane wrote:
>> The other thing that I would say is a non-negotiable minimum requirement
>> is that the patch include the necessary configure pushups so it does not
>> break machines without uintptr_t.
> There is AC_TYPE_UINTPTR_T, so that should be easy.
It's certainly do-able, the point is just to do it.
regards, tom lane