On Mon, Oct 7, 2013 at 4:47 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2013-10-07 13:25:17 -0400, Robert Haas wrote:
>> On Fri, Oct 4, 2013 at 8:19 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>> > Could it be that MAXALIGN/TYPEALIGN doesn't really work for values
>> > bigger than 32bit?
>> >
>> > #define MAXALIGN(LEN) TYPEALIGN(MAXIMUM_ALIGNOF, (LEN))
>> > #define TYPEALIGN(ALIGNVAL,LEN) \
>> > (((intptr_t) (LEN) + ((ALIGNVAL) - 1)) & ~((intptr_t) ((ALIGNVAL) - 1)))
>>
>> Isn't the problem, more specifically, that it doesn't work for values
>> larger than an intptr_t?
>
> Well, yes. And intptr_t is 32bit wide on a 32bit platform.
>
>> And does that indicate that intptr_t is the wrong type to be using here?
>
> No, I don't think so. intptr_t is defined to be a integer type to which
> you can cast a pointer, cast it back and still get the old value. On
> 32bit platforms it usually will be 32bit wide.
> All that's fine for the classic usages of TYPEALIGN where it's used on
> pointers or lengths of stuff stored in memory. Those will always fit in
> 32bit on a 32bit platform. But here we're using it on explicit 64bit
> types (XLogRecPtr).
> Now, you could argue that we should make it use 64bit math everywhere -
> but I think that might incur quite the price on some 32bit
> platforms. It's used in the tuple decoding stuff, that's quite the hot
> path in some workloads.
>
> So I guess it's either a separate macro, or we rewrite that piece of
> code to work slightly differently and work directly on the lenght or
> such.
>
> Maybe we should add a StaticAssert ensuring the TYPEALIGN macro only
> gets passed 32bit types?
I think having two macros that behave identically on all platforms
anyone cares about[1] but which can cause difficult-to-find
corner-case-bugs on other platforms is just a recipe for disaster. I
pledge to screw that up at least once.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
[1] And by anyone, I mean me.
