[HACKERS] Re: space reserved for WAL record does not match what was written: panic on windows
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
[HACKERS] Re: space reserved for WAL record does not match what was written: panic on windows
On Fri, Oct 18, 2013 at 1:39 AM, Robert Haas wrote: > On Fri, Oct 11, 2013 at 1:14 AM, Noah Misch 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 > 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. > > 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. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company >
[HACKERS] Re: space reserved for WAL record does not match what was written: panic on windows
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
[HACKERS] Re: space reserved for WAL record does not match what was written: panic on windows
On Thu, Oct 17, 2013 at 12:33 PM, 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. 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
[HACKERS] Re: space reserved for WAL record does not match what was written: panic on windows
On Thu, Oct 17, 2013 at 08:39:56AM -0400, Robert Haas wrote: > On Fri, Oct 11, 2013 at 1:14 AM, Noah Misch 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
[HACKERS] Re: space reserved for WAL record does not match what was written: panic on windows
On Fri, Oct 11, 2013 at 1:14 AM, Noah Misch 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 >> > 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
[HACKERS] Re: space reserved for WAL record does not match what was written: panic on windows
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 > > 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
