On Tue, Apr 30, 2019 at 12:48 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Fri, Apr 26, 2019 at 10:46 AM John Naylor > <john.nay...@2ndquadrant.com> wrote: > > > > On Wed, Apr 17, 2019 at 2:04 AM Andres Freund <and...@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