[prev in list] [next in list] [prev in thread] [next in thread] 

List:       postgresql-hackers
Subject:    Inaccurate comments in ReorderBufferCheckMemoryLimit()
From:       Masahiko Sawada <sawada.mshk () gmail ! com>
Date:       2023-07-31 15:15:25
Message-ID: CAD21AoA9XB7OR86BqvrCe2dMYX+Zv3-BvVmjF=GY2z6jN-kqjg () mail ! gmail ! com
[Download RAW message or body]

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

["fix_comment.patch" (application/octet-stream)]

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);



[prev in list] [next in list] [prev in thread] [next in thread]