On 07.10.2013 23:47, Andres Freund wrote:
On 2013-10-07 13:25:17 -0400, Robert Haas wrote:
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).
Yep.
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.
Committed what is pretty much David's original patch.
Maybe we should add a StaticAssert ensuring the TYPEALIGN macro only
gets passed 32bit types?
I tried that, like this:
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -532,7 +532,7 @@ typedef NameData *Name;
*/
#define TYPEALIGN(ALIGNVAL,LEN) \
- (((intptr_t) (LEN) + ((ALIGNVAL) - 1)) & ~((intptr_t) ((ALIGNVAL) - 1)))
+ ( StaticAssertExpr(sizeof(LEN) <= 4, "type is too wide"), ((intptr_t)
(LEN) + ((ALIGNVAL) - 1)) & ~((intptr_t) ((ALIGNVAL) - 1)))
#define SHORTALIGN(LEN) TYPEALIGN(ALIGNOF_SHORT, (LEN))
#define INTALIGN(LEN) TYPEALIGN(ALIGNOF_INT, (LEN))
However, it gave a lot of errors from places where we have something
like "char buf[MaxHeapTuplesPerPage]". MaxHeapTuplesPerPage uses
MAXALIGN, and gcc doesn't like it when StaticAssertExpr is used in such
a context (to determine the size of an array).
I temporarily changed places where that was a problem to use a copy of
TYPEALIGN with no assertion, to verify that there are no more genuine
bugs like this lurking. It was worth it as a one-off check, but I don't
think we want to commit that.
Thanks for the report, and analysis!
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
