Re: autovacuum and default_transaction_isolation

I was doing some tests recently with default_transaction_isolation set
to 'serializable' in postgresql.conf when I noticed pg_locks filling up
with SIReadLocks rather more quickly than I expected.

After some investigation, I found that an autovacuum worker was
starting a transaction at the default isolation level. While using a
serializable transaction doesn't affect its behavior (because it's not
using a MVCC snapshot), having a serializable transaction open prevents
other concurrent serializable transactions and their predicate locks
from being cleaned up. Since VACUUM on a large table can take a long
time, this could affect many concurrent transactions.

My one-liner fix for this was to set
DefaultXactIsoLevel = XACT_READ_COMMITTED;
in AutoVacWorkerMain.

Dan

--
Dan R. K. Ports MIT CSAIL http://drkp.net/

Dan Ports <drkp(at)csail(dot)mit(dot)edu> wrote:

> I was doing some tests recently with default_transaction_isolation
> set to 'serializable' in postgresql.conf when I noticed pg_locks
> filling up with SIReadLocks rather more quickly than I expected.
>
> After some investigation, I found that an autovacuum worker was
> starting a transaction at the default isolation level. While using
> a serializable transaction doesn't affect its behavior (because
> it's not using a MVCC snapshot), having a serializable transaction
> open prevents other concurrent serializable transactions and their
> predicate locks from being cleaned up. Since VACUUM on a large
> table can take a long time, this could affect many concurrent
> transactions.
>
> My one-liner fix for this was to set
> DefaultXactIsoLevel = XACT_READ_COMMITTED;
> in AutoVacWorkerMain.

Ouch! I think this needs to be considered a significant bug! Is it
too late to get this into 9.1.2?

-Kevin

Dan Ports <drkp(at)csail(dot)mit(dot)edu> writes:
> After some investigation, I found that an autovacuum worker was
> starting a transaction at the default isolation level. While using a
> serializable transaction doesn't affect its behavior (because it's not
> using a MVCC snapshot), having a serializable transaction open prevents
> other concurrent serializable transactions and their predicate locks
> from being cleaned up. Since VACUUM on a large table can take a long
> time, this could affect many concurrent transactions.

Hmm. Shouldn't we make the autovac launcher use READ COMMITTED, too?

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Shouldn't we make the autovac launcher use READ COMMITTED, too?

Anything which starts a transaction and doesn't need to use a
transaction-lifetime snapshot plus SIRead locks to achieve truly
serializable behavior should probably be ignoring
default_transaction_isolation and forcing READ COMMITTED. I would
think that should include the autovac launcher. Probably *any* of
the background processes which can get a start a transaction should
force READ COMMITTED.

-Kevin

On Tue, Nov 29, 2011 at 07:04:23PM -0500, Tom Lane wrote:
> Hmm. Shouldn't we make the autovac launcher use READ COMMITTED, too?

Yeah, probably. That one doesn't seem so important because its
transactions aren't long-running (IIRC, it only starts a transaction to
scan pg_database). But it wouldn't hurt to keep it from pointlessly
registering a serializable transaction.

Dan

--
Dan R. K. Ports MIT CSAIL http://drkp.net/

Dan Ports <drkp(at)csail(dot)mit(dot)edu> writes:
> I was doing some tests recently with default_transaction_isolation set
> to 'serializable' in postgresql.conf when I noticed pg_locks filling up
> with SIReadLocks rather more quickly than I expected.

> After some investigation, I found that an autovacuum worker was
> starting a transaction at the default isolation level. While using a
> serializable transaction doesn't affect its behavior (because it's not
> using a MVCC snapshot), having a serializable transaction open prevents
> other concurrent serializable transactions and their predicate locks
> from being cleaned up. Since VACUUM on a large table can take a long
> time, this could affect many concurrent transactions.

> My one-liner fix for this was to set
> DefaultXactIsoLevel = XACT_READ_COMMITTED;
> in AutoVacWorkerMain.

I've applied a patch for this in HEAD and 9.1, and also corrected what
seems to be a pre-existing bug that the autovac launcher did not force
zero_damaged_pages and statement_timeout off as autovac workers do.
I did not however touch the similar logic about synchronous_commit,
because I think that's got more problems than this.

Question 1: do we need to back-patch any of these changes further than
9.1?

Question 2: isn't this code broken?

if (synchronous_commit > SYNCHRONOUS_COMMIT_LOCAL_FLUSH)
SetConfigOption("synchronous_commit", "local",
PGC_SUSET, PGC_S_OVERRIDE);

The reason that Dan's one-liner fix isn't good enough, and we instead
need the applied two-liner fix

SetConfigOption("default_transaction_isolation", "read committed",
PGC_SUSET, PGC_S_OVERRIDE);

is that we have to make sure that guc.c knows about the change and will
honor it during subsequent activity. In particular, if SIGHUP
processing were to read a new value of default_transaction_isolation
from postgresql.conf, it would happily override a setting that had
simply been poked into the variable as Dan did it. We need to apply the
desired setting at PGC_S_OVERRIDE level to make sure that it can't be
changed later no matter what.

Because of this, I don't think I believe the above-quoted code for
adjusting synchronous_commit: it's not robust against the possibility of
the DBA changing the setting in postgresql.conf during the lifespan of
the autovac process. I could believe this:

if (synchronous_commit >= SYNCHRONOUS_COMMIT_LOCAL_FLUSH)
SetConfigOption("synchronous_commit", "local",
PGC_SUSET, PGC_S_OVERRIDE);
else
SetConfigOption("synchronous_commit", "off",
PGC_SUSET, PGC_S_OVERRIDE);

but I wonder why we are bothering, and not just forcing it to "off".
The above would still not do exactly what users might expect if the
DBA flips between "local" and "off", so what is it we're trying to
accomplish here? If it is worth worrying about, should the autovac
launcher be doing it too?

And, as mentioned, does anyone think any of these issues are significant
before 9.1?

regards, tom lane

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Shouldn't we make the autovac launcher use READ COMMITTED, too?

> Anything which starts a transaction and doesn't need to use a
> transaction-lifetime snapshot plus SIRead locks to achieve truly
> serializable behavior should probably be ignoring
> default_transaction_isolation and forcing READ COMMITTED. I would
> think that should include the autovac launcher. Probably *any* of
> the background processes which can get a start a transaction should
> force READ COMMITTED.

AFAIK only the autovac processes can run transactions. We'll need to
keep this in mind if we ever create other background processes that
can do so, though.

For the moment I duplicated the existing logic of overriding relevant
GUC variables in the process's Main() function, but I wonder if we ought
to be setting these things in some more centralized place, like
InitPostgres(). That function already knows quite a bit about what
sort of process it's initializing ...

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> For the moment I duplicated the existing logic of overriding
> relevant GUC variables in the process's Main() function,

Thanks!

> but I wonder if we ought to be setting these things in some more
> centralized place, like InitPostgres(). That function already
> knows quite a bit about what sort of process it's initializing ...

It does seem like the sort of thing which might get missed when
creating a new type of process or a new GUC which needs this type of
treatment. Whichever placement seems most likely to get noticed
seems best; one centralized placement seems to me most likely to
attract notice and the necessary thought on the topic

-Kevin