Thread: vacuum hint on elog

It occurred to me that if we elog(ERROR) during VACUUM, the vacuum
activity hint will not be reset. This will result in all subsequent I/O
by the current backend being treated by the bufmgr as though it resulted
from VACUUM. While elog(ERROR) during VACUUM is not a common occurrence,
I don't think it's wise to assume it is impossible.

Attached is a patch which resets the vacuum activity hint in
AbortTransaction().

Barring any objections, I intend to apply this to REL8_0_STABLE and HEAD
sometime on Monday.

-Neil
Index: src/backend/access/transam/xact.c
===================================================================
RCS file: /Users/neilc/local/cvs/pgsql/src/backend/access/transam/xact.c,v
retrieving revision 1.195
diff -c -r1.195 xact.c
*** src/backend/access/transam/xact.c    31 Dec 2004 21:59:29 -0000    1.195
--- src/backend/access/transam/xact.c    22 Jan 2005 12:07:16 -0000
***************
*** 32,37 ****
--- 32,38 ----
  #include "executor/spi.h"
  #include "libpq/be-fsstubs.h"
  #include "miscadmin.h"
+ #include "storage/buf_internals.h"
  #include "storage/fd.h"
  #include "storage/proc.h"
  #include "storage/sinval.h"
***************
*** 1606,1611 ****
--- 1607,1621 ----
       */
      LWLockReleaseAll();

+     /*
+      * Reset the VACUUM activity hint. If we did elog(ERROR) during
+      * VACUUM, this might still be set to true, so reset it here. For
+      * the sake of correctness, we make sure to reset the flag fairly
+      * early, before we've done any I/O that may be required for txn
+      * abort.
+      */
+     StrategyHintVacuum(false);
+
      /* Clean up buffer I/O and buffer context locks, too */
      AbortBufferIO();
      UnlockBuffers();

On Sat, Jan 22, 2005 at 11:28:52PM +1100, Neil Conway wrote:
> It occurred to me that if we elog(ERROR) during VACUUM, the vacuum
> activity hint will not be reset. This will result in all subsequent I/O
> by the current backend being treated by the bufmgr as though it resulted
> from VACUUM. While elog(ERROR) during VACUUM is not a common occurrence,
> I don't think it's wise to assume it is impossible.

Hmm ... I think you should rather use a PG_TRY/PG_CATCH block.  (Or
discover whether this is already done somewhere else ...)

--
Alvaro Herrera (<alvherre[@]dcc.uchile.cl>)
"El día que dejes de cambiar dejarás de vivir"

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

Neil Conway <neilc@samurai.com> 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

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

Neil Conway <neilc@samurai.com> 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