Re: 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(at)sraoss(dot)co(dot)jp
SRA OSS, Inc. Japan

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(at)gmx(dot)net> wrote:
> 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").

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(at)sraoss(dot)co(dot)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(at)gmx(dot)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(at)gmx(dot)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(at)snowman(dot)net> wrote:
> Peter,
>
> * Peter Eisentraut (peter_e(at)gmx(dot)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(at)pgadmin(dot)org) wrote:
> On Fri, Jul 24, 2009 at 10:35 PM, Stephen Frost<sfrost(at)snowman(dot)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(at)snowman(dot)net> wrote:
> Dave,
>
> * Dave Page (dpage(at)pgadmin(dot)org) wrote:
>> On Fri, Jul 24, 2009 at 10:35 PM, Stephen Frost<sfrost(at)snowman(dot)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(at)pgadmin(dot)org> wrote:
> On Fri, Jul 24, 2009 at 10:53 PM, Stephen Frost<sfrost(at)snowman(dot)net> wrote:
>> Dave,
>>
>> * Dave Page (dpage(at)pgadmin(dot)org) wrote:
>>> On Fri, Jul 24, 2009 at 10:35 PM, Stephen Frost<sfrost(at)snowman(dot)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 Hagander
Self: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Sat, Jul 25, 2009 at 9:18 AM, Magnus Hagander<magnus(at)hagander(dot)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(at)pgadmin(dot)org> wrote:
> On Sat, Jul 25, 2009 at 9:18 AM, Magnus Hagander<magnus(at)hagander(dot)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 Hagander
Self: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Fri, Jul 24, 2009 at 4:24 PM, Peter Eisentraut<peter_e(at)gmx(dot)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(at)gmail(dot)com> wrote:
> On Fri, Jul 24, 2009 at 4:24 PM, Peter Eisentraut<peter_e(at)gmx(dot)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.

Thanks.

--
Tsutomu Yamada
SRA OSS, Inc. Japan

On Tuesday 04 August 2009 14:03:34 Tsutomu Yamada wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Fri, Jul 24, 2009 at 4:24 PM, Peter Eisentraut<peter_e(at)gmx(dot)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(at)gmx(dot)net> wrote:
> On Tuesday 04 August 2009 14:03:34 Tsutomu Yamada wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>  > On Fri, Jul 24, 2009 at 4:24 PM, Peter Eisentraut<peter_e(at)gmx(dot)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 Hagander
Self: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> On Tue, Aug 4, 2009 at 16:10, Peter Eisentraut<peter_e(at)gmx(dot)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(at)gmx(dot)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