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 TYAPR01MB2990368F53D794ACDEE13BEBFE380@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
Re: [Patch] Optimize dropping of relation buffers using dlist
List pgsql-hackers
From: Jamison, Kirk/ジャミソン カーク <k.jamison@fujitsu.com>
> I revised the patch based from my understanding of Horiguchi-san's comment,
> but I could be wrong.
> Quoting:
>
> "
> +            /* Get the number of blocks for the supplied relation's
> fork */
> +            nblocks = smgrnblocks(smgr_reln,
> forkNum[fork_num]);
> +            Assert(BlockNumberIsValid(nblocks));
> +
> +            if (nblocks < BUF_DROP_FULLSCAN_THRESHOLD)
>
> As mentioned upthread, the criteria whether we do full-scan or
> lookup-drop is how large portion of NBUFFERS this relation-drop can be
> going to invalidate.  So the nblocks above should be the sum of number
> of blocks to be truncated (not just the total number of blocks) of all
> designated forks.  Then once we decided to do lookup-drop method, we
> do that for all forks."

One takeaway from Horiguchi-san's comment is to use the number of blocks to invalidate for comparison, instead of all
blocksin the fork.  That is, use 

nblocks = smgrnblocks(fork) - firstDelBlock[fork];

Does this make sense?

What do you think is the reason for summing up all forks?  I didn't understand why.  Typically, FSM and VM forks are
verysmall.  If the main fork is larger than NBuffers / 500, then v14 scans the entire shared buffers for the FSM and VM
forksas well as the main fork, resulting in three scans in total. 

Also, if you want to judge the criteria based on the total blocks of all forks, the following if should be placed
outsidethe for loop, right?  Because this if condition doesn't change inside the for loop. 

+            if ((nblocks / (uint32)NBuffers) < BUF_DROP_FULLSCAN_THRESHOLD &&
+                BlockNumberIsValid(nblocks))
+            {



> > (2)
> > +            if ((nblocks / (uint32)NBuffers) <
> > BUF_DROP_FULLSCAN_THRESHOLD &&
> > +                BlockNumberIsValid(nblocks))
> > +            {
> >
> > The division by NBuffers is not necessary, because both sides of = are
> > number of blocks.
>
> Again I based it from my understanding of the comment above,
> so nblocks is the sum of all blocks to be truncated for all forks.

But the left expression of "<" is a percentage, while the right one is a block count.  Two different units are
compared.


> > Why is BlockNumberIsValid(nblocks)) call needed?
>
> I thought we need to ensure that nblocks is not invalid, so I also added

When is it invalid?  smgrnblocks() seems to always return a valid block number.  Am I seeing a different source code (I
sawHEAD)? 




Regards
Takayuki Tsunakawa




pgsql-hackers by date:

Previous
From: Amit Kapila
Date:
Subject: Re: [Patch] Optimize dropping of relation buffers using dlist
Next
From: "Hou, Zhijie"
Date:
Subject: RE: Use appendStringInfoString and appendPQExpBufferStr where possible