RE: [Patch] Optimize dropping of relation buffers using dlist

On Thursday, October 8, 2020 3:38 PM, Tsunakawa-san wrote:

> Hi Kirk san,
Thank you for looking into my patches!

> (1)
> + * This returns an InvalidBlockNumber when smgr_cached_nblocks is not
> + * available and when not in recovery path.
>
> + /*
> + * We cannot believe the result from smgr_nblocks is always accurate
> + * because lseek of buggy Linux kernels doesn't account for a recent
> + * write.
> + */
> + if (!InRecovery && result == InvalidBlockNumber)
> + return InvalidBlockNumber;
> +
>
> These are unnecessary, because mdnblocks() never returns
> InvalidBlockNumber and conseuently smgrnblocks() doesn't return
> InvalidBlockNumber.

Yes. Thanks for carefully looking into that. Removed.

> (2)
> +smgrnblocks(SMgrRelation reln, ForkNumber forknum, bool *isCached)
>
> I think it's better to make the argument name iscached so that camel case
> aligns with forknum, which is not forkNum.

This is kinda tricky because of the surrounding code which follows inconsistent coding style too.
So I just followed the same like below and retained the change.

extern void smgrcreate(SMgrRelation reln, ForkNumber forknum, bool isRedo);
extern void smgrextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, char *buffer, bool skipFsync);

> (3)
> + * This is caused by buggy Linux kernels that might not have
> accounted
> + * the recent write. If a fork's nblocks is invalid, exit loop.
>
> "accounted for" is the right English?
> I think The second sentence should be described in terms of its meaning, not
> the program logic. For example, something like "Give up the optimization if
> the block count of any fork cannot be trusted."

Fixed.

> Likewise, express the following part in semantics.
>
> + * Do explicit hashtable lookup if the total of nblocks of relation's
> forks
> + * is not invalid and the nblocks to be invalidated is less than the

I revised it like below:
"Look up the buffer in the hashtable if the block size is known to
be accurate and the total blocks to be invalidated is below the
full scan threshold. Otherwise, give up the optimization."

> (4)
> + if (nForkBlocks[i] == InvalidBlockNumber)
> + {
> + nTotalBlocks = InvalidBlockNumber;
> + break;
> + }
>
> Use isCached in if condition because smgrnblocks() doesn't return
> InvalidBlockNumber.

Fixed. if (!isCached)

> (5)
> + nBlocksToInvalidate = nTotalBlocks - firstDelBlock[i];
>
> should be
>
> + nBlocksToInvalidate += (nForkBlocks[i] - firstDelBlock[i]);

Fixed.

> (6)
> + bufHdr->tag.blockNum >=
> firstDelBlock[j])
> + InvalidateBuffer(bufHdr); /*
> releases spinlock */
>
> The right side of >= should be cur_block.

Fixed.

Attached are the updated patches.
Thank you again for the reviews.

Regards,
Kirk Jamison