RE: [PATCH] Speedup truncates of relation forks - Mailing list pgsql-hackers

On Wednesday, September 18, 2019 8:38 PM, Fujii Masao wrote:
> 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.u
> > > > s
> > >
> > > Yes. And I found Andres.
> > > https://www.postgresql.org/message-id/20180621174129.hogefyopje4xazn
> > > u@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.

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?


Thank you for committing the other patch as well!


Regards,
Kirk Jamison