Re: [PATCHES] vacuum hint on elog
Alvaro Herrera wrote: Hmm ... I think you should rather use a PG_TRY/PG_CATCH block. Thanks for the suggestion, Alvaro -- I think that's a better way to go. It means we can keep vacuum-specific stuff in vacuum.c, rather than adding to AbortTransaction(). I'll post a revised patch tomorrow. While we're on the subject, the set_bool_flag do_something_complex reset_bool_flag coding pattern is really more fragile than it would initially seem to be. It is basically another variant of resource management, in the same way that manual memory management or explicit reference counting can be tricky to get right. For example, if a function that enables the vacuum hint recursively invokes itself, it is liable to reset the vacuum hint earlier than intended (vacuum_rel() comes close to making this mistake, although it does things right). We could make the vacuum hint a counter rather than a bool (bump the counter on enable hint, decrement it on disable hint, and treat hint 0 as enabled), but that just changes the error case slightly -- if you forget to bump/decrement the counter, you're still in trouble. Perhaps to make this a bit less error prone we could add an assert/elog to StrategyHintVacuum(), which would raise an error/warning if the hint is enabled when it is already true. We shouldn't warn if the flag is disabled when it is already false, since (a) that is harmless (b) it is legitimate in an exception handler, as you suggested. -Neil ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] vacuum hint on elog
Neil Conway [EMAIL PROTECTED] writes: It occurred to me that if we elog(ERROR) during VACUUM, the vacuum activity hint will not be reset. The code beginning at freelist.c line 645 is intended to deal with this. Attached is a patch which resets the vacuum activity hint in AbortTransaction(). I dislike exposing such low-level issues to AbortTransaction(). If an explicit reset is needed, there should be something like an AtAbort_Buffers() call that does this as well as any other low-level issues needed (eg AbortBufferIO()). Note that you missed AbortSubTransaction() anyway. regards, tom lane ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] vacuum hint on elog
Neil Conway [EMAIL PROTECTED] writes: Tom Lane wrote: The code beginning at freelist.c line 645 is intended to deal with this. Ah, good point -- sorry, I missed that. The code as-is should be fine, then. Well, one point is that the flag bit is checked elsewhere in the same file without similar code to see if it's stale. I'm not sure if the other places can be reached without having reached line 645 first. I tend to agree with Alvaro that a cleaner approach would be to PG_TRY around the places that can set the flag, and get rid of the cleanup logic at line 645. We didn't have PG_TRY when that code was written, but we do now ... regards, tom lane ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send unregister YourEmailAddressHere to [EMAIL PROTECTED])
Re: [PATCHES] vacuum hint on elog
Tom Lane wrote: The code beginning at freelist.c line 645 is intended to deal with this. Ah, good point -- sorry, I missed that. The code as-is should be fine, then. -Neil ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send unregister YourEmailAddressHere to [EMAIL PROTECTED])