Re: [PATCHES] vacuum hint on elog

2005-01-22 Thread Neil Conway
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

2005-01-22 Thread Tom Lane
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

2005-01-22 Thread Tom Lane
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

2005-01-22 Thread Neil Conway
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])