Re: [PATCH] Speedup truncates of relation forks

On Thu, Sep 5, 2019 at 5:53 PM Jamison, Kirk <k.jamison@jp.fujitsu.com> wrote:
>
> 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?

My this comment is pointless, so please ignore it. Sorry for noise..

Here are other comments for the latest patch:

+ block = visibilitymap_truncate_prepare(rel, 0);
+ if (BlockNumberIsValid(block))
+ fork = VISIBILITYMAP_FORKNUM;
+
+ smgrtruncate(rel->rd_smgr, &fork, 1, &block);

If visibilitymap_truncate_prepare() returns InvalidBlockNumber,
smgrtruncate() should not be called.

+ FreeSpaceMapVacuumRange(rel, first_removed_nblocks, InvalidBlockNumber);

FreeSpaceMapVacuumRange() should be called only when FSM exists,
like the original code does?

Regards,

-- 
Fujii Masao