dsm use of uint64

Started by Robert Haasover 12 years ago7 messageshackers
Jump to latest
#1Robert Haas
robertmhaas@gmail.com

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

Attachments:

dsm-size.patchtext/x-patch; charset=US-ASCII; name=dsm-size.patchDownload+35-40
#2Noah Misch
noah@leadboat.com
In reply to: Robert Haas (#1)
Re: dsm use of uint64

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Robert Haas
robertmhaas@gmail.com
In reply to: Noah Misch (#2)
Re: dsm use of uint64

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Peter Eisentraut
peter_e@gmx.net
In reply to: Robert Haas (#3)
Re: dsm use of uint64

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&amp;dt=2013-11-01%2020%3A45%3A01&amp;stg=make

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5Robert Haas
robertmhaas@gmail.com
In reply to: Peter Eisentraut (#4)
Re: dsm use of uint64

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&amp;dt=2013-11-01%2020%3A45%3A01&amp;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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#6Andres Freund
andres@anarazel.de
In reply to: Robert Haas (#5)
Re: dsm use of uint64

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&amp;dt=2013-11-01%2020%3A45%3A01&amp;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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#7Robert Haas
robertmhaas@gmail.com
In reply to: Andres Freund (#6)
Re: dsm use of uint64

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers