Re: New FSM patch
От | Heikki Linnakangas |
---|---|
Тема | Re: New FSM patch |
Дата | |
Msg-id | 48D2A399.1020501@enterprisedb.com обсуждение исходный текст |
Ответ на | Re: New FSM patch (Tom Lane <tgl@sss.pgh.pa.us>) |
Список | 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
В списке pgsql-hackers по дате отправления: