On 8/1/07, Simon Riggs <[EMAIL PROTECTED]> wrote: > > On Wed, 2007-08-01 at 14:36 +0530, Pavan Deolasee wrote: > > > BufferIsLockedForCleanup() should be named BufferIsAvilableForCleanup(). > There is no cleanup mode, what we mean is that there is only one pin; > the comments say "If we are lucky enough to have already acquired the > cleanup lock", which made me look for the point where we had executed > LockBufferForCleanup(), which we never do.
Ok. I am open to suggestions here as well as other parts of the code about naming. heap_page_prune() needs to comment what prune_hard means. I think you > mean true=prune page, false=prune just this hot chain. It might be > better to have a prune type so that the code reads as PRUNE_HOT_CHAIN or > PRUNE_WHOLE_PAGE. Does it means the same thing as in > heap_prune_tuplechain()? No. prune_hard means remove all RECENTLY_DEAD tuples before a DEFINITELY DEAD tuple. We use this only for VACUUM FULL. The pruneoff is set to InvalidOffsetNumber to prune all tuples chains in the page. I'm concerned that HeapTupleSatisfiesAbort() may be very expensive and > yet almost never return true. Do we need this code at this point? We actually run SatisfiesVacuum anyways on all the tuples. We should expect hint bits to be set and hence the call may not be expensive. We need this to reclaims aborted heap-only tuples which otherwise are not reachable. It would be good to put some explanatory notes somewhere. > Do all calls to PageGetFreeSpace() get replaced by > PageHeapGetFreeSpace()? Ok. Would do that. Right now, PageHeapGetFreeSpace is used for heap pages. To answer some of your XXXs: > > > > XXX Should we set PageSetPrunable on this page ? If the transaction > aborts, there will be a prunable tuple in this page. > Think you need to explain a little more...but we probably shouldn't tune > for aborts. If the INSERTing transaction aborts, we are left with a DEAD tuple in the page which can be pruned and removed. So the question is whether we should set the hint bit. If we don't set the hint bit, the aborted DEAD tuple would stay there till the next vacuum or some other tuple in the page is UPDATED/DELETEd and the page is cleaned up. XXX Should we set hint on the newbuf as well ? If the transaction > aborts, there would be a prunable tuple in the newbuf. > Think you need to explain a little more...but we probably shouldn't tune > for aborts. Same as above. XXX Should we update the FSM information of this page ? > (after pruning) > No. Updating FSM would only take place if updates are taking place on > this block. We would make this page a target for other INSERTs and > UPDATEs, so would effectively bring more contention onto those pages. > Bad Thing. On the other hand, if we don't update FSM all COLD updates may result in extending the relation even though there is some free space available elsewhere in the relation. One option could be to leave fillfactor worth of space and update FSM accordingly. XXX We may want to tweak the percentage value below: > (1.2 * RelationGetAvgFSM(relation) > Not sure what difference the 1.2 makes... Thats a quick hack. I thought of leaving some margin because avg FSM request size is a moving average and there is a good chance to err. So that means I agree with all of your XXXs apart from the last one. > > > The logic of chain pruning has been simplified a lot. In addition, > > there > > are fewer new WAL log records. We rather reuse the existing WAL > > record types to log the operations. > > I can't find any mention of XLogInserts for the new record types. > > e.g. XLOG_HEAP2_PRUNEHOT is only mentioned on one line in this patch. > > I this a mistake, or could you explain? Sorry, thats just a left over from the previous version. I shall clean that up. > It would be good to see the results... I shall post them soon. Thanks, Pavan -- Pavan Deolasee EnterpriseDB http://www.enterprisedb.com