RE: [PATCH] Speedup truncates of relation forks

On Tuesday, September 24, 2019 5:41 PM (GMT+9), Fujii Masao wrote:
> On Thu, Sep 19, 2019 at 9:42 AM Jamison, Kirk <k(dot)jamison(at)jp(dot)fujitsu(dot)com>
> wrote:
> >
> > On Wednesday, September 18, 2019 8:38 PM, Fujii Masao wrote:
> > > On Tue, Sep 17, 2019 at 10:44 AM Jamison, Kirk
> > > <k(dot)jamison(at)jp(dot)fujitsu(dot)com>
> > > wrote:
> > > >
> > > > On Friday, September 13, 2019 10:06 PM (GMT+9), Fujii Masao wrote:
> > > > > On Fri, Sep 13, 2019 at 9:51 PM Alvaro Herrera
> > > > > <alvherre(at)2ndquadrant(dot)com>
> > > > > wrote:
> > > > > >
> > > > > > On 2019-Sep-13, Fujii Masao wrote:
> > > > > >
> > > > > > > On Mon, Sep 9, 2019 at 3:52 PM Jamison, Kirk
> > > > > > > <k(dot)jamison(at)jp(dot)fujitsu(dot)com>
> > > > > wrote:
> > > > > >
> > > > > > > > > Please add a preliminary patch that removes the function.
> > > > > > > > > Dead code is good, as long as it is gone. We can get it
> > > > > > > > > pushed ahead of
> > > > > the rest of this.
> > > > > > > >
> > > > > > > > Alright. I've attached a separate patch removing the
> > > smgrdounlinkfork.
> > > > > > >
> > > > > > > Per the past discussion, some people want to keep this "dead"
> > > > > > > function for some reasons. So, in my opinion, it's better to
> > > > > > > just enclose the function with #if NOT_USED and #endif, to
> > > > > > > keep the function itself as it is, and then to start new
> > > > > > > discussion on hackers about the removal of that separatedly from
> this patch.
> > > > > >
> > > > > > I searched for anybody requesting to keep the function. I
> > > > > > couldn't find anything. Tom said in 2012:
> > > > > > https://www.postgresql.org/message-id/1471.1339106082@sss.pgh.
> > > > > > pa.u
> > > > > > s
> > > > >
> > > > > Yes. And I found Andres.
> > > > > https://www.postgresql.org/message-id/20180621174129.hogefyopje4
> > > > > xazn
> > > > > u(at)al
> > > > > ap3.anarazel.de
> > > > >
> > > > > > > As committed, the smgrdounlinkfork case is actually dead
> > > > > > > code; it's never called from anywhere. I left it in place
> > > > > > > just in case we want it someday.
> > > > > >
> > > > > > but if no use has appeared in 7 years, I say it's time to kill it.
> > > > >
> > > > > +1
> > > >
> > > > The consensus is we remove it, right?
> > > > Re-attaching the patch that removes the deadcode: smgrdounlinkfork().
> > > >
> > > > ---
> > > > I've also fixed Fujii-san's comments below in the latest attached
> > > > speedup
> > > truncate rel patch (v8).
> > >
> > > Thanks for updating the patch!
> > >
> > > + block = visibilitymap_truncate_prepare(rel, 0); if
> > > + (BlockNumberIsValid(block))
> > > {
> > > - xl_smgr_truncate xlrec;
> > > + fork = VISIBILITYMAP_FORKNUM;
> > > + smgrtruncate(rel->rd_smgr, &fork, 1, &block);
> > > +
> > > + if (RelationNeedsWAL(rel))
> > > + {
> > > + xl_smgr_truncate xlrec;
> > >
> > > I don't think this fix is right. Originally, WAL is generated even
> > > in the case where visibilitymap_truncate_prepare() returns
> > > InvalidBlockNumber. But the patch unexpectedly changed the logic so
> > > that WAL is not generated in that case.
> > >
> > > + if (fsm)
> > > + FreeSpaceMapVacuumRange(rel, first_removed_nblocks,
> > > + InvalidBlockNumber);
> > >
> > > This code means that FreeSpaceMapVacuumRange() is called if FSM
> > > exists even if FreeSpaceMapLocateBlock() returns InvalidBlockNumber.
> > > This seems not right. Originally, FreeSpaceMapVacuumRange() is not
> > > called in the case where InvalidBlockNumber is returned.
> > >
> > > So I updated the patch based on yours and fixed the above issues.
> > > Attached. Could you review this one? If there is no issue in that,
> > > I'm thinking to commit that.
> >
> > Oops. Thanks for the catch to correct my fix and revision of some
> descriptions.
> > I also noticed you reordered the truncation of forks, by which main
> > fork will be truncated first instead of FSM. I'm not sure if the order
> > matters now given that we're truncating the forks simultaneously, so I'm
> ok with that change.
>
> I changed that order so that DropRelFileNodeBuffers() can scan shared_buffers
> more efficiently. Usually the number of buffers for MAIN fork is larger than
> the others, in shared_buffers. So it's better to compare MAIN fork first for
> performance, during full scan of shared_buffers.
>
> > Just one minor comment:
> > + * Return the number of blocks of new FSM after it's truncated.
> >
> > "after it's truncated" is quite confusing.
> > How about, "as a result of previous truncation" or just end the sentence
> after new FSM?
>
> Thanks for the comment!
> I adopted the latter and committed the patch. Thanks!

Thank you very much Fujii-san for taking time to review
as well as for committing this patch!

Regards,
Kirk Jamison