Re: [PATCH] Speedup truncates of relation forks

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.hogefyopje4xazn
> > > > 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!

Regards,

--
Fujii Masao