Re: New FSM patch

Tom Lane wrote:
> 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:

Thanks. It's probably not worthwhile to dig too deep into the FSM
internals, until the performance problem is solved.

> 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.

Right, the FSM should be empty at that point, it's extended in btbuild
after that. nbtsort.c isn't concerned about FSM at all. I can add a
comment on that.

> Likewise for smgrimmedsync in tablecmds.c's copy_relation_data

Well, no, copy_relation_data() copies and syncs only one fork at a time.
Hmm, perhaps it should be renamed to reflect that, copy_fork() might be
more appropriate.

> Grepping for P_NEW suggests that you missed some places where
> FreeSpaceMapExtendRel or IndexFreeSpaceMapExtend calls should be added.
> In particular GiST/GIN.

Hmm, actually I think there's a problem with this approach to extending.
If we crash after extending the file, but before the FSM extension has
been WAL-logged, the next time the relation is vacuumed, vacuum will try
to mark the page that FSM doesn't know about as free, and
RecordPageWithFreeSpace doesn't like that.

I think we'll have to chance that rule so that the FSM is extended
automatically when RecordPageWithFreeSpace() is called on a page that's
not in the FSM yet. That way we also won't need to remember to extend
the FSM whenever the relation is extended.

That's easy to do by always calling FreeSpaceMapExtendRel from
RecordPageWithFreeSpace(), but I'm afraid of the performance implication
of that. Perhaps we could store the previously observed size of the FSM
in RelationData, and only try to extend it when we get a request for a
page beyond that. I think we'll then need locking to avoid extending the
FSM from two backends at the same time, though, I'm relying on the
extension lock of the main relation at the moment.

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

Thanks. I'm actually a bit unhappy about creating the FSM fork there.

> 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 took them all out now, though it would be nice to keep tracking it.

> 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.

Hmm, seems to work fine without it.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com