Thread: space reserved for WAL record does not match what was written: panic on windows
space reserved for WAL record does not match what was written: panic on windows
From
David Rowley
Date:
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
Re: space reserved for WAL record does not match what was written: panic on windows
From
Andres Freund
Date:
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
Re: space reserved for WAL record does not match what was written: panic on windows
From
David Rowley
Date:
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:Could it be that MAXALIGN/TYPEALIGN doesn't really work for values
> 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
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
Re: space reserved for WAL record does not match what was written: panic on windows
From
David Rowley
Date:
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:Could it be that MAXALIGN/TYPEALIGN doesn't really work for values
> 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
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
RegardsDavid
Attachment
Re: space reserved for WAL record does not match what was written: panic on windows
From
Robert Haas
Date:
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
Re: space reserved for WAL record does not match what was written: panic on windows
From
Andres Freund
Date:
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
Re: space reserved for WAL record does not match what was written: panic on windows
From
Heikki Linnakangas
Date:
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
Re: space reserved for WAL record does not match what was written: panic on windows
From
Robert Haas
Date:
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.
Re: space reserved for WAL record does not match what was written: panic on windows
From
Andres Freund
Date:
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
Re: space reserved for WAL record does not match what was written: panic on windows
From
David Rowley
Date:
On Wed, Oct 9, 2013 at 5:26 AM, Robert Haas <robertmhaas@gmail.com> wrote:
I think having two macros that behave identically on all platformsOn 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?
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
[1] And by anyone, I mean me.
Re: space reserved for WAL record does not match what was written: panic on windows
From
Robert Haas
Date:
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
Re: space reserved for WAL record does not match what was written: panic on windows
From
Andres Freund
Date:
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
Re: space reserved for WAL record does not match what was written: panic on windows
From
Noah Misch
Date:
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
Re: space reserved for WAL record does not match what was written: panic on windows
From
Robert Haas
Date:
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
Re: space reserved for WAL record does not match what was written: panic on windows
From
Noah Misch
Date:
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
Re: space reserved for WAL record does not match what was written: panic on windows
From
Robert Haas
Date:
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
Re: space reserved for WAL record does not match what was written: panic on windows
From
Andres Freund
Date:
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
signed vs. unsigned in TYPEALIGN (was Re: space reserved for WAL record does not match what was written: panic on windows)
From
Noah Misch
Date:
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: http://www.postgresql.org/message-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
Re: signed vs. unsigned in TYPEALIGN (was Re: space reserved for WAL record does not match what was written: panic on windows)
From
Andres Freund
Date:
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
Re: space reserved for WAL record does not match what was written: panic on windows
From
David Rowley
Date:
On Fri, Oct 18, 2013 at 1:39 AM, Robert Haas <robertmhaas@gmail.com> wrote:
Well, if this is the consensus, then I think the dynamic shared memoryOn 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?
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
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.
Re: space reserved for WAL record does not match what was written: panic on windows
From
Noah Misch
Date:
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
Re: signed vs. unsigned in TYPEALIGN (was Re: space reserved for WAL record does not match what was written: panic on windows)
From
Noah Misch
Date:
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