Re: New FSM patch

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
> Here's a new patch, updated per your comments.

I did a read-through of the portions of this patch that change the rest
of the system (ie, not the guts of the new FSM itself). Mostly it looks
pretty nice, but I have a few gripes:

Does smgrimmedsync at the bottom of nbtsort.c need to cover FSM too?
Maybe not, since index's FSM should be empty, but in that case you
still should add a comment saying so.

Likewise for smgrimmedsync in tablecmds.c's copy_relation_data

Grepping for P_NEW suggests that you missed some places where
FreeSpaceMapExtendRel or IndexFreeSpaceMapExtend calls should be added.
In particular GiST/GIN. (I assume hash indexes still don't use FSM.
I wonder whether it'd be a good idea to get rid of hash bitmap pages
in favor of using FSM? TODO item, not material for this patch.)

The change in catalog/heap.c invalidates the comment immediately
above it.

In vacuum.c's vac_update_fsm, the outPages counter is now useless.

In vacuumlazy.c, the num_free_pages, max_free_pages, and tot_free_pages
members of LVRelStats are now useless, as is the comment above them.
You need to take out the reporting of tot_free_pages if you are not
going to track it.

I think it's a modularity violation for bufmgr.c to be calling FSM.
Furthermore, it's pretty useless to have RelationTruncate doing
the fixup only for heaps and not indexes. Please move that out
to the callers instead.

Does smgr.c still need to include storage/freespace.h at all?
Again, I think it would be a modularity violation to have it
calling FSM, now that FSM lives on top of storage.

RESOURCES_FSM needs to be removed from utils/guc_tables.h

The NOTE in the enum ForkNumber declaration was wrong before and
still is.

GetFreeSpaceOnPage() seems a bit misleadingly named; it's not obvious
from the name that it's not giving you the *true* free space on the page
but rather what FSM thinks it is. Maybe call it something like
GetRecordedFreeSpace(). Also, please do not use the declaration style
that omits parameter names; I think those are important for
documentation/readability.

Doc updates are missing, but you knew that.

regards, tom lane