Re: [PATCHES] HOT synced with HEAD
On 9/17/07, Tom Lane [EMAIL PROTECTED] wrote: Yeah. As the code stands, anything that's XMIN_INVALID will be considered not-HotUpdated (look at the macro...). So far I've seen no place where there is any value in following a HOT chain past such a tuple --- do you see any? No, I don't think we would ever need to follow a HOT chain past the aborted tuple. The only thing that worries about this setup though is the dependency on hint bits being set properly. But the places where this would be used right now for detecting aborted dead tuples, apply HeapTupleSatisfiesVacuum on the tuple before checking for HeapTupleIsHotUpdated, so we are fine. Or should we just check for XMIN_INVALID explicitly at those places ? Thanks, Pavan -- Pavan Deolasee EnterpriseDB http://www.enterprisedb.com
Re: [PATCHES] HOT synced with HEAD
On 9/17/07, Tom Lane [EMAIL PROTECTED] wrote: Meanwhile I've started looking at the vacuum code, and it seems that v16 has made that part of the patch significantly worse. VACUUM will fail to count tuples that are removed by pruning, which seems like something it should report somehow. I understand. I did not give real weight to it because I thought we anyways remove tuples elsewhere during pruning. But I agree that the heap_page_prune_defrag in the VACUUM code path is doing so on behalf of vacuum and hence we should credit that to VACUUM. And you've introduced a race condition: as I just mentioned, it's perfectly possible that the second call of HeapTupleSatisfiesVacuum gets a different answer than what the prune code saw, especially in lazy VACUUM (in VACUUM FULL it'd suggest that someone released lock early ... but we do have to cope with that). Hmm.. you are right. Those extra notices I added are completely unnecessary and wrong. The comments you added seem to envision a more invasive patch that gets rid of the second HeapTupleSatisfiesVacuum pass altogether, but I'm not sure how practical that is, and am not real inclined to try to do it right now anyway ... I agree. I just wanted to leave a hint there that such a possibility exists if someone really wants to optimize, now or later. Thanks, Pavan -- Pavan Deolasee EnterpriseDB http://www.enterprisedb.com
Re: [PATCHES] HOT synced with HEAD
On 9/16/07, Tom Lane [EMAIL PROTECTED] wrote: Attached is as far as I've gotten with reviewing the HOT patch; I see that Pavan is still fooling with it so we'd better re-sync. I am done with most of the items on my plate. I was not sure how far you have gone with the patch, so was trying to finish as many items as possible myself. Now that I know you have started refactoring the code, I would try not to change it unless its urgent. And when its required, I will send a add-on patch just the way I did earlier. Let me know if you want me to focus of something at this point. * Many API cleanups, in particular I didn't like what had been done to heap_fetch and thought it better to split out the chain-following logic I liked those changes. I am wondering if we could have avoided duplicating the chain following code in heap_hot_search_buffer and index_getnext. But I trust your judgment. I also liked the way you reverted the API changes to various index build methods. I would test the patch tomorrow in detail. Thanks, Pavan -- Pavan Deolasee EnterpriseDB http://www.enterprisedb.com
Re: [PATCHES] HOT synced with HEAD
Pavan Deolasee [EMAIL PROTECTED] writes: I liked those changes. I am wondering if we could have avoided duplicating the chain following code in heap_hot_search_buffer and index_getnext. I wasn't totally thrilled with duplicating that code, but the callers of heap_hot_search are only interested in finding a single visible tuple, whereas index_getnext has to find them all on successive visits. Exposing a capability to do that in heap_hot_search seemed to make its API ugly enough that duplicated code was better. (I really didn't like the original setup where it was supposed to find the next tuple after the one passed in, thereby forcing the caller to handle the first chain member...) BTW, I'm in process of taking out the separate HEAPTUPLE_DEAD_CHAIN return value from HeapTupleSatisfiesVacuum. After looking through all the callers I concluded that this complicated life more than it helped. The places that actually want to make the distinction can check HeapTupleIsHotUpdated easily enough for themselves, and in several of the places that don't want to make the distinction it's notationally cumbersome to deal with it. For instance, in the patch as I posted it last night, both heap_hot_search and index_getnext probably fail to detect all-dead HOT chains, because some of the chain members are likely to be HEAPTUPLE_DEAD_CHAIN rather than plain DEAD, and they're not coping. regards, tom lane ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] HOT synced with HEAD
On 9/16/07, Tom Lane [EMAIL PROTECTED] wrote: BTW, I'm in process of taking out the separate HEAPTUPLE_DEAD_CHAIN return value from HeapTupleSatisfiesVacuum. I agree. I myself suggested doing so earlier in the discussion (I actually even removed this before I sent out the add-on patch last night, but then reverted back because I realized at least it is required at one place) The place where I thought its required is to avoid marking an index tuple dead even though the corresponding root tuple is dead and the root tuple was HOT updated. But seems like you have already put in a different mechanism to handle that. So we should be able to get rid of HEAPTUPLE_DEAD_CHAIN. Thanks, Pavan -- Pavan Deolasee EnterpriseDB http://www.enterprisedb.com
Re: [PATCHES] HOT synced with HEAD
BTW, I am still looking for a reason for the hard-prune logic to live. It seems to complicate matters far more than it's worth --- in particular the way that the WAL replay representation is set up seems confusing and fragile. (If prune_hard is set, the redirect entries mean something completely different.) There was some suggestion that VACUUM FULL has to have it, but unless I see proof of that I'm thinking of taking it out. regards, tom lane ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] HOT synced with HEAD
On 9/16/07, Tom Lane [EMAIL PROTECTED] wrote: Something else I was just looking at: in the pruning logic, SetRedirect and SetDead operations are done at the same time that we record them for the eventual WAL record creation, but SetUnused operations are postponed and only done at the end. This seems a bit odd/nonorthogonal. Furthermore it's costing extra code complexity: if you were to SetUnused immediately then you wouldn't need that bitmap thingy to prevent you from doing it twice. I think that the reason it's like that is probably because of the problem of potential multiple visits to a DEAD heap-only tuple, but it looks to me like that's not really an issue given the current form of the testing for aborted tuples, which I have as if (HeapTupleSatisfiesVacuum(tuple-t_data, global_xmin, buffer) == HEAPTUPLE_DEAD !HeapTupleIsHotUpdated(tuple)) heap_prune_record_unused(nowunused, nunused, unusedbitmap, rootoffnum); ISTM that if HeapTupleIsHotUpdated is false, we could simply SetUnused immediately, because any potential chain members after this one must be dead too, and they'll get reaped by this same bit of code --- there is no need to preserve the chain. (The only case we're really covering here is XMIN_INVALID, and any later chain members must certainly be XMIN_INVALID as well.) When the HOT-chain is later followed, we'll detect chain break anyway, so I see no need to postpone clearing the item pointer. So are you suggesting we go back to the earlier way of handling aborted tuples separately ? But then we can not do that by simply checking for !HeaptupleIsHotUpdated. There could be several aborted tuples at the end of the chain of which all but one are marked HotUpdated. Or are you suggesting we also check for XMIN_INVALID for detecting aborted tuples ? If we handle aborted tuples differently, then we don't need that extra bitmap and can in fact set the line pointer unused immediately. Otherwise, as you rightly pointed out, we might break a chain before we prune it. Thanks, Pavan -- Pavan Deolasee EnterpriseDB http://www.enterprisedb.com
Re: [PATCHES] HOT synced with HEAD
Pavan Deolasee [EMAIL PROTECTED] writes: So are you suggesting we go back to the earlier way of handling aborted tuples separately ? But then we can not do that by simply checking for !HeaptupleIsHotUpdated. There could be several aborted tuples at the end of the chain of which all but one are marked HotUpdated. Or are you suggesting we also check for XMIN_INVALID for detecting aborted tuples ? Yeah. As the code stands, anything that's XMIN_INVALID will be considered not-HotUpdated (look at the macro...). So far I've seen no place where there is any value in following a HOT chain past such a tuple --- do you see any? Every descendant tuple must be XMIN_INVALID as well ... regards, tom lane ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
