On Mon, Oct 5, 2020 at 3:04 PM k.jami...@fujitsu.com <k.jami...@fujitsu.com> wrote: > > On Monday, October 5, 2020 3:30 PM, Amit Kapila wrote: > > > + for (i = 0; i < nforks; i++) > > + { > > + /* Get the total nblocks for a relation's fork */ nForkBlocks = > > + smgrcachednblocks(smgr_reln, forkNum[i]); > > + > > + if (nForkBlocks == InvalidBlockNumber) { nTotalBlocks = > > + InvalidBlockNumber; break; } nTotalBlocks += nForkBlocks; > > + nBlocksToInvalidate = nTotalBlocks - firstDelBlock[i]; } > > + > > + /* > > + * Do explicit hashtable probe if the total of nblocks of relation's > > + forks > > + * is not invalid and the nblocks to be invalidated is less than the > > + * full-scan threshold of buffer pool. Otherwise, full scan is executed. > > + */ > > + if (nTotalBlocks != InvalidBlockNumber && nBlocksToInvalidate < > > + BUF_DROP_FULL_SCAN_THRESHOLD) { for (j = 0; j < nforks; j++) { > > + BlockNumber curBlock; > > + > > + nForkBlocks = smgrcachednblocks(smgr_reln, forkNum[j]); > > + > > + for (curBlock = firstDelBlock[j]; curBlock < nForkBlocks; curBlock++) > > > > What if one or more of the forks doesn't have cached value? I think the > > patch > > will skip such forks and will invalidate/unpin buffers for others. > > Not having a cached value is equivalent to InvalidBlockNumber, right? > Maybe I'm missing something? But in the first loop we are already doing the > pre-check of whether or not one of the forks doesn't have cached value. > If it's not cached, then the nTotalBlocks is set to InvalidBlockNumber so we > won't need to enter the optimization loop and just execute the full scan > buffer > invalidation process. >
oh, I have missed that, so the existing code will work fine for that case. > > You probably > > need a local array of nForkBlocks which will be formed first time and then > > used in the second loop. You also in some way need to handle the case where > > that local array doesn't have cached blocks. > > Understood. that would be cleaner. > BlockNumber nForkBlocks[MAX_FORKNUM]; > > As for handling whether the local array is empty, I think the first loop > would cover it, > and there's no need to pre-check if the array is empty again in the second > loop. > for (i = 0; i < nforks; i++) > { > nForkBlocks[i] = smgrcachednblocks(smgr_reln, forkNum[i]); > > if (nForkBlocks[i] == InvalidBlockNumber) > { > nTotalBlocks = InvalidBlockNumber; > break; > } > nTotalBlocks += nForkBlocks[i]; > nBlocksToInvalidate = nTotalBlocks - firstDelBlock[i]; > } > This appears okay. > > 2. Also, the other thing is I have asked for some testing to avoid the small > > regression we have for a smaller number of shared buffers which I don't see > > the results nor any change in the code. I think it is better if you post the > > pending/open items each time you post a new version of the patch. > > Ah. Apologies for forgetting to include updates about that, but since I keep > on updating > the patch I've decided not to post results yet as performance may vary per > patch-update > due to possible bugs. > But for the performance case of not using recovery check, I just removed it > from below. > Does it meet the intention? > > BlockNumber smgrcachednblocks(SMgrRelation reln, ForkNumber forknum) { > - if (InRecovery && reln->smgr_cached_nblocks[forknum] != > InvalidBlockNumber) > + if (reln->smgr_cached_nblocks[forknum] != InvalidBlockNumber) > return reln->smgr_cached_nblocks[forknum]; > Yes, we can do that for the purpose of testing. -- With Regards, Amit Kapila.