Re: HOT patch - version 11

On 8/1/07, Simon Riggs <simon(at)2ndquadrant(dot)com> 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