On Tuesday, September 3, 2019 9:44 PM (GMT+9), Fujii Masao wrote:
> Thanks for the patch!
Thank you as well for the review!
> -smgrdounlinkfork(SMgrRelation reln, ForkNumber forknum, bool isRedo)
> +smgrdounlinkfork(SMgrRelation reln, ForkNumber *forknum, int nforks,
> bool isRedo)
>
> smgrdounlinkfork() is dead code. Per the discussion [1], this unused function
> was left intentionally. But it's still dead code since 2012, so I'd like to
> remove it. Or, even if we decide to keep that function for some reasons, I
> don't think that we need to update that so that it can unlink multiple forks
> at once. So, what about keeping
> smgrdounlinkfork() as it is?
>
> [1]
> https://www.postgresql.org/message-id/1471.1339106082@sss.pgh.pa.us
I also mentioned it from my first post if we can just remove this dead code.
If not, it would require to modify the function because it would also
need nforks as input argument when calling DropRelFileNodeBuffers. I kept my
changes in the latest patch.
So should I remove the function now or keep my changes?
> + for (int i = 0; i < nforks; i++)
>
> The variable "i" should not be declared in for loop per PostgreSQL coding
> style.
Fixed.
> + /* Check with the lower bound block number and skip the loop */ if
> + (bufHdr->tag.blockNum < minBlock) continue; /* skip checking the
> + buffer pool scan */
>
> Because of the above code, the following source comment in bufmgr.c should
> be updated.
>
> * We could check forkNum and blockNum as well as the rnode, but the
> * incremental win from doing so seems small.
>
> And, first of all, is this check really useful for performance?
> Since firstDelBlock for FSM fork is usually small, minBlock would also be
> small. So I'm not sure how much this is helpful for performance.
This was a suggestion from Sawada-san in the previous email,
but he also thought that the performance benefit might be small..
so I just removed the related code block in this patch.
> When relation is completely truncated at all (i.e., the number of block to
> delete first is zero), can RelationTruncate() and smgr_redo() just call
> smgrdounlinkall() like smgrDoPendingDeletes() does, instead of calling
> MarkFreeSpaceMapTruncateRel(), visibilitymap_truncate_prepare() and
> smgrtruncate()? ISTM that smgrdounlinkall() is faster and simpler.
I haven't applied this in my patch yet.
If my understanding is correct, smgrdounlinkall() is used for deleting
relation forks. However, we only truncate (not delete) relations
in RelationTruncate() and smgr_redo(). I'm not sure if it's correct to
use it here. Could you expound more your idea on using smgrdounlinkall?
Regards,
Kirk Jamison
