Typo in bufmgr.c that result in waste of memory
Hi all,
I have just found a typo in the source code (not in a comment) of bufmgr.c
that result in waste of memory. It might be a 'bug' but it does not result
in any incorrect operation but just results in waste of a few memory
resource.
As sizeof(PrivateRefCountArray) found in InitBufferPoolAccess() is 64 and
sizeof(PrivateRefCountEntry) which should be used here is 8, this typo
produces 56 byte of unused memory area per one PrivateRefCount entry in the
hash table. I think this result in not only the waste of memory but also
reduces the cache hit ratio.
----
void
InitBufferPoolAccess(void)
{
HASHCTL hash_ctl;
memset(&PrivateRefCountArray, 0, sizeof(PrivateRefCountArray));
MemSet(&hash_ctl, 0, sizeof(hash_ctl));
hash_ctl.keysize = sizeof(int32);
X hash_ctl.entrysize = sizeof(PrivateRefCountArray);
O hash_ctl.entrysize = sizeof(PrivateRefCountEntry);
PrivateRefCountHash = hash_create("PrivateRefCount", 100, &hash_ctl,
HASH_ELEM | HASH_BLOBS);
}
----
Attached patch is for the latest development version at the git repository.
--
Takashi Horikawa
NEC Corporation
Knowledge Discovery Research Laboratories
Attachments:
correction_of_PrivateRefCount.patchapplication/octet-stream; name=correction_of_PrivateRefCount.patchDownload
diff --git src/backend/storage/buffer/bufmgr.c src/backend/storage/buffer/bufmgr.c
index 7141eb8..68cf5cc 100644
--- src/backend/storage/buffer/bufmgr.c
+++ src/backend/storage/buffer/bufmgr.c
@@ -2166,7 +2166,7 @@ InitBufferPoolAccess(void)
MemSet(&hash_ctl, 0, sizeof(hash_ctl));
hash_ctl.keysize = sizeof(int32);
- hash_ctl.entrysize = sizeof(PrivateRefCountArray);
+ hash_ctl.entrysize = sizeof(PrivateRefCountEntry);
PrivateRefCountHash = hash_create("PrivateRefCount", 100, &hash_ctl,
HASH_ELEM | HASH_BLOBS);
On Fri, Feb 19, 2016 at 8:28 AM, Takashi Horikawa <t-horikawa@aj.jp.nec.com>
wrote:
Hi all,
I have just found a typo in the source code (not in a comment) of bufmgr.c
that result in waste of memory. It might be a 'bug' but it does not result
in any incorrect operation but just results in waste of a few memory
resource.As sizeof(PrivateRefCountArray) found in InitBufferPoolAccess() is 64 and
sizeof(PrivateRefCountEntry) which should be used here is 8, this typo
produces 56 byte of unused memory area per one PrivateRefCount entry in
the
hash table. I think this result in not only the waste of memory but also
reduces the cache hit ratio.----
void
InitBufferPoolAccess(void)
{
HASHCTL hash_ctl;memset(&PrivateRefCountArray, 0, sizeof(PrivateRefCountArray));
MemSet(&hash_ctl, 0, sizeof(hash_ctl));
hash_ctl.keysize = sizeof(int32);
X hash_ctl.entrysize = sizeof(PrivateRefCountArray);
O hash_ctl.entrysize = sizeof(PrivateRefCountEntry);PrivateRefCountHash = hash_create("PrivateRefCount", 100, &hash_ctl,
HASH_ELEM | HASH_BLOBS);
}
----
Your proposed change seems right to me.
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On 19 February 2016 at 02:58, Takashi Horikawa <t-horikawa@aj.jp.nec.com>
wrote:
I have just found a typo in the source code (not in a comment) of bufmgr.c
that result in waste of memory. It might be a 'bug' but it does not result
in any incorrect operation but just results in waste of a few memory
resource.As sizeof(PrivateRefCountArray) found in InitBufferPoolAccess() is 64 and
sizeof(PrivateRefCountEntry) which should be used here is 8, this typo
produces 56 byte of unused memory area per one PrivateRefCount entry in the
hash table. I think this result in not only the waste of memory but also
reduces the cache hit ratio.X hash_ctl.entrysize = sizeof(PrivateRefCountArray);
O hash_ctl.entrysize = sizeof(PrivateRefCountEntry);
I see the problem, but I don't buy the argument that it wastes large
amounts of memory. Or do you have some evidence that it does?
I think we should fix it, but not backpatch.
--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
I see the problem, but I don't buy the argument that it wastes large amounts
of memory. Or do you have some evidence that it does?
No. I don’t have any trouble caused by it.
I think I did not mention it wastes 'large' amount of memory but 'a few'.
I think we should fix it, but not backpatch.
I agree that backpatch is unnecessary.
I hope it will be fixed for the current development version
at some suitable opportunity.
Best regards,
Takashi Horikawa
NEC Corporation
Knowledge Discovery Research Laboratories
Show quoted text
-----Original Message-----
From: pgsql-hackers-owner@postgresql.org
[mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Simon Riggs
Sent: Friday, February 19, 2016 6:24 PM
To: Horikawa Takashi(堀川 隆)
Cc: PostgreSQL Hackers
Subject: Re: [HACKERS] Typo in bufmgr.c that result in waste of memoryOn 19 February 2016 at 02:58, Takashi Horikawa <t-horikawa@aj.jp.nec.com>
wrote:I have just found a typo in the source code (not in a comment) of
bufmgr.c
that result in waste of memory. It might be a 'bug' but it does
not result
in any incorrect operation but just results in waste of a few memory
resource.As sizeof(PrivateRefCountArray) found in InitBufferPoolAccess()
is 64 and
sizeof(PrivateRefCountEntry) which should be used here is 8, this
typo
produces 56 byte of unused memory area per one PrivateRefCount entry
in the
hash table. I think this result in not only the waste of memory
but also
reduces the cache hit ratio.X hash_ctl.entrysize = sizeof(PrivateRefCountArray);
O hash_ctl.entrysize = sizeof(PrivateRefCountEntry);I see the problem, but I don't buy the argument that it wastes large amounts
of memory. Or do you have some evidence that it does?I think we should fix it, but not backpatch.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
Simon Riggs <simon@2ndQuadrant.com> writes:
I see the problem, but I don't buy the argument that it wastes large
amounts of memory. Or do you have some evidence that it does?
Agreed, it seems unlikely that that hash table gets large enough for
this to be really significant. Still ...
I think we should fix it, but not backpatch.
I don't think that's particularly good policy. It's a clear bug, why
would we not fix it? Leaving it as-is in the back branches can have
no good effect, and what it does do is create a merge hazard for other
back-patchable bug fixes in the same area.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi,
Nice catch!
On February 19, 2016 2:42:08 PM GMT+01:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I think we should fix it, but not backpatch.
I don't think that's particularly good policy. It's a clear bug, why
would we not fix it? Leaving it as-is in the back branches can have
no good effect, and what it does do is create a merge hazard for other
back-patchable bug fixes in the same area.
Agreed. Unless somebody beats be to it, I'll do do so in a couple hours (11h flight now.
Andres
---
Please excuse brevity and formatting - I am writing this on my mobile phone.--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Feb 19, 2016 at 7:20 PM, Andres Freund <andres@anarazel.de> wrote:
On February 19, 2016 2:42:08 PM GMT+01:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I think we should fix it, but not backpatch.
I don't think that's particularly good policy. It's a clear bug, why
would we not fix it? Leaving it as-is in the back branches can have
no good effect, and what it does do is create a merge hazard for other
back-patchable bug fixes in the same area.Agreed.
+1. I think this is clearly a back-patchable fix.
--
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
On Sat, Feb 20, 2016 at 01:55:55PM +0530, Robert Haas wrote:
On Fri, Feb 19, 2016 at 7:20 PM, Andres Freund <andres@anarazel.de> wrote:
On February 19, 2016 2:42:08 PM GMT+01:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I think we should fix it, but not backpatch.
I don't think that's particularly good policy. It's a clear bug, why
would we not fix it? Leaving it as-is in the back branches can have
no good effect, and what it does do is create a merge hazard for other
back-patchable bug fixes in the same area.Agreed.
+1. I think this is clearly a back-patchable fix.
Fix applied to head and 9.5.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers