Re: Visibility map, partial vacuums

Heikki Linnakangas wrote:
> Another thing that does need to be fixed, is the way that the extension 
> and truncation of the visibility map is handled; that's broken in the 
> current patch. I started working on the patch a long time ago, before 
> the FSM rewrite was finished, and haven't gotten around fixing that part 
> yet. We already solved it for the FSM, so we could just follow that 
> pattern. The way we solved truncation in the FSM was to write a separate 
> WAL record with the new heap size, but perhaps we want to revisit that 
> decision, instead of adding again new code to write a third WAL record, 
> for truncation of the visibility map. smgrtruncate() writes a WAL record 
> of its own, if any full blocks are truncated away of the FSM, but we 
> needed a WAL record even if no full blocks are truncated from the FSM 
> file, because the "tail" of the last remaining FSM page, representing 
> the truncated away heap pages, still needs to cleared. Visibility map 
> has the same problem.
> 
> One proposal was to piggyback on the smgrtruncate() WAL-record, and call 
> FreeSpaceMapTruncateRel from smgr_redo(). I considered that ugly from a 
> modularity point of view; smgr.c shouldn't be calling higher-level 
> functions. But maybe it wouldn't be that bad, after all. Or, we could 
> remove WAL-logging from smgrtruncate() altogether, and move it to 
> RelationTruncate() or another higher-level function, and handle the 
> WAL-logging and replay there.

In preparation for the visibility map patch, I revisited the truncation 
issue, and hacked together a patch to piggyback the FSM truncation to 
the main fork smgr truncation WAL record. I moved the WAL-logging from 
smgrtruncate() to RelationTruncate(). There's a new flag to 
RelationTruncate indicating whether the FSM should be truncated too, and 
only one truncation WAL record is written for the operation.

That does seem cleaner than the current approach where the FSM writes a 
separate WAL record just to clear the bits of the last remaining FSM 
page. I had to move RelationTruncate() to smgr.c, because I don't think 
a function in bufmgr.c should be doing WAL-logging. However, 
RelationTruncate really doesn't belong in smgr.c either. Also, now that 
smgrtruncate doesn't write its own WAL record, it doesn't seem right for 
smgrcreate to be doing that either.

So, I think I'll take this one step forward, and move RelationTruncate() 
to a new higher level file, e.g. src/backend/catalog/storage.c, and also 
create a new RelationCreateStorage() function that calls smgrcreate(), 
and move the WAL-logging from smgrcreate() to RelationCreateStorage().

So, we'll have two functions in a new file:

/* Create physical storage for a relation. If 'fsm' is true, an FSM fork 
is also created */
RelationCreateStorage(Relation rel, bool fsm)
/* Truncate the relation to 'nblocks' blocks. If 'fsm' is true, the FSM 
is also truncated */
RelationTruncate(Relation rel, BlockNumber nblocks, bool fsm)

The next question is whether the "pending rel deletion" stuff in smgr.c 
should be moved to the new file too. It seems like it would belong there 
better. That would leave smgr.c as a very thin wrapper around md.c

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