RE: [Patch] Optimize dropping of relation buffers using dlist - Mailing list pgsql-hackers

From tsunakawa.takay@fujitsu.com
Subject RE: [Patch] Optimize dropping of relation buffers using dlist
Date
Msg-id TYAPR01MB2990E564DF4D0902E639A191FE040@TYAPR01MB2990.jpnprd01.prod.outlook.com
Whole thread Raw
In response to RE: [Patch] Optimize dropping of relation buffers using dlist  ("k.jamison@fujitsu.com" <k.jamison@fujitsu.com>)
Responses RE: [Patch] Optimize dropping of relation buffers using dlist
List pgsql-hackers
From: Jamison, Kirk/ジャミソン カーク <k.jamison@fujitsu.com>


(1)
> Alright. I also removed nTotalBlocks in v24-0003 patch.
>
> for (i = 0; i < nforks; i++)
> {
>     if (nForkBlocks[i] != InvalidBlockNumber &&
>         nBlocksToInvalidate < BUF_DROP_FULL_SCAN_THRESHOLD)
>     {
>         Optimization loop
>     }
>     else
>         break;
> }
> if (i >= nforks)
>     return;
> { usual buffer invalidation process }

Why do you do this way?  I think the previous patch was more correct (while agreeing with Horiguchi-san in that
nTotalBlocksmay be unnecessary.  What you want to do is "if the size of any fork could be inaccurate, do the
traditionalfull buffer scan without performing any optimization for any fork," right?  But the above code performs
optimizationfor forks until it finds a fork with inaccurate size. 

(2)
+     * Get the total number of cached blocks and to-be-invalidated blocks
+     * of the relation.  The cached value returned by smgrnblocks could be
+     * smaller than the actual number of existing buffers of the file.

As you changed the meaning of the smgrnblocks() argument from cached to accurate, and you nolonger calculate the total
blocks,the comment should reflect them. 


(3)
In smgrnblocks(), accurate is not set to false when mdnblocks() is called.  The caller doesn't initialize the value
either,so it can see garbage value. 


(4)
+        if (nForkBlocks[i] != InvalidBlockNumber &&
+            nBlocksToInvalidate < BUF_DROP_FULL_SCAN_THRESHOLD)
+        {
...
+        }
+        else
+            break;
+    }

In cases like this, it's better to reverse the if and else.  Thus, you can reduce the nest depth.


 Regards
Takayuki Tsunakawa




pgsql-hackers by date:

Previous
From: Kyotaro Horiguchi
Date:
Subject: Re: Transactions involving multiple postgres foreign servers, take 2
Next
From: Kasahara Tatsuhito
Date:
Subject: Re: Add a description to the documentation that toast_tuple_target affects "Main"