Re: Unhappy about API changes in the no-fsm-for-small-rels patch

On Tue, Apr 30, 2019 at 12:48 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Apr 26, 2019 at 10:46 AM John Naylor
> <john.naylor@2ndquadrant.com> wrote:
> >
> > On Wed, Apr 17, 2019 at 2:04 AM Andres Freund <andres@anarazel.de> wrote:
> > >
> > > Hi,
> > >
> > > I'm somewhat unhappy in how much the no-fsm-for-small-rels exposed
> > > complexity that looks like it should be purely in freespacemap.c to
> > > callers.
> >
> > I took a stab at untying the free space code from any knowledge about
> > heaps, and made it the responsibility of each access method that calls
> > these free space routines to specify their own threshold (possibly
> > zero). The attached applies on top of HEAD, because it's independent
> > of the relcache logic being discussed. If I can get that working, the
> > I'll rebase it on top of this API, if you like.
> >
> > >  extern Size GetRecordedFreeSpace(Relation rel, BlockNumber heapBlk);
> > > -extern BlockNumber GetPageWithFreeSpace(Relation rel, Size spaceNeeded);
> > > +extern BlockNumber GetPageWithFreeSpace(Relation rel, Size spaceNeeded,
> > > +                    bool check_fsm_only);
> > >
> > > So now freespace.c has an argument that says we should only check the
> > > fsm. That's confusing. And it's not explained to callers what that
> > > argument means, and when it should be set.
> >
> > I split this up into 2 routines: GetPageWithFreeSpace() is now exactly
> > like it is in v11, and GetAlternatePage() is available for access
> > methods that can use it.
> >
>
> I don't much like the new function name GetAlternatePage, may be
> GetPageFromLocalFSM or something like that.  OTOH, I am not sure if we
> should go that far to address this concern of Andres's, maybe just
> adding a proper comment is sufficient.

That's a clearer name. I think 2 functions is easier to follow than
the boolean parameter.

> > Putting the thresholds in 3 files with completely different purposes
> > is a mess, and serves no example for future access methods, but I
> > don't have a better idea.
> >
>
> Yeah, I am also not sure if it is a good idea because it still won't
> be easy for pluggable storage especially the pg_upgrade part.  I think
> if we really want to make it easy for pluggable storage to define
> this, then we might need to build something along the lines of how to
> estimate relation size works.
>
> See how table_relation_estimate_size is defined and used
> and TableAmRoutine heapam_methods
> {
> ..
> relation_estimate_size
> }

That might be the best way for table ams, but I guess we'd still need
to keep the hard-coding for indexes to always have a FSM. That might
not be too bad.

-- 
John Naylor                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services