Thread: dsm use of uint64
When I wrote the dynamic shared memory patch, I used uint64 everywhere to measure sizes - rather than, as we do for the main shared memory segment, Size. This now seems to me to have been the wrong decision; I'm finding that it's advantageous to make dynamic shared memory behave as much like the main shared memory segment as is reasonably possible, and using Size facilitates the use of MAXALIGN(), TYPEALIGN(), etc. as well as things like add_size() and mul_size() which are just as relevant in the dynamic shared memory case as they are for the main shared memory segment. Therefore, I propose to apply the attached patch. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
On Fri, Oct 25, 2013 at 10:11:41PM -0400, Robert Haas wrote: > When I wrote the dynamic shared memory patch, I used uint64 everywhere > to measure sizes - rather than, as we do for the main shared memory > segment, Size. This now seems to me to have been the wrong decision; > I'm finding that it's advantageous to make dynamic shared memory > behave as much like the main shared memory segment as is reasonably > possible, and using Size facilitates the use of MAXALIGN(), > TYPEALIGN(), etc. as well as things like add_size() and mul_size() > which are just as relevant in the dynamic shared memory case as they > are for the main shared memory segment. > > Therefore, I propose to apply the attached patch. +1. The simplicity of platform-independent type sizing had some attraction, but not so much to justify this sort of friction with the rest of the system. -- Noah Misch EnterpriseDB http://www.enterprisedb.com
On Sun, Oct 27, 2013 at 11:34 PM, Noah Misch <noah@leadboat.com> wrote: > On Fri, Oct 25, 2013 at 10:11:41PM -0400, Robert Haas wrote: >> When I wrote the dynamic shared memory patch, I used uint64 everywhere >> to measure sizes - rather than, as we do for the main shared memory >> segment, Size. This now seems to me to have been the wrong decision; >> I'm finding that it's advantageous to make dynamic shared memory >> behave as much like the main shared memory segment as is reasonably >> possible, and using Size facilitates the use of MAXALIGN(), >> TYPEALIGN(), etc. as well as things like add_size() and mul_size() >> which are just as relevant in the dynamic shared memory case as they >> are for the main shared memory segment. >> >> Therefore, I propose to apply the attached patch. > > +1. OK, committed. > The simplicity of platform-independent type sizing had some attraction, > but not so much to justify this sort of friction with the rest of the system. That's a good way of putting it. I'm repeatedly learning - invariably the hard way - that everything the main shared memory segment is or does needs a parallel for dynamic shared memory, and the closer the parallel, the better. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, 2013-10-28 at 12:17 -0400, Robert Haas wrote: > On Sun, Oct 27, 2013 at 11:34 PM, Noah Misch <noah@leadboat.com> wrote: > > On Fri, Oct 25, 2013 at 10:11:41PM -0400, Robert Haas wrote: > >> When I wrote the dynamic shared memory patch, I used uint64 everywhere > >> to measure sizes - rather than, as we do for the main shared memory > >> segment, Size. This now seems to me to have been the wrong decision; This change is now causing compiler warnings on 32-bit platforms. You can see them here, for example: http://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=lapwing&dt=2013-11-01%2020%3A45%3A01&stg=make
On Fri, Nov 1, 2013 at 11:45 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > On Mon, 2013-10-28 at 12:17 -0400, Robert Haas wrote: >> On Sun, Oct 27, 2013 at 11:34 PM, Noah Misch <noah@leadboat.com> wrote: >> > On Fri, Oct 25, 2013 at 10:11:41PM -0400, Robert Haas wrote: >> >> When I wrote the dynamic shared memory patch, I used uint64 everywhere >> >> to measure sizes - rather than, as we do for the main shared memory >> >> segment, Size. This now seems to me to have been the wrong decision; > > This change is now causing compiler warnings on 32-bit platforms. You > can see them here, for example: > http://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=lapwing&dt=2013-11-01%2020%3A45%3A01&stg=make Ah. This is because I didn't change the format code used to print the arguments; it's still using UINT64_FORMAT, but the argument is now a Size. What's the right way to print out a Size, anyway? Should I just try %lu? It seems that sysv_shmem.c uses %lu, but also casts the value to (unsigned long); I could follow that precedent here, if there's no consistency to what format is needed to print a size_t. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2013-11-04 10:46:06 -0500, Robert Haas wrote: > On Fri, Nov 1, 2013 at 11:45 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > > On Mon, 2013-10-28 at 12:17 -0400, Robert Haas wrote: > >> On Sun, Oct 27, 2013 at 11:34 PM, Noah Misch <noah@leadboat.com> wrote: > >> > On Fri, Oct 25, 2013 at 10:11:41PM -0400, Robert Haas wrote: > >> >> When I wrote the dynamic shared memory patch, I used uint64 everywhere > >> >> to measure sizes - rather than, as we do for the main shared memory > >> >> segment, Size. This now seems to me to have been the wrong decision; > > > > This change is now causing compiler warnings on 32-bit platforms. You > > can see them here, for example: > > http://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=lapwing&dt=2013-11-01%2020%3A45%3A01&stg=make > > Ah. This is because I didn't change the format code used to print the > arguments; it's still using UINT64_FORMAT, but the argument is now a > Size. What's the right way to print out a Size, anyway? There isn't a nice one currently. glibc/gcc have %z that automatically has the right width, but that's not supported by windows. I've been wondering if we shouldn't add support for that just like we have added support for %m. > Should I > just try %lu? It seems that sysv_shmem.c uses %lu, but also casts the > value to (unsigned long); I could follow that precedent here, if > there's no consistency to what format is needed to print a size_t. Yes, you need a cast like that. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Mon, Nov 4, 2013 at 10:55 AM, Andres Freund <andres@2ndquadrant.com> wrote: >> Ah. This is because I didn't change the format code used to print the >> arguments; it's still using UINT64_FORMAT, but the argument is now a >> Size. What's the right way to print out a Size, anyway? > > There isn't a nice one currently. glibc/gcc have %z that automatically > has the right width, but that's not supported by windows. I've been > wondering if we shouldn't add support for that just like we have added > support for %m. > >> Should I >> just try %lu? It seems that sysv_shmem.c uses %lu, but also casts the >> value to (unsigned long); I could follow that precedent here, if >> there's no consistency to what format is needed to print a size_t. > > Yes, you need a cast like that. Whee, portability is fun. OK, I changed it to work that way. Hopefully that'll do the trick. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company