Inaccurate comments in ReorderBufferCheckMemoryLimit()
Hi all,
While reading the code, I realized that the following code comments
might not be accurate:
/*
* Pick the largest transaction (or subtransaction) and evict it from
* memory by streaming, if possible. Otherwise, spill to disk.
*/
if (ReorderBufferCanStartStreaming(rb) &&
(txn = ReorderBufferLargestStreamableTopTXN(rb)) != NULL)
{
/* we know there has to be one, because the size is not zero */
Assert(txn && rbtxn_is_toptxn(txn));
Assert(txn->total_size > 0);
Assert(rb->size >= txn->total_size);
ReorderBufferStreamTXN(rb, txn);
}
AFAICS since ReorderBufferLargestStreamableTopTXN() returns only
top-level transactions, the comment above the if statement is not
right. It would not pick a subtransaction.
Also, I'm not sure that the second comment "we know there has to be
one, because the size is not zero" is right since there might not be
top-transactions that are streamable. I think this is why we say
"Otherwise, spill to disk".
I've attached a patch to fix these comments. Feedback is very welcome.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Attachments:
fix_comment.patchapplication/octet-stream; name=fix_comment.patchDownload
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 26d252bd87..bd643d96c6 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -3602,13 +3602,13 @@ ReorderBufferCheckMemoryLimit(ReorderBuffer *rb)
rb->size > 0))
{
/*
- * Pick the largest transaction (or subtransaction) and evict it from
- * memory by streaming, if possible. Otherwise, spill to disk.
+ * Pick the largest top-level (streamable) transaction evict it
+ * from memory by streaming, if possible. Otherwise, spill to
+ * disk.
*/
if (ReorderBufferCanStartStreaming(rb) &&
(txn = ReorderBufferLargestStreamableTopTXN(rb)) != NULL)
{
- /* we know there has to be one, because the size is not zero */
Assert(txn && rbtxn_is_toptxn(txn));
Assert(txn->total_size > 0);
Assert(rb->size >= txn->total_size);
On Mon, Jul 31, 2023 at 8:46 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
While reading the code, I realized that the following code comments
might not be accurate:/*
* Pick the largest transaction (or subtransaction) and evict it from
* memory by streaming, if possible. Otherwise, spill to disk.
*/
if (ReorderBufferCanStartStreaming(rb) &&
(txn = ReorderBufferLargestStreamableTopTXN(rb)) != NULL)
{
/* we know there has to be one, because the size is not zero */
Assert(txn && rbtxn_is_toptxn(txn));
Assert(txn->total_size > 0);
Assert(rb->size >= txn->total_size);ReorderBufferStreamTXN(rb, txn);
}AFAICS since ReorderBufferLargestStreamableTopTXN() returns only
top-level transactions, the comment above the if statement is not
right. It would not pick a subtransaction.
I think the subtransaction case is for the spill-to-disk case as both
cases are explained in the same comment.
Also, I'm not sure that the second comment "we know there has to be
one, because the size is not zero" is right since there might not be
top-transactions that are streamable.
I think this comment is probably referring to asserts related to the
size similar to spill to disk case.
How about if we just remove (or subtransaction) from the following
comment: "Pick the largest transaction (or subtransaction) and evict
it from memory by streaming, if possible. Otherwise, spill to disk."?
Then by referring to streaming/spill-to-disk cases, one can understand
in which cases only top-level xacts are involved and in which cases
both are involved.
--
With Regards,
Amit Kapila.
On Tue, Aug 1, 2023 at 11:33 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Mon, Jul 31, 2023 at 8:46 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
While reading the code, I realized that the following code comments
might not be accurate:/*
* Pick the largest transaction (or subtransaction) and evict it from
* memory by streaming, if possible. Otherwise, spill to disk.
*/
if (ReorderBufferCanStartStreaming(rb) &&
(txn = ReorderBufferLargestStreamableTopTXN(rb)) != NULL)
{
/* we know there has to be one, because the size is not zero */
Assert(txn && rbtxn_is_toptxn(txn));
Assert(txn->total_size > 0);
Assert(rb->size >= txn->total_size);ReorderBufferStreamTXN(rb, txn);
}AFAICS since ReorderBufferLargestStreamableTopTXN() returns only
top-level transactions, the comment above the if statement is not
right. It would not pick a subtransaction.I think the subtransaction case is for the spill-to-disk case as both
cases are explained in the same comment.Also, I'm not sure that the second comment "we know there has to be
one, because the size is not zero" is right since there might not be
top-transactions that are streamable.I think this comment is probably referring to asserts related to the
size similar to spill to disk case.How about if we just remove (or subtransaction) from the following
comment: "Pick the largest transaction (or subtransaction) and evict
it from memory by streaming, if possible. Otherwise, spill to disk."?
Then by referring to streaming/spill-to-disk cases, one can understand
in which cases only top-level xacts are involved and in which cases
both are involved.
Sounds good. I've updated the patch accordingly.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Attachments:
v2-0001-Fix-ReorderBufferCheckMemoryLimit-comment.patchapplication/octet-stream; name=v2-0001-Fix-ReorderBufferCheckMemoryLimit-comment.patchDownload
From 93f9518cabd904de7188823ffe1c8185737f80d3 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Tue, 1 Aug 2023 17:18:58 +0900
Subject: [PATCH v2] Fix ReorderBufferCheckMemoryLimit() comment.
Commit 7259736a6 updated the comment but it was not correct since
ReorderBufferLargestStreamableTopTXN() retuns only top-level
transactions.
Reviewed-by: Amit Kapila
Discussion: https://postgr.es/m/CAD21AoA9XB7OR86BqvrCe2dMYX%2BZv3-BvVmjF%3DGY2z6jN-kqjg%40mail.gmail.com
Backpatch-through: 14
---
src/backend/replication/logical/reorderbuffer.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 26d252bd87..87a4d2a24b 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -3602,8 +3602,8 @@ ReorderBufferCheckMemoryLimit(ReorderBuffer *rb)
rb->size > 0))
{
/*
- * Pick the largest transaction (or subtransaction) and evict it from
- * memory by streaming, if possible. Otherwise, spill to disk.
+ * Pick the largest transaction and evict it from memory by streaming,
+ * if possible. Otherwise, spill to disk.
*/
if (ReorderBufferCanStartStreaming(rb) &&
(txn = ReorderBufferLargestStreamableTopTXN(rb)) != NULL)
--
2.31.1
On Tue, Aug 1, 2023 at 2:06 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Tue, Aug 1, 2023 at 11:33 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Mon, Jul 31, 2023 at 8:46 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
While reading the code, I realized that the following code comments
might not be accurate:/*
* Pick the largest transaction (or subtransaction) and evict it from
* memory by streaming, if possible. Otherwise, spill to disk.
*/
if (ReorderBufferCanStartStreaming(rb) &&
(txn = ReorderBufferLargestStreamableTopTXN(rb)) != NULL)
{
/* we know there has to be one, because the size is not zero */
Assert(txn && rbtxn_is_toptxn(txn));
Assert(txn->total_size > 0);
Assert(rb->size >= txn->total_size);ReorderBufferStreamTXN(rb, txn);
}AFAICS since ReorderBufferLargestStreamableTopTXN() returns only
top-level transactions, the comment above the if statement is not
right. It would not pick a subtransaction.I think the subtransaction case is for the spill-to-disk case as both
cases are explained in the same comment.Also, I'm not sure that the second comment "we know there has to be
one, because the size is not zero" is right since there might not be
top-transactions that are streamable.I think this comment is probably referring to asserts related to the
size similar to spill to disk case.How about if we just remove (or subtransaction) from the following
comment: "Pick the largest transaction (or subtransaction) and evict
it from memory by streaming, if possible. Otherwise, spill to disk."?
Then by referring to streaming/spill-to-disk cases, one can understand
in which cases only top-level xacts are involved and in which cases
both are involved.Sounds good. I've updated the patch accordingly.
LGTM.
--
With Regards,
Amit Kapila.
On Wed, Aug 2, 2023 at 11:21 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Aug 1, 2023 at 2:06 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Tue, Aug 1, 2023 at 11:33 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Mon, Jul 31, 2023 at 8:46 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
While reading the code, I realized that the following code comments
might not be accurate:/*
* Pick the largest transaction (or subtransaction) and evict it from
* memory by streaming, if possible. Otherwise, spill to disk.
*/
if (ReorderBufferCanStartStreaming(rb) &&
(txn = ReorderBufferLargestStreamableTopTXN(rb)) != NULL)
{
/* we know there has to be one, because the size is not zero */
Assert(txn && rbtxn_is_toptxn(txn));
Assert(txn->total_size > 0);
Assert(rb->size >= txn->total_size);ReorderBufferStreamTXN(rb, txn);
}AFAICS since ReorderBufferLargestStreamableTopTXN() returns only
top-level transactions, the comment above the if statement is not
right. It would not pick a subtransaction.I think the subtransaction case is for the spill-to-disk case as both
cases are explained in the same comment.Also, I'm not sure that the second comment "we know there has to be
one, because the size is not zero" is right since there might not be
top-transactions that are streamable.I think this comment is probably referring to asserts related to the
size similar to spill to disk case.How about if we just remove (or subtransaction) from the following
comment: "Pick the largest transaction (or subtransaction) and evict
it from memory by streaming, if possible. Otherwise, spill to disk."?
Then by referring to streaming/spill-to-disk cases, one can understand
in which cases only top-level xacts are involved and in which cases
both are involved.Sounds good. I've updated the patch accordingly.
LGTM.
Thank you for reviewing the patch! Pushed.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com