Re: New FSM patch - Mailing list pgsql-hackers

Tom Lane wrote:
> Heikki Linnakangas <heikki.linnakangas@enterprisedb.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