space reserved for WAL record does not match what was written: panic on windows
In HEAD of 9.4 I'm getting the following:
D:\9.4\bin>postgres.exe -D d:\9.4\data
LOG: database system was shut down at 2013-10-05 00:43:33 NZDT
LOG: database system is ready to accept connections
LOG: autovacuum launcher started
PANIC: space reserved for WAL record does not match what was written:
CurrPos = 18446744071562067968 EndPos = 2147483648
STATEMENT: insert into test (items) select x.x from
generate_series(1,10000000) x(x);
LOG: server process (PID 5476) exited with exit code 3
DETAIL: Failed process was running: insert into test (items) select x.x
from generate_series(1,10000000) x(x);
LOG: terminating any other active server processes
debug_assertions = on
I made a slight change to the line that causes the panic to print out the
values of CurrPos and EndPos, as you can see above.
Only changes made to postgresql.conf are:
checkpoint_segments = 64 # in logfile segments, min 1, 16MB each
checkpoint_timeout = 15min # range 30s-1h
I discovered this was happening just after I bumped the checkpoint segment
up to 64 for bulk loading some test data.
create table test (
id serial not null,
name varchar(64) not null default 'name of something',
price numeric(10,2) not null default 1000.00,
active boolean not null default true,
items int not null default 1,
description text not null default 'description of item',
primary key(id)
);
insert into test (items) select x.x from generate_series(1,10000000) x(x);
I'm running this on windows 7 64bit with postgres compiled as 32bit.
Regards
David
On 2013-10-05 01:05:37 +1300, David Rowley wrote:
In HEAD of 9.4 I'm getting the following:
D:\9.4\bin>postgres.exe -D d:\9.4\data
LOG: database system was shut down at 2013-10-05 00:43:33 NZDT
LOG: database system is ready to accept connections
LOG: autovacuum launcher started
PANIC: space reserved for WAL record does not match what was written:
CurrPos = 18446744071562067968 EndPos = 2147483648
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)))
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Oct 5, 2013 at 1:19 AM, Andres Freund <andres@2ndquadrant.com>wrote:
On 2013-10-05 01:05:37 +1300, David Rowley wrote:
In HEAD of 9.4 I'm getting the following:
D:\9.4\bin>postgres.exe -D d:\9.4\data
LOG: database system was shut down at 2013-10-05 00:43:33 NZDT
LOG: database system is ready to accept connections
LOG: autovacuum launcher started
PANIC: space reserved for WAL record does not match what was written:
CurrPos = 18446744071562067968 EndPos = 2147483648Could 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)))
It looks that way.
As a quick test I put some printf's around where the MAXALIGN is used:
/* Align the end position, so that the next record starts aligned */
printf("CurrPos == %llu (before MAXALIGN)\n", CurrPos);
CurrPos = MAXALIGN(CurrPos);
printf("CurrPos == %llu (after MAXALIGN)\n", CurrPos);
I got the following just before the PANIC.
CurrPos == 2147483711 (before MAXALIGN)
CurrPos == 18446744071562068032 (after MAXALIGN)
Regards
David
On Sat, Oct 5, 2013 at 1:34 AM, David Rowley <dgrowleyml@gmail.com> wrote:
On Sat, Oct 5, 2013 at 1:19 AM, Andres Freund <andres@2ndquadrant.com>wrote:
On 2013-10-05 01:05:37 +1300, David Rowley wrote:
In HEAD of 9.4 I'm getting the following:
D:\9.4\bin>postgres.exe -D d:\9.4\data
LOG: database system was shut down at 2013-10-05 00:43:33 NZDT
LOG: database system is ready to accept connections
LOG: autovacuum launcher started
PANIC: space reserved for WAL record does not match what was written:
CurrPos = 18446744071562067968 EndPos = 2147483648Could 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)))It looks that way.
As a quick test I put some printf's around where the MAXALIGN is used:
/* Align the end position, so that the next record starts aligned */
printf("CurrPos == %llu (before MAXALIGN)\n", CurrPos);
CurrPos = MAXALIGN(CurrPos);
printf("CurrPos == %llu (after MAXALIGN)\n", CurrPos);I got the following just before the PANIC.
CurrPos == 2147483711 (before MAXALIGN)
CurrPos == 18446744071562068032 (after MAXALIGN)
So I guess this would also break the 32bit linux builds too.
I've attached a proposed patch which seems to fix the problem.
Regards
David
Show quoted text
Regards
David
Attachments:
maxalign64.patchapplication/octet-stream; name=maxalign64.patchDownload
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index fc495d6..06f5eb0 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1482,7 +1482,7 @@ CopyXLogRecordToWAL(int write_len, bool isLogSwitch, XLogRecData *rdata,
Assert(written == write_len);
/* Align the end position, so that the next record starts aligned */
- CurrPos = MAXALIGN(CurrPos);
+ CurrPos = MAXALIGN64(CurrPos);
/*
* If this was an xlog-switch, it's not enough to write the switch record,
diff --git a/src/include/c.h b/src/include/c.h
index 14bfdcd..5ad97ea 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -534,11 +534,15 @@ typedef NameData *Name;
#define TYPEALIGN(ALIGNVAL,LEN) \
(((intptr_t) (LEN) + ((ALIGNVAL) - 1)) & ~((intptr_t) ((ALIGNVAL) - 1)))
+#define TYPEALIGN64(ALIGNVAL,LEN) \
+ (((uint64) (LEN) + ((ALIGNVAL) - 1)) & ~((uint64) ((ALIGNVAL) - 1)))
+
#define SHORTALIGN(LEN) TYPEALIGN(ALIGNOF_SHORT, (LEN))
#define INTALIGN(LEN) TYPEALIGN(ALIGNOF_INT, (LEN))
#define LONGALIGN(LEN) TYPEALIGN(ALIGNOF_LONG, (LEN))
#define DOUBLEALIGN(LEN) TYPEALIGN(ALIGNOF_DOUBLE, (LEN))
#define MAXALIGN(LEN) TYPEALIGN(MAXIMUM_ALIGNOF, (LEN))
+#define MAXALIGN64(LEN) TYPEALIGN64(MAXIMUM_ALIGNOF, (LEN))
/* MAXALIGN covers only built-in types, not buffers */
#define BUFFERALIGN(LEN) TYPEALIGN(ALIGNOF_BUFFER, (LEN))
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?
And does that indicate that intptr_t is the wrong type to be using here?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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?
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
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]And by anyone, I mean me. 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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Is anyone else getting these? I'm getting these for many of my -hackers posts.
---------- Forwarded message ----------
From: Mail Delivery System <MAILER-DAEMON@mail>
Date: Tue, Oct 8, 2013 at 3:48 PM
Subject: Undelivered Mail Returned to Sender
To: robertmhaas@gmail.com
This is the mail system at host mail.zehome.com.
I'm sorry to have to inform you that your message could not
be delivered to one or more recipients. It's attached below.
For further assistance, please send mail to postmaster.
If you do so, please include this problem report. You can
delete your own text from the attached returned message.
The mail system
<ed@zehome.com>: mail forwarding loop for ed@zehome.com
Final-Recipient: rfc822; ed@zehome.com
Original-Recipient: rfc822;ed@zehome.com
Action: failed
Status: 4.4.6
Diagnostic-Code: X-Postfix; mail forwarding loop for ed@zehome.com
---------- Forwarded message ----------
From: Robert Haas <robertmhaas@gmail.com>
To: Andres Freund <andres@2ndquadrant.com>
Cc: David Rowley <dgrowleyml@gmail.com>, PostgreSQL-development
<pgsql-hackers@postgresql.org>
Date: Mon, 7 Oct 2013 13:25:17 -0400
Subject: Re: [HACKERS] space reserved for WAL record does not match
what was written: panic on windows
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?
And does that indicate that intptr_t is the wrong type to be using here?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-www mailing list (pgsql-www@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-www
Import Notes
Reply to msg id not found: 20131008194816.CB4AC6047A@mail.zehome.com
Robert Haas <robertmhaas@gmail.com> wrote:
Is anyone else getting these? I'm getting these for many of my
-hackers posts.
This is the mail system at host mail.zehome.com.
I'm sorry to have to inform you that your message could not
be delivered to one or more recipients. It's attached below.
I just got one, and I have also seen very long delays on some of my
posts appearing on the list. Just for the last few days.
At around the same time this started, I started getting odd extra
linefeeds in many of my posts, too.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-www mailing list (pgsql-www@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-www
On 2013-10-08 12:26:17 -0400, Robert Haas wrote:
On Mon, Oct 7, 2013 at 4:47 PM, Andres Freund <andres@2ndquadrant.com> wrote:
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.
Do you have a better alternative? Making the computation unconditionally
64bit will have a runtime overhead and adding a StaticAssert in the
existing macro doesn't work because we use it in array sizes where gcc
balks.
We could try using inline functions, but that's not going to be pretty
either.
I don't really see that many further usecases that will align 64bit
values on 32bit platforms, so I think we're ok for now.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Oct 9, 2013 at 5:26 AM, Robert Haas <robertmhaas@gmail.com> wrote:
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.
The only improvement I thought of during writing the patch was to rename
MAXALIGN to something more related to RAM, like perhaps RAMMAXALIGN or
MEMORYMAXALIGN, so callers might think twice if they're using it for
anything apart from memory addresses. I didn't really come up with a name I
thought was good enough to warrant the change, so I left it as it was.
I also don't think it is perfect that both the marcos do the same thing on
a 64bit compilation, but I think I was in the same boat as you... couldn't
think of anything better.
If you can think of a name that will confuse users less then maybe it's
worth making the change.
David
Show quoted text
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company[1] And by anyone, I mean me.
On Tue, Oct 8, 2013 at 6:24 PM, Andres Freund <andres@2ndquadrant.com> wrote:
Do you have a better alternative? Making the computation unconditionally
64bit will have a runtime overhead and adding a StaticAssert in the
existing macro doesn't work because we use it in array sizes where gcc
balks.
We could try using inline functions, but that's not going to be pretty
either.I don't really see that many further usecases that will align 64bit
values on 32bit platforms, so I think we're ok for now.
I'd be inclined to make the computation unconditionally 64-bit. I
doubt the speed penalty is enough to worry about, and I think we're
going to have more and more cases where optimizing for 32-bit
platforms is just not the right decision.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-10-10 08:59:47 -0400, Robert Haas wrote:
On Tue, Oct 8, 2013 at 6:24 PM, Andres Freund <andres@2ndquadrant.com> wrote:
Do you have a better alternative? Making the computation unconditionally
64bit will have a runtime overhead and adding a StaticAssert in the
existing macro doesn't work because we use it in array sizes where gcc
balks.
We could try using inline functions, but that's not going to be pretty
either.I don't really see that many further usecases that will align 64bit
values on 32bit platforms, so I think we're ok for now.I'd be inclined to make the computation unconditionally 64-bit. I
doubt the speed penalty is enough to worry about, and I think we're
going to have more and more cases where optimizing for 32-bit
platforms is just not the right decision.
MAXALIGN is used in several of PG's hottest functions in many
scenarios. att_align_nominal is used in slot_deform_tuple,
heap_deform_tuple, nocachegetattr, etc. So I don't think that's viable
yet. At least not with much more benefit than this...
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 10/08/2013 10:23 PM, Kevin Grittner wrote:
Robert Haas <robertmhaas@gmail.com> wrote:
Is anyone else getting these? I'm getting these for many of my
-hackers posts.This is the mail system at host mail.zehome.com.
I'm sorry to have to inform you that your message could not
be delivered to one or more recipients. It's attached below.I just got one, and I have also seen very long delays on some of my
posts appearing on the list. Just for the last few days.At around the same time this started, I started getting odd extra
linefeeds in many of my posts, too.
can you point me to some specific mails that got either delayed
significantly and/or got extra linefeeds? - I find especially the later
very hard to believe...
Stefan
--
Sent via pgsql-www mailing list (pgsql-www@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-www
On Thu, Oct 10, 2013 at 03:23:30PM +0200, Andres Freund wrote:
On 2013-10-10 08:59:47 -0400, Robert Haas wrote:
On Tue, Oct 8, 2013 at 6:24 PM, Andres Freund <andres@2ndquadrant.com> wrote:
Do you have a better alternative? Making the computation unconditionally
64bit will have a runtime overhead and adding a StaticAssert in the
existing macro doesn't work because we use it in array sizes where gcc
balks.
We could try using inline functions, but that's not going to be pretty
either.I don't really see that many further usecases that will align 64bit
values on 32bit platforms, so I think we're ok for now.I'd be inclined to make the computation unconditionally 64-bit. I
doubt the speed penalty is enough to worry about, and I think we're
going to have more and more cases where optimizing for 32-bit
platforms is just not the right decision.MAXALIGN is used in several of PG's hottest functions in many
scenarios. att_align_nominal is used in slot_deform_tuple,
heap_deform_tuple, nocachegetattr, etc. So I don't think that's viable
yet. At least not with much more benefit than this...
Agreed. Besides performance, aligning a wider-than-pointer value is an
unusual need; authors should think thrice before doing that. I might have
even defined the MAXALIGN64 macro in xlog.c rather than a core header.
Incidentally, why does MAXALIGN64 use unsigned math while MAXALIGN uses signed
math?
--
Noah Misch
EnterpriseDB http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Oct 11, 2013 at 1:14 AM, Noah Misch <noah@leadboat.com> wrote:
On Thu, Oct 10, 2013 at 03:23:30PM +0200, Andres Freund wrote:
On 2013-10-10 08:59:47 -0400, Robert Haas wrote:
On Tue, Oct 8, 2013 at 6:24 PM, Andres Freund <andres@2ndquadrant.com> wrote:
Do you have a better alternative? Making the computation unconditionally
64bit will have a runtime overhead and adding a StaticAssert in the
existing macro doesn't work because we use it in array sizes where gcc
balks.
We could try using inline functions, but that's not going to be pretty
either.I don't really see that many further usecases that will align 64bit
values on 32bit platforms, so I think we're ok for now.I'd be inclined to make the computation unconditionally 64-bit. I
doubt the speed penalty is enough to worry about, and I think we're
going to have more and more cases where optimizing for 32-bit
platforms is just not the right decision.MAXALIGN is used in several of PG's hottest functions in many
scenarios. att_align_nominal is used in slot_deform_tuple,
heap_deform_tuple, nocachegetattr, etc. So I don't think that's viable
yet. At least not with much more benefit than this...Agreed. Besides performance, aligning a wider-than-pointer value is an
unusual need; authors should think thrice before doing that. I might have
even defined the MAXALIGN64 macro in xlog.c rather than a core header.Incidentally, why does MAXALIGN64 use unsigned math while MAXALIGN uses signed
math?
Well, if this is the consensus, then I think the dynamic shared memory
patch may need some revision. In that patch, I used uint64 to
represent the size of the dynamic shared memory segment, sort of on
the theory that we were going to use this to be allocating big chunks
of dynamic shared memory for stuff like parallel sort. In follow-on
patches I'm currently developing to actually do stuff with dynamic
shared memory, this results in extensive use of MAXALIGN64, and it
really kind of looks like it wants the whole set of alignment macros,
not just that one. So option one is to leave the dsm code alone and
add the rest of the macros.
But if we're bent on minimizing the use of 64-bit arithmetic on 32-bit
systems, then presumably I should instead go back and retrofit that
patch to use Size rather than uint64 to represent the size of a
segment. But then I have two concerns:
1. Is there any guarantee that sizeof(intptr_t) >= sizeof(size_t)?
(Note that Size is just a typedef for size_t, in c.h)
2. If intptr_t is a signed type, as it appears to be, and size_t is an
unsigned type, as I believe it to be, then is it safe to use the
macros written for the signed type with a value of the unsigned type?
Off-hand I can't see a problem there, but I'm not certain I'm not
missing something.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Oct 17, 2013 at 08:39:56AM -0400, Robert Haas wrote:
On Fri, Oct 11, 2013 at 1:14 AM, Noah Misch <noah@leadboat.com> wrote:
On Thu, Oct 10, 2013 at 03:23:30PM +0200, Andres Freund wrote:
On 2013-10-10 08:59:47 -0400, Robert Haas wrote:
I'd be inclined to make the computation unconditionally 64-bit. I
doubt the speed penalty is enough to worry about, and I think we're
going to have more and more cases where optimizing for 32-bit
platforms is just not the right decision.MAXALIGN is used in several of PG's hottest functions in many
scenarios. att_align_nominal is used in slot_deform_tuple,
heap_deform_tuple, nocachegetattr, etc. So I don't think that's viable
yet. At least not with much more benefit than this...Agreed. Besides performance, aligning a wider-than-pointer value is an
unusual need; authors should think thrice before doing that. I might have
even defined the MAXALIGN64 macro in xlog.c rather than a core header.Incidentally, why does MAXALIGN64 use unsigned math while MAXALIGN uses signed
math?Well, if this is the consensus, then I think the dynamic shared memory
patch may need some revision. In that patch, I used uint64 to
represent the size of the dynamic shared memory segment, sort of on
the theory that we were going to use this to be allocating big chunks
of dynamic shared memory for stuff like parallel sort. In follow-on
patches I'm currently developing to actually do stuff with dynamic
shared memory, this results in extensive use of MAXALIGN64, and it
really kind of looks like it wants the whole set of alignment macros,
not just that one. So option one is to leave the dsm code alone and
add the rest of the macros.But if we're bent on minimizing the use of 64-bit arithmetic on 32-bit
systems, then presumably I should instead go back and retrofit that
patch to use Size rather than uint64 to represent the size of a
segment. But then I have two concerns:
I'm not bent on _minimizing_ use of 64-bit arithmetic on 32-bit systems, but I
disfavor an addition of such usage rippling through various hot paths of the
system indiscriminately. Making a design choice to use *ALIGN64 in a
particular module doesn't bother me that way.
1. Is there any guarantee that sizeof(intptr_t) >= sizeof(size_t)?
(Note that Size is just a typedef for size_t, in c.h)
C99 doesn't require it, but I have never heard of a platform where it is
false. sizeof(intptr_t) > sizeof(size_t) systems have existed.
2. If intptr_t is a signed type, as it appears to be, and size_t is an
unsigned type, as I believe it to be, then is it safe to use the
macros written for the signed type with a value of the unsigned type?
Off-hand I can't see a problem there, but I'm not certain I'm not
missing something.
Yes; we do that all the time, e.g. the MAXALIGN call in AllocSetAlloc().
Looping back to my question above, I think it doesn't matter (on a two's
complement system) whether the macro uses a signed type or an unsigned type.
It changes the type of the result; that's all. Nonetheless, we should be
consistent about signedness between the regular and 64-bit macro variants.
Thanks,
nm
--
Noah Misch
EnterpriseDB http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Oct 17, 2013 at 12:33 PM, Noah Misch <noah@leadboat.com> wrote:
But if we're bent on minimizing the use of 64-bit arithmetic on 32-bit
systems, then presumably I should instead go back and retrofit that
patch to use Size rather than uint64 to represent the size of a
segment. But then I have two concerns:I'm not bent on _minimizing_ use of 64-bit arithmetic on 32-bit systems, but I
disfavor an addition of such usage rippling through various hot paths of the
system indiscriminately. Making a design choice to use *ALIGN64 in a
particular module doesn't bother me that way.
OK. Well that'd probably be simpler from my point of view but I'm not
100% sure. If we're going to allow that, then I think we need 64-bit
versions of all of the alignment macros. Anyone think that's a bad
idea?
1. Is there any guarantee that sizeof(intptr_t) >= sizeof(size_t)?
(Note that Size is just a typedef for size_t, in c.h)C99 doesn't require it, but I have never heard of a platform where it is
false. sizeof(intptr_t) > sizeof(size_t) systems have existed.
That's good, I think. Because if it weren't true then we'd
potentially need three versions of this macro: one for intptr_t,
another for size_t, and a third for uint64.
2. If intptr_t is a signed type, as it appears to be, and size_t is an
unsigned type, as I believe it to be, then is it safe to use the
macros written for the signed type with a value of the unsigned type?
Off-hand I can't see a problem there, but I'm not certain I'm not
missing something.Yes; we do that all the time, e.g. the MAXALIGN call in AllocSetAlloc().
Looping back to my question above, I think it doesn't matter (on a two's
complement system) whether the macro uses a signed type or an unsigned type.
It changes the type of the result; that's all. Nonetheless, we should be
consistent about signedness between the regular and 64-bit macro variants.
So, are you proposing using uintptr_t there instead of intptr_t? Or what?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-10-17 12:33:45 -0400, Noah Misch wrote:
But if we're bent on minimizing the use of 64-bit arithmetic on 32-bit
systems, then presumably I should instead go back and retrofit that
patch to use Size rather than uint64 to represent the size of a
segment. But then I have two concerns:I'm not bent on _minimizing_ use of 64-bit arithmetic on 32-bit systems, but I
disfavor an addition of such usage rippling through various hot paths of the
system indiscriminately. Making a design choice to use *ALIGN64 in a
particular module doesn't bother me that way.
I am fine with that as well. It seems unlikely that parallel sort or
whatever will be as hot as tuple deforming code on systems where 32bit
arithmetic is slow.
1. Is there any guarantee that sizeof(intptr_t) >= sizeof(size_t)?
(Note that Size is just a typedef for size_t, in c.h)C99 doesn't require it, but I have never heard of a platform where it is
false. sizeof(intptr_t) > sizeof(size_t) systems have existed.
Either way, both have to be at least 4byte on 32bit platforms and 8byte
on 64bit ones. So I as well think we're good.
2. If intptr_t is a signed type, as it appears to be, and size_t is an
unsigned type, as I believe it to be, then is it safe to use the
macros written for the signed type with a value of the unsigned type?
Off-hand I can't see a problem there, but I'm not certain I'm not
missing something.Yes; we do that all the time, e.g. the MAXALIGN call in AllocSetAlloc().
Looping back to my question above, I think it doesn't matter (on a two's
complement system) whether the macro uses a signed type or an unsigned type.
It changes the type of the result; that's all. Nonetheless, we should be
consistent about signedness between the regular and 64-bit macro variants.
I am not all that comfortable on relying on 2s complement here. Maybe we
want to compile without -fwrapv one day which would make signed overflow
undefined again. I don't think there are too many situations where the
compiler would have the required knowledge to optimize stuff away,
but...
So I vote for only using unsigned arithmetic. I don't see anything
preventing that?
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Oct 17, 2013 at 08:27:01PM +0200, Andres Freund wrote:
On 2013-10-17 12:33:45 -0400, Noah Misch wrote:
1. Is there any guarantee that sizeof(intptr_t) >= sizeof(size_t)?
(Note that Size is just a typedef for size_t, in c.h)C99 doesn't require it, but I have never heard of a platform where it is
false. sizeof(intptr_t) > sizeof(size_t) systems have existed.Either way, both have to be at least 4byte on 32bit platforms and 8byte
on 64bit ones. So I as well think we're good.
C99 does not have concepts like "32bit platform" and "64bit platform", so it
cannot make such a constraint. Nonetheless, I agree we're good with respect
to implementations actually worth anticipating.
2. If intptr_t is a signed type, as it appears to be, and size_t is an
unsigned type, as I believe it to be, then is it safe to use the
macros written for the signed type with a value of the unsigned type?
Off-hand I can't see a problem there, but I'm not certain I'm not
missing something.Yes; we do that all the time, e.g. the MAXALIGN call in AllocSetAlloc().
Looping back to my question above, I think it doesn't matter (on a two's
complement system) whether the macro uses a signed type or an unsigned type.
It changes the type of the result; that's all. Nonetheless, we should be
consistent about signedness between the regular and 64-bit macro variants.I am not all that comfortable on relying on 2s complement here. Maybe we
want to compile without -fwrapv one day which would make signed overflow
undefined again. I don't think there are too many situations where the
compiler would have the required knowledge to optimize stuff away,
but...
Here is my position statement on that issue:
/messages/by-id/20130220025819.GB6791@tornado.leadboat.com
So I vote for only using unsigned arithmetic. I don't see anything
preventing that?
TYPEALIGN has used signed math since the dawn of history, and TYPEALIGN64
departed from that precedent without comment. That led me to think of the
situation as prompting a fix for the oversight in TYPEALIGN64:
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -560,3 +560,3 @@ typedef NameData *Name;
#define TYPEALIGN64(ALIGNVAL,LEN) \
- (((uint64) (LEN) + ((ALIGNVAL) - 1)) & ~((uint64) ((ALIGNVAL) - 1)))
+ (((int64) (LEN) + ((ALIGNVAL) - 1)) & ~((int64) ((ALIGNVAL) - 1)))Having said that, changing the ancient macros to use uintptr_t does have the
advantage you mention, and I'm failing to think of a disadvantage.
--
Noah Misch
EnterpriseDB http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-10-17 18:04:34 -0400, Noah Misch wrote:
On Thu, Oct 17, 2013 at 08:27:01PM +0200, Andres Freund wrote:
On 2013-10-17 12:33:45 -0400, Noah Misch wrote:
1. Is there any guarantee that sizeof(intptr_t) >= sizeof(size_t)?
(Note that Size is just a typedef for size_t, in c.h)C99 doesn't require it, but I have never heard of a platform where it is
false. sizeof(intptr_t) > sizeof(size_t) systems have existed.Either way, both have to be at least 4byte on 32bit platforms and 8byte
on 64bit ones. So I as well think we're good.C99 does not have concepts like "32bit platform" and "64bit platform", so it
cannot make such a constraint. Nonetheless, I agree we're good with respect
to implementations actually worth anticipating.
But afaik we indirectly require either 4 or 8 byte pointers or in
configure. And we have a requirement for non-segmented memory afaics. So
both size_t and intptr_t have to be big enough to store a pointer. Which
in turn implies that they have to be at least 4/8 bytes.
Having said that, changing the ancient macros to use uintptr_t does have the
advantage you mention, and I'm failing to think of a disadvantage.
+1
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Oct 18, 2013 at 1:39 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Fri, Oct 11, 2013 at 1:14 AM, Noah Misch <noah@leadboat.com> wrote:
On Thu, Oct 10, 2013 at 03:23:30PM +0200, Andres Freund wrote:
On 2013-10-10 08:59:47 -0400, Robert Haas wrote:
On Tue, Oct 8, 2013 at 6:24 PM, Andres Freund <andres@2ndquadrant.com>
wrote:
Do you have a better alternative? Making the computation
unconditionally
64bit will have a runtime overhead and adding a StaticAssert in the
existing macro doesn't work because we use it in array sizes wheregcc
balks.
We could try using inline functions, but that's not going to bepretty
either.
I don't really see that many further usecases that will align 64bit
values on 32bit platforms, so I think we're ok for now.I'd be inclined to make the computation unconditionally 64-bit. I
doubt the speed penalty is enough to worry about, and I think we're
going to have more and more cases where optimizing for 32-bit
platforms is just not the right decision.MAXALIGN is used in several of PG's hottest functions in many
scenarios. att_align_nominal is used in slot_deform_tuple,
heap_deform_tuple, nocachegetattr, etc. So I don't think that's viable
yet. At least not with much more benefit than this...Agreed. Besides performance, aligning a wider-than-pointer value is an
unusual need; authors should think thrice before doing that. I mighthave
even defined the MAXALIGN64 macro in xlog.c rather than a core header.
Incidentally, why does MAXALIGN64 use unsigned math while MAXALIGN uses
signed
math?
Well, if this is the consensus, then I think the dynamic shared memory
patch may need some revision. In that patch, I used uint64 to
represent the size of the dynamic shared memory segment, sort of on
the theory that we were going to use this to be allocating big chunks
of dynamic shared memory for stuff like parallel sort. In follow-on
patches I'm currently developing to actually do stuff with dynamic
shared memory, this results in extensive use of MAXALIGN64, and it
really kind of looks like it wants the whole set of alignment macros,
not just that one. So option one is to leave the dsm code alone and
add the rest of the macros.
For me I don't really see why there's a need to use MAXALIGN64 for any
memory addresses related to RAM.
I only created MAXALIGN64 because I needed it to fix the WAL code which
needed as 64bit type on all platforms, not just 64bit ones. For me it made
perfect sense, so I'm a bit confused at most of this fuss. Though I do
understand that it's a bit weird that both macros are almost the same on a
64 bit machine...
As for signed vs unsigned, I've not looked at all of the places where
MAXALIGN is used, but I just assumed it was for memory addresses, if this
is the case then I'm confused why we'd ever want a negative valued memory
address?
This might be an obvious one, but can anyone tell me why the casts are in
the macro at all? Can a compiler not decide for itself which type it should
be using?
Regards
David Rowley
Show quoted text
But if we're bent on minimizing the use of 64-bit arithmetic on 32-bit
systems, then presumably I should instead go back and retrofit that
patch to use Size rather than uint64 to represent the size of a
segment. But then I have two concerns:1. Is there any guarantee that sizeof(intptr_t) >= sizeof(size_t)?
(Note that Size is just a typedef for size_t, in c.h)2. If intptr_t is a signed type, as it appears to be, and size_t is an
unsigned type, as I believe it to be, then is it safe to use the
macros written for the signed type with a value of the unsigned type?
Off-hand I can't see a problem there, but I'm not certain I'm not
missing something.--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Fri, Oct 18, 2013 at 09:05:38PM +1300, David Rowley wrote:
As for signed vs unsigned, I've not looked at all of the places where
MAXALIGN is used, but I just assumed it was for memory addresses, if this
is the case then I'm confused why we'd ever want a negative valued memory
address?
The result will invariably be cast to a pointer type before use, at which
point it's no longer negative. (That's not to say we should keep using signed
math, but it doesn't cause active problems for memory addresses.)
This might be an obvious one, but can anyone tell me why the casts are in
the macro at all? Can a compiler not decide for itself which type it should
be using?
The casts allow passing values of pointer type, which are not valid as
arguments to the bitwise AND operator.
--
Noah Misch
EnterpriseDB http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Oct 18, 2013 at 12:10:17AM +0200, Andres Freund wrote:
On 2013-10-17 18:04:34 -0400, Noah Misch wrote:
On Thu, Oct 17, 2013 at 08:27:01PM +0200, Andres Freund wrote:
On 2013-10-17 12:33:45 -0400, Noah Misch wrote:
1. Is there any guarantee that sizeof(intptr_t) >= sizeof(size_t)?
(Note that Size is just a typedef for size_t, in c.h)C99 doesn't require it, but I have never heard of a platform where it is
false. sizeof(intptr_t) > sizeof(size_t) systems have existed.Either way, both have to be at least 4byte on 32bit platforms and 8byte
on 64bit ones. So I as well think we're good.C99 does not have concepts like "32bit platform" and "64bit platform", so it
cannot make such a constraint. Nonetheless, I agree we're good with respect
to implementations actually worth anticipating.But afaik we indirectly require either 4 or 8 byte pointers or in
configure. And we have a requirement for non-segmented memory afaics. So
both size_t and intptr_t have to be big enough to store a pointer. Which
in turn implies that they have to be at least 4/8 bytes.
Conformance is possible in an implementation with 8-byte size_t and 4-byte
pointers. Filling in the details makes for a decent party game.
Having said that, changing the ancient macros to use uintptr_t does have the
advantage you mention, and I'm failing to think of a disadvantage.+1
Committed that way, then.
--
Noah Misch
EnterpriseDB http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers