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

At Thu, 3 Dec 2020 07:18:16 +0000, "tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com> wrote in 
> From: Jamison, Kirk/ジャミソン カーク <k.jamison@fujitsu.com>
> > Apologies for the delay, but attached are the updated versions to simplify the
> > patches.
> 
> Looks good for me.  Thanks to Horiguchi-san and Andres-san, the code bebecame further compact and easier to read.
I'vemarked this ready for committer.
 
> 
> 
> To the committer:
> I don't think it's necessary to refer to COMMIT/ROLLBACK PREPARED in the following part of the 0003 commit message.
Theysurely call DropRelFileNodesAllBuffers(), but COMMIT/ROLLBACK also call it.
 
> 
> the full scan threshold. This improves the DropRelationFiles()
> performance when the TRUNCATE command truncated off any of the empty
> pages at the end of relation, and when dropping relation buffers if a
> commit/rollback transaction has been prepared in FinishPreparedTransaction().

I think whether we can use this optimization only by looking
InRecovery is still in doubt.  Or if we can decide that on that
criteria, 0003 also can be simplivied using the same assumption.


Separate from the maybe-remaining discussion, I have a comment on the
revised code in 0004.

+         * equal to the full scan threshold.
+         */
+        if (nBlocksToInvalidate >= BUF_DROP_FULL_SCAN_THRESHOLD)
+        {
+            pfree(block);
+            goto buffer_full_scan;
+        }

I don't particularily hate goto statement but we can easily avoid that
by reversing the condition here.  You might consider the length of the
line calling "FindAndDropRelFileNodeBuffers" but the indentation can
be lowered by inverting the condition on BLockNumberIsValid.

!| if (nBlocksToInvalidate < BUF_DROP_FULL_SCAN_THRESHOLD)
 | {
 |     for (i = 0; i < n; i++)
 |     {
 |         /*
 |          * If block to drop is valid, drop the buffers of the fork.
 |          * Zero the firstDelBlock because all buffers will be
 |          * dropped anyway.
 |          */
 |         for (j = 0; j <= MAX_FORKNUM; j++)
 |         {
!|             if (!BlockNumberIsValid(block[i][j]))
!|                 continue;
 | 
 |             FindAndDropRelFileNodeBuffers(smgr_reln[i]->smgr_rnode.node,
 |                                           j, block[i][j], 0);
 |         }
 |     }
 |     pfree(block);
 |     return;
 | }
 | 
 | pfree(block);

Or we can separate the calcualtion part and the execution part by
introducing a flag "do_fullscan".

 |    /*
 |     * We enter the optimization iff we are in recovery.  Otherwise,
 |     * we proceed to full scan of the whole buffer pool.
 |     */
 |    if (InRecovery)
 |    {
...
!|         if (nBlocksToInvalidate < BUF_DROP_FULL_SCAN_THRESHOLD)
!|            do_fullscan = false;
!|    }
!|
!|    if (!do_fullscan)
!|    {
 |        for (i = 0; i < n; i++)
 |        {
 |            /*
 |             * If block to drop is valid, drop the buffers of the fork.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center