Re: 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

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(at)leadboat(dot)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(at)leadboat(dot)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(at)gmx(dot)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(at)leadboat(dot)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(at)gmx(dot)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(at)leadboat(dot)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(at)2ndquadrant(dot)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