Double shared memory allocation for SLRU LWLocks
Hi, all!
It seems to me that we're allocating shared memory for SLRU lwlocks twice,
unless I'm missing something.
SimpleLruShmemSize() calculates total SLRU shared memory size including
lwlocks size.
SimpleLruInit() starts with line
shared = (SlruShared) ShmemInitStruct(name,
SimpleLruShmemSize(nslots, nlsns),
&found);
which allocates SLRU shared memory (LWLocks size is included because
SimpleLruShmemSize() is used for size computation).
Following line allocates shared memory for LWLocks again:
shared->buffer_locks = (LWLockPadded *) ShmemAlloc(sizeof(LWLockPadded) *
nslots);
Attached patch fixes that by removing extra ShmemAlloc for SLRU.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachments:
fix-slru-lwlock-shmem-double-allocation.patchapplication/octet-stream; name=fix-slru-lwlock-shmem-double-allocation.patchDownload
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
new file mode 100644
index 93ec653..927c3e4
*** a/src/backend/access/transam/slru.c
--- b/src/backend/access/transam/slru.c
*************** SimpleLruInit(SlruCtl ctl, const char *n
*** 205,219 ****
shared->page_lru_count = (int *) (ptr + offset);
offset += MAXALIGN(nslots * sizeof(int));
if (nlsns > 0)
{
shared->group_lsn = (XLogRecPtr *) (ptr + offset);
offset += MAXALIGN(nslots * nlsns * sizeof(XLogRecPtr));
}
- /* Initialize LWLocks */
- shared->buffer_locks = (LWLockPadded *) ShmemAlloc(sizeof(LWLockPadded) * nslots);
-
Assert(strlen(name) + 1 < SLRU_MAX_NAME_LENGTH);
strlcpy(shared->lwlock_tranche_name, name, SLRU_MAX_NAME_LENGTH);
shared->lwlock_tranche_id = tranche_id;
--- 205,220 ----
shared->page_lru_count = (int *) (ptr + offset);
offset += MAXALIGN(nslots * sizeof(int));
+ /* Initialize LWLocks */
+ shared->buffer_locks = (LWLockPadded *) (ptr + offset);
+ offset += MAXALIGN(nslots * sizeof(LWLockPadded));
+
if (nlsns > 0)
{
shared->group_lsn = (XLogRecPtr *) (ptr + offset);
offset += MAXALIGN(nslots * nlsns * sizeof(XLogRecPtr));
}
Assert(strlen(name) + 1 < SLRU_MAX_NAME_LENGTH);
strlcpy(shared->lwlock_tranche_name, name, SLRU_MAX_NAME_LENGTH);
shared->lwlock_tranche_id = tranche_id;
It seems to me that we're allocating shared memory for SLRU lwlocks twice,
unless I'm missing something.
I think you are right.
Did you check previous versions? i.e. should it be applyed to previous
branches?? I suppose yes, to minimize code difference.
Also I'd like an idea to add Assert(offset <= SimpleLruShmemSize(nslots, nlsns))
at the end of SimpleLruInit()
SimpleLruShmemSize() calculates total SLRU shared memory size including lwlocks
size.SimpleLruInit() starts with line
shared = (SlruShared) ShmemInitStruct(name,
SimpleLruShmemSize(nslots, nlsns),
&found);which allocates SLRU shared memory (LWLocks size is included because
SimpleLruShmemSize() is used for size computation).Following line allocates shared memory for LWLocks again:
shared->buffer_locks = (LWLockPadded *) ShmemAlloc(sizeof(LWLockPadded) * nslots);Attached patch fixes that by removing extra ShmemAlloc for SLRU.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
--
Teodor Sigaev E-mail: teodor@sigaev.ru
WWW: http://www.sigaev.ru/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jul 12, 2017 at 2:03 PM, Teodor Sigaev <teodor@sigaev.ru> wrote:
It seems to me that we're allocating shared memory for SLRU lwlocks twice,
unless I'm missing something.
I think you are right.
Did you check previous versions? i.e. should it be applyed to previous
branches?? I suppose yes, to minimize code difference.
Problem was introduced by fe702a7b. I think it would be good to backpatch
to 9.6. Besides it doesn't cause any user-visible problem, nevertheless
it's a bug.
Also I'd like an idea to add Assert(offset <= SimpleLruShmemSize(nslots,
nlsns)) at the end of SimpleLruInit()
Good point. Please, find it in attached patch.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachments:
fix-slru-lwlock-shmem-double-allocation-2.patchapplication/octet-stream; name=fix-slru-lwlock-shmem-double-allocation-2.patchDownload
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
new file mode 100644
index 93ec653..14d14cf
*** a/src/backend/access/transam/slru.c
--- b/src/backend/access/transam/slru.c
*************** SimpleLruInit(SlruCtl ctl, const char *n
*** 205,219 ****
shared->page_lru_count = (int *) (ptr + offset);
offset += MAXALIGN(nslots * sizeof(int));
if (nlsns > 0)
{
shared->group_lsn = (XLogRecPtr *) (ptr + offset);
offset += MAXALIGN(nslots * nlsns * sizeof(XLogRecPtr));
}
- /* Initialize LWLocks */
- shared->buffer_locks = (LWLockPadded *) ShmemAlloc(sizeof(LWLockPadded) * nslots);
-
Assert(strlen(name) + 1 < SLRU_MAX_NAME_LENGTH);
strlcpy(shared->lwlock_tranche_name, name, SLRU_MAX_NAME_LENGTH);
shared->lwlock_tranche_id = tranche_id;
--- 205,223 ----
shared->page_lru_count = (int *) (ptr + offset);
offset += MAXALIGN(nslots * sizeof(int));
+ /* Initialize LWLocks */
+ shared->buffer_locks = (LWLockPadded *) (ptr + offset);
+ offset += MAXALIGN(nslots * sizeof(LWLockPadded));
+
+ /* Should fit to estimated shmem size */
+ Assert(offset <= SimpleLruShmemSize(nslots, nlsns));
+
if (nlsns > 0)
{
shared->group_lsn = (XLogRecPtr *) (ptr + offset);
offset += MAXALIGN(nslots * nlsns * sizeof(XLogRecPtr));
}
Assert(strlen(name) + 1 < SLRU_MAX_NAME_LENGTH);
strlcpy(shared->lwlock_tranche_name, name, SLRU_MAX_NAME_LENGTH);
shared->lwlock_tranche_id = tranche_id;