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