Thread: explicit tracking of ActiveSnapshot

Hello,

In the previous installment,
http://archives.postgresql.org/message-id/20080328140606.GL7464@alvh.no-ip.org
I was wondering whether the tracking of snapshots could be made more
robust by having ActiveSnapshot be some sort of stack instead of a
global pointer.  So I set to do that, and it turns out to appear a sane
thing to do.  Here is a patch.

This is all kinda simple: whenever a piece of code wants ActiveSnapshot
to be set, instead of just assigning to it, it calls
PushActiveSnapshot(some-snap).  When it's done with the snap, it calls
PopActiveSnapshot().  When some code needs to grab hold of the active
snapshot, we have GetActiveSnapshot() for it.  There are also a couple
of places that need to know whether there currently is an active
snapshot; for those, we have ActiveSnapshotSet().

In practice this means we can get rid of several PG_TRY blocks that were
only concerned with "restoring" the previous value of ActiveSnapshot in
case of error: we now have snapmgr in charge of removing from the stack
all those snapshots that were set in the current subtransaction.  For
all intents and purposes, this behaves the same as the old coding.

So, if you read the patch you'll notice that it's mostly about changing
ActiveSnapshot uses to either Push, Pop or GetActiveSnapshot.  There are
a few extra hunks that deal with popping a snapshot just before
CommitTransactionCommand -- these are there because I decided that it is
a good idea to complain if someone pops a snapshot and then doesn't push
it.  So things like VACUUM FULL, CREATE INDEX CONCURRENTLY and
multitable CLUSTER must deal with that internally.  ISTM this is a sound
safety measure and it doesn't otherwise cause any other complication.

There is one place where the coding is a bit more involved:
_SPI_execute_plan is rather straightforward, _except_ that currently it
is being called with ActiveSnapshot = InvalidSnapshot -- and then we
call CopySnapshot(InvalidSnapshot).  I'm not sure why this happens, but
in the meantime I've just made sure that we work around that and pass
InvalidSnapshot around as an explicit value instead of stealthly
creating copies of it.

The only disadvantage AFAICS is that there is a somewhat more palloc
traffic.  I don't have numbers, but my guess is that the overall impact
is negligible.

Note: in the patch as attached, I removed some PG_TRY blocks and braces;
however, I did not reindent the contents of these blocks, in order to
keep the patch small.

--
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera wrote:

> In the previous installment,
> http://archives.postgresql.org/message-id/20080328140606.GL7464@alvh.no-ip.org
> I was wondering whether the tracking of snapshots could be made more
> robust by having ActiveSnapshot be some sort of stack instead of a
> global pointer.  So I set to do that, and it turns out to appear a sane
> thing to do.  Here is a patch.

Here is the combined patch, keeping track of both ActiveSnapshot as a
stack and snapshots registered on a list.  I figured out that the
easiest way to manage the memory is to keep track of reference counts in
the snapshot itself, separately for the active snapshot stack and the
registered list, so we know the earliest time to free it.

Open issues:

- SerializableSnapshot is not handled.  (I think this is just a matter
of adding a RegisterSnapshot call as soon as the serializable snapshot
is created).

- Creating the VacuumXmin stuff to actually use it, and decide what to
do with GetOldestXmin.


Comments are very welcome.

--
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.