Обсуждение: ShmemAlloc() alignment patch

In ShmemAlloc() we have:
 newStart = BUFFERALIGN(newStart); newSpace = (void *) (ShmemBase + newStart); return newSpace;

Notice that though newStart is ALIGNOF_BUFFER, ShmemBase is not. Thus the
newSpace is not aligned as we disired.

Attached please find the patch.

Regards,
Qingqing

Index: src/backend/storage/ipc/shmem.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/storage/ipc/shmem.c,v
retrieving revision 1.92
diff -c -r1.92 shmem.c
*** src/backend/storage/ipc/shmem.c     13 Jul 2006 16:49:16 -0000      1.92
--- src/backend/storage/ipc/shmem.c     14 Jul 2006 06:47:43 -0000
***************
*** 169,178 ****       SpinLockAcquire(ShmemLock);
       newStart = shmemseghdr->freeoffset;
       /* extra alignment for large requests, since they are probably
buffers */       if (size >= BLCKSZ)
!               newStart = BUFFERALIGN(newStart);
       newFree = newStart + size;       if (newFree <= shmemseghdr->totalsize)
--- 169,179 ----       SpinLockAcquire(ShmemLock);
       newStart = shmemseghdr->freeoffset;
+       newSpace = (void *) MAKE_PTR(newStart);
       /* extra alignment for large requests, since they are probably
buffers */       if (size >= BLCKSZ)
!               newStart = MAKE_OFFSET(BUFFERALIGN(newSpace));
       newFree = newStart + size;       if (newFree <= shmemseghdr->totalsize)

On Fri, Jul 14, 2006 at 02:50:31PM +0800, Qingqing Zhou wrote:
> In ShmemAlloc() we have:
>
>   newStart = BUFFERALIGN(newStart);
>   newSpace = (void *) (ShmemBase + newStart);
>   return newSpace;
>
> Notice that though newStart is ALIGNOF_BUFFER, ShmemBase is not. Thus the
> newSpace is not aligned as we disired.

How can ShmemBase not be aligned? Surely it's page-aligned?

Have a nice day,
--
Martijn van Oosterhout   <kleptog@svana.org>   http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

Martijn van Oosterhout <kleptog@svana.org> writes:
> On Fri, Jul 14, 2006 at 02:50:31PM +0800, Qingqing Zhou wrote:
>> Notice that though newStart is ALIGNOF_BUFFER, ShmemBase is not. Thus the
>> newSpace is not aligned as we disired.

> How can ShmemBase not be aligned? Surely it's page-aligned?

That's certainly what the code expects.  I'm disinclined to apply this
patch unless you can identify a real system where ShmemBase might not
point to a page boundary.

(Note: in a standalone backend, the "shared memory segment" is just a
huge malloc chunk, and so depending on your platform it might not be
page-aligned.  I don't feel a need to add cycles to ShmemAlloc to
optimize this case, though.  We only care about performance in the
normal shared-memory case.)
        regards, tom lane

Tom Lane wrote:
> Martijn van Oosterhout <kleptog@svana.org> writes:
> > On Fri, Jul 14, 2006 at 02:50:31PM +0800, Qingqing Zhou wrote:
> >> Notice that though newStart is ALIGNOF_BUFFER, ShmemBase is not. Thus the
> >> newSpace is not aligned as we disired.
> 
> > How can ShmemBase not be aligned? Surely it's page-aligned?
> 
> That's certainly what the code expects.  I'm disinclined to apply this
> patch unless you can identify a real system where ShmemBase might not
> point to a page boundary.
> 
> (Note: in a standalone backend, the "shared memory segment" is just a
> huge malloc chunk, and so depending on your platform it might not be
> page-aligned.  I don't feel a need to add cycles to ShmemAlloc to
> optimize this case, though.  We only care about performance in the
> normal shared-memory case.)

Should we add an assert?

--  Bruce Momjian   bruce@momjian.us EnterpriseDB    http://www.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> writes:
>>> How can ShmemBase not be aligned? Surely it's page-aligned?

> Should we add an assert?

No, because even if it's not page-aligned, there's no correctness issue
here.  (Besides, how would you know what the page size is on any given
platform?)
        regards, tom lane