Fix comment for max_cached_tuplebufs definition

Started by Kuntal Ghoshalmost 6 years ago2 messages

kuntalghosh.2007@gmail.com

almost 6 years ago

1 attachment(s)

Hello Hackers,

While working on some issue in logical decoding, I found some
inconsistencies in the comment for defining max_cached_tuplebufs in
reorderbuffer.c. It only exists till PG10 because after that the
definition got removed by the generational memory allocator patch. The
variable is defined as follows in reorderbuffer.c:
static const Size max_cached_tuplebufs = 4096 * 2; /* ~8MB */

And it gets compared with rb->nr_cached_tuplebufs in
ReorderBufferReturnTupleBuf as follows:
if (tuple->alloc_tuple_size == MaxHeapTupleSize &&
rb->nr_cached_tuplebufs < max_cached_tuplebufs)

{
rb->nr_cached_tuplebufs++;
}

So, what this variable actually tracks is 4096 * 2 times
MaxHeapTupleSize amount of memory which is approximately 64MB. I've
attached a patch to modify the comment.

But, I'm not sure whether the intention was to keep 8MB cache only. In
that case, I can come up with another patch.

Thoughts?

--
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

Attachments:

0001-Fix-comment-for-max_cached_tuplebufs-definition.patchapplication/octet-stream; name=0001-Fix-comment-for-max_cached_tuplebufs-definition.patchDownload

From 07ee0e4c860e0c16a41809a175166f3837988901 Mon Sep 17 00:00:00 2001
From: Kuntal Ghosh <kuntal.ghosh@enterprisedb.com>
Date: Fri, 7 Feb 2020 14:44:37 +0530
Subject: [PATCH] Fix comment for max_cached_tuplebufs definition

---
 src/backend/replication/logical/reorderbuffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 0f607bab70..c970324b63 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -157,7 +157,7 @@ static const Size max_changes_in_memory = 4096;
  * major bottleneck, especially when spilling to disk while decoding batch
  * workloads.
  */
-static const Size max_cached_tuplebufs = 4096 * 2;	/* ~8MB */
+static const Size max_cached_tuplebufs = 4096 * 2;	/* ~64MB */
 
 /* ---------------------------------------
  * primary reorderbuffer support routines
-- 
2.17.1

Bruce Momjian

bruce@momjian.us

almost 6 years ago

In reply to: Kuntal Ghosh (#1)

Re: Fix comment for max_cached_tuplebufs definition

On Fri, Feb 7, 2020 at 02:49:14PM +0530, Kuntal Ghosh wrote:

Hello Hackers,

While working on some issue in logical decoding, I found some
inconsistencies in the comment for defining max_cached_tuplebufs in
reorderbuffer.c. It only exists till PG10 because after that the
definition got removed by the generational memory allocator patch. The
variable is defined as follows in reorderbuffer.c:
static const Size max_cached_tuplebufs = 4096 * 2; /* ~8MB */

And it gets compared with rb->nr_cached_tuplebufs in
ReorderBufferReturnTupleBuf as follows:
if (tuple->alloc_tuple_size == MaxHeapTupleSize &&
rb->nr_cached_tuplebufs < max_cached_tuplebufs)

{
rb->nr_cached_tuplebufs++;
}

So, what this variable actually tracks is 4096 * 2 times
MaxHeapTupleSize amount of memory which is approximately 64MB. I've
attached a patch to modify the comment.

But, I'm not sure whether the intention was to keep 8MB cache only. In
that case, I can come up with another patch.

Yes, I see you are correct, since each tuplebuf is MaxHeapTupleSize.
Patch applied from PG 9.5 to PG 10. Thanks.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +