Re: [PATCH] Speedup truncates of relation forks

On Tue, Sep 17, 2019 at 10:44 AM Jamison, Kirk <k.jamison@jp.fujitsu.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@2ndquadrant.com>
> > wrote:
> > >
> > > On 2019-Sep-13, Fujii Masao wrote:
> > >
> > > > On Mon, Sep 9, 2019 at 3:52 PM Jamison, Kirk <k.jamison@jp.fujitsu.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.us
> >
> > Yes. And I found Andres.
> > https://www.postgresql.org/message-id/20180621174129.hogefyopje4xaznu@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.

Regards,

-- 
Fujii Masao