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

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.

> +/* Only create the FSM if the heap has greater than this many blocks */
> +#define HEAP_FSM_CREATION_THRESHOLD 4
>
> Hm, this seems to be tying freespace.c closer to heap than I think is
> great - think of new AMs like zheap, that also want to use it.

This was a bit harder than expected. Because of the pg_upgrade
optimization, it was impossible to put this symbol in hio.h or
heapam.h, because they include things unsafe for frontend code. I
decided to create heap_fe.h, which is a hack. Also, because they have
freespace.c callers, I put other thresholds in

src/backend/storage/freespace/indexfsm.c
src/include/access/brin_pageops.h

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.

On the upside, untying free space from heap allowed me to remove most
of the checks for

(rel->rd_rel->relkind == RELKIND_RELATION ||
 rel->rd_rel->relkind == RELKIND_TOASTVALUE)

except for the one in pg_upgrade.c, which is again a bit of a hack, but not bad.

> Hm, given realistic HEAP_FSM_CREATION_THRESHOLD, and the fact that we
> really only need one bit per relation, it seems like map should really
> be just a uint32 with one bit per page.

Done. Regarding the idea upthread about using bytes to store ordinary
freespace values, I think that's better for correctness, but also
makes it more difficult to use different thresholds per access method.

> +static bool
> +fsm_allow_writes(Relation rel, BlockNumber heapblk,
> +                BlockNumber nblocks, BlockNumber *get_nblocks)
>
> +   RelationOpenSmgr(rel);
> +   if (smgrexists(rel->rd_smgr, FSM_FORKNUM))
> +       return true;
>
> Isn't this like really expensive? mdexists() closes the relations and
> reopens it from scratch. Shouldn't we at the very least check
> smgr_fsm_nblocks beforehand, so this is only done once?

I did this in an earlier patch above -- do you have an opinion about that?

I also removed the call to smgrnblocks(smgr, MAIN_FORKNUM) from
XLogRecordPageWithFreeSpace() because I don't think it's actually
needed. There's a window where a table could have 5 blocks, but trying
to record space on, say, block 2 won't actually create the FSM on the
standby. When block 5 fills up enough, then the xlog call will cause
the FSM to be created. Not sure if this is best, but it saves another
syscall, and this function is only called when freespace is less than
20%, IIRC.

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